From 600617c31c0d7cfd31cb62ebbd786a408fc090a1 Mon Sep 17 00:00:00 2001 From: Dom Christie Date: Sat, 20 Jul 2024 10:50:33 +0100 Subject: [PATCH 1/5] Tidy history popstate handling. Safari stopped triggering popstate on load from version 10 (~2016) so we can safely remove this workaround. https://developer.mozilla.org/en-US/docs/Web/API/Window/popstate_event#the_history_stack --- src/core/drive/history.js | 39 +++++++++------------------------------ 1 file changed, 9 insertions(+), 30 deletions(-) diff --git a/src/core/drive/history.js b/src/core/drive/history.js index 45015b0cc..569b6a51d 100644 --- a/src/core/drive/history.js +++ b/src/core/drive/history.js @@ -1,11 +1,10 @@ -import { nextMicrotask, uuid } from "../../util" +import { uuid } from "../../util" export class History { location restorationIdentifier = uuid() restorationData = {} started = false - pageLoaded = false currentIndex = 0 constructor(delegate) { @@ -15,7 +14,6 @@ export class History { start() { if (!this.started) { addEventListener("popstate", this.onPopState, false) - addEventListener("load", this.onPageLoad, false) this.currentIndex = history.state?.turbo?.restorationIndex || 0 this.started = true this.replace(new URL(window.location.href)) @@ -25,7 +23,6 @@ export class History { stop() { if (this.started) { removeEventListener("popstate", this.onPopState, false) - removeEventListener("load", this.onPageLoad, false) this.started = false } } @@ -81,32 +78,14 @@ export class History { // Event handlers onPopState = (event) => { - if (this.shouldHandlePopState()) { - const { turbo } = event.state || {} - if (turbo) { - this.location = new URL(window.location.href) - const { restorationIdentifier, restorationIndex } = turbo - this.restorationIdentifier = restorationIdentifier - const direction = restorationIndex > this.currentIndex ? "forward" : "back" - this.delegate.historyPoppedToLocationWithRestorationIdentifierAndDirection(this.location, restorationIdentifier, direction) - this.currentIndex = restorationIndex - } + const { turbo } = event.state || {} + if (turbo) { + this.location = new URL(window.location.href) + const { restorationIdentifier, restorationIndex } = turbo + this.restorationIdentifier = restorationIdentifier + const direction = restorationIndex > this.currentIndex ? "forward" : "back" + this.delegate.historyPoppedToLocationWithRestorationIdentifierAndDirection(this.location, restorationIdentifier, direction) + this.currentIndex = restorationIndex } } - - onPageLoad = async (_event) => { - await nextMicrotask() - this.pageLoaded = true - } - - // Private - - shouldHandlePopState() { - // Safari dispatches a popstate event after window's load event, ignore it - return this.pageIsLoaded() - } - - pageIsLoaded() { - return this.pageLoaded || document.readyState == "complete" - } } From 783517ced190e342554ab16113faf26a27cb4836 Mon Sep 17 00:00:00 2001 From: Dom Christie Date: Sat, 20 Jul 2024 21:05:00 +0100 Subject: [PATCH 2/5] Let browsers handle same-page anchor clicks --- src/util.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util.js b/src/util.js index 13ffb6b40..4e90ba3bb 100644 --- a/src/util.js +++ b/src/util.js @@ -253,6 +253,8 @@ export function findLinkFromClickTarget(target) { const linkTarget = link.getAttribute("target") if (linkTarget && linkTarget !== "_self") return null + if (/\A#/.test(link.href)) return null + return link } From 50ff9c7e7587a96b33d9ed87fd63732f0aeacc70 Mon Sep 17 00:00:00 2001 From: Dom Christie Date: Sat, 20 Jul 2024 21:18:51 +0100 Subject: [PATCH 3/5] Handle same-page anchor clicks by checking for absence of popstate data. Absent popstate event data suggests that the event originates from a same-page anchor click. Reconcile the empty state by incrementing the History's currentIndex, replacing the null history entry, setting the View's lastRenderedLocation, and then caching it --- src/core/drive/history.js | 11 +++++++---- src/core/session.js | 12 +++++++++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/core/drive/history.js b/src/core/drive/history.js index 569b6a51d..16f11da53 100644 --- a/src/core/drive/history.js +++ b/src/core/drive/history.js @@ -78,14 +78,17 @@ export class History { // Event handlers onPopState = (event) => { - const { turbo } = event.state || {} - if (turbo) { - this.location = new URL(window.location.href) - const { restorationIdentifier, restorationIndex } = turbo + this.location = new URL(window.location.href) + + if (event.state?.turbo) { + const { restorationIdentifier, restorationIndex } = event.state.turbo this.restorationIdentifier = restorationIdentifier const direction = restorationIndex > this.currentIndex ? "forward" : "back" this.delegate.historyPoppedToLocationWithRestorationIdentifierAndDirection(this.location, restorationIdentifier, direction) this.currentIndex = restorationIndex + } else { + this.currentIndex++ + this.delegate.historyPoppedWithEmptyState(this.location) } } } diff --git a/src/core/session.js b/src/core/session.js index a43112831..3a090d48f 100644 --- a/src/core/session.js +++ b/src/core/session.js @@ -216,6 +216,10 @@ export class Session { } } + historyPoppedWithEmptyState(location) { + this.#reconcileEmptyHistoryEntry(location) + } + // Scroll observer delegate scrollPositionChanged(position) { @@ -260,7 +264,7 @@ export class Session { // Navigator delegate allowsVisitingLocationWithAction(location, action) { - return this.locationWithActionIsSamePage(location, action) || this.applicationAllowsVisitingLocation(location) + return this.applicationAllowsVisitingLocation(location) } visitProposedToLocation(location, options) { @@ -496,6 +500,12 @@ export class Session { get snapshot() { return this.view.snapshot } + + #reconcileEmptyHistoryEntry(location) { + this.history.replace(location) + this.view.lastRenderedLocation = location + this.view.cacheSnapshot() + } } // Older versions of the Turbo Native adapters referenced the From e591ea9eb087a308f8d6997dc3ffe051058e3ec9 Mon Sep 17 00:00:00 2001 From: Dom Christie Date: Sat, 20 Jul 2024 21:19:49 +0100 Subject: [PATCH 4/5] Clean up unnecessary code now that same-page link clicks are handled outside a Visit --- src/core/drive/navigator.js | 18 ++++-------------- src/core/drive/visit.js | 25 ++----------------------- src/core/native/browser_adapter.js | 1 - src/core/session.js | 25 ++----------------------- src/util.js | 3 +-- 5 files changed, 9 insertions(+), 63 deletions(-) diff --git a/src/core/drive/navigator.js b/src/core/drive/navigator.js index 195b46a48..454ce766f 100644 --- a/src/core/drive/navigator.js +++ b/src/core/drive/navigator.js @@ -1,6 +1,6 @@ import { getVisitAction } from "../../util" import { FormSubmission } from "./form_submission" -import { expandURL, getAnchor, getRequestURL } from "../url" +import { expandURL } from "../url" import { Visit } from "./visit" import { PageSnapshot } from "./page_snapshot" @@ -139,20 +139,10 @@ export class Navigator { delete this.currentVisit } + // Same-page links are no longer handled with a Visit. + // This method is still needed for Turbo Native adapters. locationWithActionIsSamePage(location, action) { - const anchor = getAnchor(location) - const currentAnchor = getAnchor(this.view.lastRenderedLocation) - const isRestorationToTop = action === "restore" && typeof anchor === "undefined" - - return ( - action !== "replace" && - getRequestURL(location) === getRequestURL(this.view.lastRenderedLocation) && - (isRestorationToTop || (anchor != null && anchor !== currentAnchor)) - ) - } - - visitScrolledToSamePageLocation(oldURL, newURL) { - this.delegate.visitScrolledToSamePageLocation(oldURL, newURL) + return false } // Visits diff --git a/src/core/drive/visit.js b/src/core/drive/visit.js index 59896b630..092b73b01 100644 --- a/src/core/drive/visit.js +++ b/src/core/drive/visit.js @@ -85,7 +85,6 @@ export class Visit { this.snapshot = snapshot this.snapshotHTML = snapshotHTML this.response = response - this.isSamePage = this.delegate.locationWithActionIsSamePage(this.location, this.action) this.isPageRefresh = this.view.isPageRefresh(this) this.visitCachedSnapshot = visitCachedSnapshot this.willRender = willRender @@ -113,10 +112,6 @@ export class Visit { return this.history.getRestorationDataForIdentifier(this.restorationIdentifier) } - get silent() { - return this.isSamePage - } - start() { if (this.state == VisitState.initialized) { this.recordTimingMetric(TimingMetric.visitStart) @@ -253,7 +248,7 @@ export class Visit { const isPreview = this.shouldIssueRequest() this.render(async () => { this.cacheSnapshot() - if (this.isSamePage || this.isPageRefresh) { + if (this.isPageRefresh) { this.adapter.visitRendered(this) } else { if (this.view.renderPromise) await this.view.renderPromise @@ -281,17 +276,6 @@ export class Visit { } } - goToSamePageAnchor() { - if (this.isSamePage) { - this.render(async () => { - this.cacheSnapshot() - this.performScroll() - this.changeHistory() - this.adapter.visitRendered(this) - }) - } - } - // Fetch request delegate prepareRequest(request) { @@ -353,9 +337,6 @@ export class Visit { } else { this.scrollToAnchor() || this.view.scrollToTop() } - if (this.isSamePage) { - this.delegate.visitScrolledToSamePageLocation(this.view.lastRenderedLocation, this.location) - } this.scrolled = true } @@ -394,9 +375,7 @@ export class Visit { } shouldIssueRequest() { - if (this.isSamePage) { - return false - } else if (this.action == "restore") { + if (this.action == "restore") { return !this.hasCachedSnapshot() } else { return this.willRender diff --git a/src/core/native/browser_adapter.js b/src/core/native/browser_adapter.js index c071f9ac5..fc99ce21e 100644 --- a/src/core/native/browser_adapter.js +++ b/src/core/native/browser_adapter.js @@ -24,7 +24,6 @@ export class BrowserAdapter { visit.loadCachedSnapshot() visit.issueRequest() - visit.goToSamePageAnchor() } visitRequestStarted(visit) { diff --git a/src/core/session.js b/src/core/session.js index 3a090d48f..ec7fabe50 100644 --- a/src/core/session.js +++ b/src/core/session.js @@ -280,9 +280,7 @@ export class Session { this.view.markVisitDirection(visit.direction) } extendURLWithDeprecatedProperties(visit.location) - if (!visit.silent) { - this.notifyApplicationAfterVisitingLocation(visit.location, visit.action) - } + this.notifyApplicationAfterVisitingLocation(visit.location, visit.action) } visitCompleted(visit) { @@ -291,14 +289,6 @@ export class Session { this.notifyApplicationAfterPageLoad(visit.getTimingMetrics()) } - locationWithActionIsSamePage(location, action) { - return this.navigator.locationWithActionIsSamePage(location, action) - } - - visitScrolledToSamePageLocation(oldURL, newURL) { - this.notifyApplicationAfterVisitingSamePageLocation(oldURL, newURL) - } - // Form submit observer delegate willSubmitForm(form, submitter) { @@ -338,9 +328,7 @@ export class Session { // Page view delegate viewWillCacheSnapshot() { - if (!this.navigator.currentVisit?.silent) { - this.notifyApplicationBeforeCachingSnapshot() - } + this.notifyApplicationBeforeCachingSnapshot() } allowsImmediateRender({ element }, options) { @@ -432,15 +420,6 @@ export class Session { }) } - notifyApplicationAfterVisitingSamePageLocation(oldURL, newURL) { - dispatchEvent( - new HashChangeEvent("hashchange", { - oldURL: oldURL.toString(), - newURL: newURL.toString() - }) - ) - } - notifyApplicationAfterFrameLoad(frame) { return dispatch("turbo:frame-load", { target: frame }) } diff --git a/src/util.js b/src/util.js index 4e90ba3bb..5a9024a1f 100644 --- a/src/util.js +++ b/src/util.js @@ -248,13 +248,12 @@ export function findLinkFromClickTarget(target) { const link = findClosestRecursively(target, "a[href], a[xlink\\:href]") if (!link) return null + if (link.href.startsWith("#")) return null if (link.hasAttribute("download")) return null const linkTarget = link.getAttribute("target") if (linkTarget && linkTarget !== "_self") return null - if (/\A#/.test(link.href)) return null - return link } From ffc3ea30491616197ae7691e9ac8d0ec6f89f115 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Fri, 14 Nov 2025 09:07:35 -0500 Subject: [PATCH 5/5] Reduce diff size and pass failing test --- src/core/drive/history.js | 5 +++-- src/core/session.js | 10 +++------- src/tests/functional/navigation_tests.js | 2 +- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/core/drive/history.js b/src/core/drive/history.js index 16f11da53..868d1fc9e 100644 --- a/src/core/drive/history.js +++ b/src/core/drive/history.js @@ -78,10 +78,11 @@ export class History { // Event handlers onPopState = (event) => { + const { turbo } = event.state || {} this.location = new URL(window.location.href) - if (event.state?.turbo) { - const { restorationIdentifier, restorationIndex } = event.state.turbo + if (turbo) { + const { restorationIdentifier, restorationIndex } = turbo this.restorationIdentifier = restorationIdentifier const direction = restorationIndex > this.currentIndex ? "forward" : "back" this.delegate.historyPoppedToLocationWithRestorationIdentifierAndDirection(this.location, restorationIdentifier, direction) diff --git a/src/core/session.js b/src/core/session.js index ec7fabe50..1e9f1c7bd 100644 --- a/src/core/session.js +++ b/src/core/session.js @@ -217,7 +217,9 @@ export class Session { } historyPoppedWithEmptyState(location) { - this.#reconcileEmptyHistoryEntry(location) + this.history.replace(location) + this.view.lastRenderedLocation = location + this.view.cacheSnapshot() } // Scroll observer delegate @@ -479,12 +481,6 @@ export class Session { get snapshot() { return this.view.snapshot } - - #reconcileEmptyHistoryEntry(location) { - this.history.replace(location) - this.view.lastRenderedLocation = location - this.view.cacheSnapshot() - } } // Older versions of the Turbo Native adapters referenced the diff --git a/src/tests/functional/navigation_tests.js b/src/tests/functional/navigation_tests.js index 6648558ef..ef70a1256 100644 --- a/src/tests/functional/navigation_tests.js +++ b/src/tests/functional/navigation_tests.js @@ -299,7 +299,7 @@ test("following a same-origin link inside an SVG element", async ({ page }) => { await page.keyboard.press("Enter") await expect(page).toHaveURL(withPathname("/src/tests/fixtures/one.html")) - expect(await visitAction(page)).toEqual("advance") + expect(await visitAction(page)).toEqual("load") }) test("following a cross-origin link inside an SVG element", async ({ page }) => {