Skip to content

Simplify same page anchor visits #1285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jorgemanrubia merged 5 commits into from
Nov 18, 2025
Merged

Simplify same page anchor visits #1285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
600617c
Tidy history popstate handling.
domchristie Jul 20, 2024
783517c
Let browsers handle same-page anchor clicks
domchristie Jul 20, 2024
50ff9c7
Handle same-page anchor clicks by checking for absence of popstate data.
domchristie Jul 20, 2024
e591ea9
Clean up unnecessary code now that same-page link clicks are handled …
domchristie Jul 20, 2024
ffc3ea3
Reduce diff size and pass failing test
seanpdoyle Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 13 additions & 30 deletions src/core/drive/history.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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) {
Expand All @@ -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))
Expand All @@ -25,7 +23,6 @@ export class History {
stop() {
if (this.started) {
removeEventListener("popstate", this.onPopState, false)
removeEventListener("load", this.onPageLoad, false)
this.started = false
}
}
Expand Down Expand Up @@ -81,32 +78,18 @@ 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 || {}
this.location = new URL(window.location.href)

if (turbo) {
const { restorationIdentifier, restorationIndex } = 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)
}
}

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"
}
}
18 changes: 4 additions & 14 deletions src/core/drive/navigator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"

Expand Down Expand Up @@ -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
Expand Down
25 changes: 2 additions & 23 deletions src/core/drive/visit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion src/core/native/browser_adapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ export class BrowserAdapter {

visit.loadCachedSnapshot()
visit.issueRequest()
visit.goToSamePageAnchor()
}

visitRequestStarted(visit) {
Expand Down
33 changes: 9 additions & 24 deletions src/core/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,12 @@ export class Session {
}
}

historyPoppedWithEmptyState(location) {
this.history.replace(location)
this.view.lastRenderedLocation = location
this.view.cacheSnapshot()
}

// Scroll observer delegate

scrollPositionChanged(position) {
Expand Down Expand Up @@ -260,7 +266,7 @@ export class Session {
// Navigator delegate

allowsVisitingLocationWithAction(location, action) {
return this.locationWithActionIsSamePage(location, action) || this.applicationAllowsVisitingLocation(location)
return this.applicationAllowsVisitingLocation(location)
}

visitProposedToLocation(location, options) {
Expand All @@ -276,9 +282,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) {
Expand All @@ -287,14 +291,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) {
Expand Down Expand Up @@ -334,9 +330,7 @@ export class Session {
// Page view delegate

viewWillCacheSnapshot() {
if (!this.navigator.currentVisit?.silent) {
this.notifyApplicationBeforeCachingSnapshot()
}
this.notifyApplicationBeforeCachingSnapshot()
}

allowsImmediateRender({ element }, options) {
Expand Down Expand Up @@ -428,15 +422,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 })
}
Expand Down
2 changes: 1 addition & 1 deletion src/tests/functional/navigation_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
Expand Down
1 change: 1 addition & 0 deletions src/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ export function findLinkFromClickTarget(target) {
const link = findClosestRecursively(target, "a[href], a[xlink\\:href]")

if (!link) return null
if (link.href.startsWith("#")) return null
Copy link
Copy Markdown
Contributor

@packagethief packagethief Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domchristie, I was looking into flaky tests and came across this. I don't think it's working as intended. The href is a DOM property that will always resolve to a full URL:

link = document.createElement("a")
link.href = "#foo"
link.href // => https://github.com/hotwired/turbo/pull/1285/changes#foo

Instead, we need to check the raw attribute value:

if (link.getAttribute("href")?.startsWith("#")) return null

I think this means that we've been intercepting hash-only link clicks and performing full visits. It probably went unnoticed because the result looks the same: we still scroll to the anchor and update the URL.

Fixing it would mean we'll really hand off hash-only links entirely to the browser. But there will be some consequences:

  • Turbo events will stop firing for hash-only clicks. This was the original intent, of course, but I don't think it's been happening, so it might break some expectations.
  • data-turbo-action will be ignored on hash-only links. So something like <a href="#section" data-turbo-action="replace"> would no longer use replaceState. There's a test that purports to assert this behavior, but I don't think it's working as intended either. It was introduced in Fix page loads when refreshing location with anchor #324 and expects a full visit to refresh the page. The test will still pass if we really start ignoring hash-only links, but the original intent would be broken.

Copy link
Copy Markdown
Contributor Author

@domchristie domchristie Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing it would mean we'll really hand off hash-only links entirely to the browser.

Yes, this was the original intention, but was not compatible with subsequent changes.

Turbo events will stop firing for hash-only clicks.

I don't think this should be an issue. Prior to this change, same-page anchor visits were "silent" i.e. they didn't dispatch events.

data-turbo-action will be ignored on hash-only links … The test will still pass if we really start ignoring hash-only links, but the original intent would be broken.

Hmm. Tricky! Weird that the test passes for ignored hash-only links, as it should wait for the nextBody, which I'd expect to never be replaced.

Perhaps a reasonable fix would be:

if (link.getAttribute("href")?.startsWith("#") && !link.dataset.turboAction) return null

(This is reminiscent of Turbo Frame visits that can update the address bar by adding a data-turbo-action attribute.)

I wouldn't be opposed to just checking the href, bearing in mind this fix was before Turbo Refreshes existed.

Copy link
Copy Markdown
Contributor

@packagethief packagethief Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Tricky! Weird that the test passes for ignored hash-only links, as it should wait for the nextBody, which I'd expect to never be replaced.

That's the thing. It seems like we lost the nextBody somewhere along the line. The test is now only asserting scroll and location behavior, which would remain unchanged.

turbo/src/tests/functional/navigation_tests.js

Lines 371 to 377 in b67039e

test("same-page anchored replace link assumes the intention was a refresh", async ({ page }) => {
await page.click("#refresh-link")
await expect(page).toHaveURL(withPathname("/src/tests/fixtures/navigation.html"))
await expect(page).toHaveURL(withHash("#main"))
expect(await isScrolledToSelector(page, "#main"), "scrolled to #main").toEqual(true)
})

Perhaps a reasonable fix would be...

Yes, we could be more specific, but I don't necessarily think we need to support that replacement strategy going forward. As you say, we have proper refreshes now.

I think we should make it work as advertised. We've evidently got a gap in our testing, too! That's why I wanted a gut-check on this diagnosis. It seems unlikely we could have made it this far with this not working?

if (link.hasAttribute("download")) return null

const linkTarget = link.getAttribute("target")
Expand Down