Fix page loads when refreshing location with anchor

[domchristie]

Currently, if the location is http://example.com#main, and we call Turbo.visit(location.toString()), the page will just scroll to the anchor without reloading the page. This makes is impossible to refresh a current page via a Turbo visit.

Rather than checking for the presence of a URL hash, this pull request just checks that the hash has changed. If so, it's a same page visit.

/cc @jayohms

[jayohms]

@domchristie Thanks, this looks like an improvement 👍. I'm wondering if this covers all situations here:

We were using Turbo.visit() to force redirect of same page to anchor after modal window will close to refresh just added record, and now with this PR doesn't trigger the visit.
Is there a way to override this behaviour or introduce a new action to Turbo.visit() ?

Say you're on page http://example.com/feature, display a modal, add a new record, close the modal and call Turbo.visit("http://example.com/feature#new_record_id_1234"). The new visit will not be performed to refresh the page and the anchor will not exist on the page yet. Should we support this situation? I'm wondering if we should allow replace visits to bypass the same-page anchor scrolling entirely. Thoughts?

[jayohms]

Another idea is to validate that the #anchor element exists on the page before considering it a same-page visit.

[domchristie]

I'm wondering if this covers all situations here:

@jayohms Unfortunately not. This PR was inspired by that feedback, but will only perform a full visit if the locations are identical—with a nice side-effect of simplifying the code (I'm not sure why I didn't think of this approach before!)

I'm wondering if we should allow replace visits to bypass the same-page anchor scrolling entirely. Thoughts?
Another idea is to validate that the #anchor element exists on the page before considering it a same-page visit.

Interesting! I've been mulling over a fix for this, and to summarise the options:

1. isSamePage should be false for replace actions
2. validate presence of anchor on page, and if it does not exist isSamePage is false
3. add an visit option to override the isSamePage check: Turbo.visit(location, { samePage: false })
4. allow same-page visits to be intercepted: addEventListener('turbo:before-same-page', e => e.preventDefault())
5. don't support this behaviour and instead require a full-page workaround: window.location = newLocation

I think I prefer your options as they elegantly don't require additional API. Just wondering if they cover all situations? For example, let's say that rather than Turbo.visit("http://example.com/feature#new_record_id_1234"), we had Turbo.visit("http://example.com/feature#last_record"). The #last_record element will be on the page but won't be updated if the page isn't refreshed.

Re. option 1: I'm struggling to think of a reason why someone would intentionally perform a same-page replace visit and not want the page to reload—I don't think this is even possible with the default browser behaviour?

I could be wrongly second-guessing people's intentions for same-page visits, but the more I think about it, the more I think option 1 (plus perhaps 2), will cover most situations.

[jayohms]

I think I prefer your options as they elegantly don't require additional API. Just wondering if they cover all situations? For example, let's say that rather than Turbo.visit("http://example.com/feature#new_record_id_1234"), we had Turbo.visit("http://example.com/feature#last_record"). The #last_record element will be on the page but won't be updated if the page isn't refreshed.

Yeah, I'd like to avoid new APIs if possible, too. In this situation, I think it's ok to say a replace visit is necessary if you're visiting #last_record, it already exists on the page, but you want new contents.

re. option 1: I'm struggling to think of a reason why someone would intentionally perform a same-page replace visit and not want the page to reload—I don't think this is even possible with the default browser behaviour?

I can't think of any negative consequences here, either.

I could be wrongly second-guessing people's intentions for same-page visits, but the more I think about it, the more I think option 1 (plus perhaps 2), will cover most situations.

Yeah, I think so too. I think we should try solutions 1 and 2 together, because then by default it'll handle the situation where you try to visit a new anchor that doesn't currently exist, i.e. Turbo.visit("http://example.com/feature#new_record_id_1234"). It preserves the old behavior for edge cases, while allowing same-page scrolling to occur for natural situations.

[domchristie]

I think we should try solutions 1 and 2 together
👍

Solution 1 is currently complete in this PR. Part 2 might require a bit more thought. I haven't fully got my head round it, but the logic might get complex if we consider blank/undefined anchors

[jayohms]

Thanks @domchristie! I haven't really considered the implementation for solution 2. If it looks like it's going to be complex, consider splitting into a separate PR that we can evaluate separately.

[domchristie]

(I'm not sure why I didn't think of this approach before!)

Now I do! … If we only check the hash changes, then navigating from /#main to / will be considered a same-page location and will just scroll to the top. I think the typical browser/expected behaviour is to reload the page(?). However navigating Back from /#main to / should just scroll to the top.

So this results in a pretty gnarly check:

  locationWithActionIsSamePage(location: URL, action: Action): boolean {
    const anchor = getAnchor(location)
    const currentAnchor = getAnchor(this.view.lastRenderedLocation)
    const restoreToTop = action === 'restore' && typeof anchor === 'undefined'
    const anchorElementExists = anchor === '' || this.view.snapshot.hasAnchor(anchor)

    return action !== "replace" &&
      getRequestURL(location) === getRequestURL(this.view.lastRenderedLocation) &&
      (
        restoreToTop ||
        (anchor != null && anchor !== currentAnchor && anchorElementExists)
      )
  }

:/

[domchristie]

Still in two minds about validating whether an element exists. It's cool, but it feels a bit odd given that a browser will just scroll to the top.

[domchristie]

@jayohms 4eb3720 updates this PR to fix the navigate back issues. I've not included the anchor element check, but can do so if needed.

[jayohms]

Still in two minds about validating whether an element exists. It's cool, but it feels a bit odd given that a browser will just scroll to the top.

Yeah, totally fine leaving out for now. If we hear feedback on it, we can reevaluate 👍.

Thanks for doing this, looks great!

[domchristie]

Yeah, totally fine leaving out for now. If we hear feedback on it, we can reevaluate 👍.

Great, rebasing now…

[domchristie]

@jayohms Done. (I think that failing test is a flaky one)