Simplify same page anchor visits

[domchristie]

This pull request vastly simplifies the code around the handling of same-page links. No complex isSamePage logic and no silent Visits, thereby reducing the number of conditions and branches in the code.

This is achieved by letting the browser handle same-page anchor links. So now, links whose hrefs start with "#" are treated in a similar way as [target^=_] and [download] links.

By default, the browser will push same-page link clicks to the history stack with null state data, but we still need to update the state so it is correctly handled when traversing the history. We do this in the popstate event.

popstate is dispatched when navigating back and forth but also when a same-page anchor link is clicked. turbo state data is added during Turbo navigations, so if the event.state is null on popstate, we can be reasonably sure that it originated from a same-page anchor click. When this happens, we reconcile the empty event state by: incrementing the History's currentIndex, replacing the null state with a turbo state, setting the lastRenderedLocation, and caching the view.

This pull request also tidies up an outdated workaround for Safari's popstate behavior. Safari used to dispatch popstate on page load, but behaviour was removed in v10 (~2016), so we can safely remove this workaround.

---

Tests pass, and it seems to work when testing manually. However, I think it would be good to test this more thoroughly.

[packagethief]

Nice simplification. The complexity around same-page anchor navigation has always bothered me.

This is achieved by letting the browser handle same-page anchor links

I remember trying a similar approach a long time ago, and the problem then was that by letting the browser handle the navigation, the adapter didn't have a chance to intervene. Currently it can cancel same-page anchor navigations and it's notified of things like visitRendered. The non-visit navigation stepped on some assumptions.

Of course, that's the whole point of the change: these aren't visits and things are simpler if we don't treat them as such. I'm just not sure what the implications of such an API change would be for native adapters.

[domchristie]

@packagethief Thanks!

The complexity around same-page anchor navigation has always bothered me.

Same! And I feel responsible 😳

I remember trying a similar approach a long time ago, and the problem then was that by letting the browser handle the navigation, the adapter didn't have a chance to intervene. Currently it can cancel same-page anchor navigations and it's notified of things like visitRendered. The non-visit navigation stepped on some assumptions.

Of course, that's the whole point of the change: these aren't visits and things are simpler if we don't treat them as such. I'm just not sure what the implications of such an API change would be for native adapters.

Yes, I'm in two minds. One the one hand, jumping to a fragment on the same-page doesn't feel like a native interaction. The Turbo Native adapters seem predominantly concerned with loading content, and adding it to the broader page-based navigation stack. Because same-page links don't fit with this flow, it feels appropriate to just use the browser's behaviour, and bypass any Visit handling.

On the other hand, same-page links do change the location, so in this way it could be considered a Visit, and like you say, give the adapter a chance to intervene.

The native adapter integration was discussed in #298 (comment), so I wonder if @jayohms has any thoughts on this?

[brunoprietog]

This is also an accessibility improvement! Currently, if you want the anchor to work correctly with screen readers, you must disable Turbo on those links.

[domchristie]

@brunoprietog can you explain a bit more about this? (I recall we spent a bit of time ensuring that the focus state was correct after a same-page visit)

[brunoprietog]

According to my tests, the focus moves to the anchor only the first time, and has a small delay. This is what I can perceive with the NVDA screen reader. I have never checked this in more detail because I always ended up disabling Turbo in those cases. In fact, in Basecamp and HEY, the links to skip to the main content ignore Turbo completely.

[domchristie]

@brunoprietog yes, I think that might be an issue with the current Visit based approach. It's limited by checking the current location against the Visit location, rather than the precise scroll location of the user on the page, and whether the Visit anchor will change the scroll position.

In terms of the the native adapters intercepting these kinds of link-clicks could be handled on the client and sent to the native adapter manually:

addEventListener('click', function (event) {
  if (event.target.matches('a[href^="#"]') {
    event.preventDefault()
    Turbo.visit(event.href) // sent to native adapter
  }
})

[brunoprietog]

Well, I'm still not quite clear on why we would want to pass this event to native adapters either, I haven't gotten deep enough in there though. Philosophically I still think this should simply be handled by the browser, but I'm probably ignoring something on the Turbo native side. If it's still something necessary, we might as well add that little hint to intercept those clicks to the documentation. We should certainly test this thoroughly and hopefully merge it.

[jeffse]

This fixes an issue I've been trying to track down where the document position isn't restored properly after visiting an anchor in the same document then hitting the back button.

[seanpdoyle]

@domchristie I've rebased this branch to catch up with the latest main. That involved replacing the CSS selector for :not([href^="#"]) with a call to String.startsWith, since that method has changed to interact directly with the HTMLAnchorElement instance.

[fschwahn]

@domchristie I've rebased this branch to catch up with the latest main. That involved replacing the CSS selector for :not([href^="#"]) with a call to String.startsWith, since that method has changed to interact directly with the HTMLAnchorElement instance.

@seanpdoyle this broke clicking links inside an SVG for us. In that case link.href returns SVGAnimatedString { baseVal: "#foo", animVal: "#foo" }, so startsWith is not defined and throws an error.

I tried opting out by putting data-turbo="false" on a wrapper, but that did not work (I guess that's checked after this code has run).

[bep]

Pushing this massive change in a patch release is incredibly irresponsible, I suspect more people found their site(s) broken as a side effect of this.

[seanpdoyle]

@fschwahn thank you for raising this issue. There is explicit coverage in the test suite aimed at exercising an <a> element nested within an <svg> element:

https://github.com/hotwired/turbo/blob/v8.0.21/src/tests/functional/navigation_tests.js#L295-L302

Could you share more information about the circumstances of your issue? That way, we can expand upon the existing test coverage to guard against future regressions.

[seanpdoyle]

@fschwahn I've opened #1495. Could you review that code change within the context of the issues you're encountering?

[packagethief]

@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.

[domchristie]

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.

[packagethief]

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?