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.