Prevent reloading pages when tapping same-page anchors #298
Conversation
|
That test is flaky. I reran the suite and it passed. Reviewing whether we need something for the native adapters. |
|
I think we're probably OK re: native apps. The iOS + Android adapters just call the visit code directly: https://github.com/hotwired/turbo-ios/blob/main/Source/WebView/turbo.js and https://github.com/hotwired/turbo-android/blob/main/turbo/src/main/assets/js/turbo_bridge.js. |
|
Do we not need some kind of equivalent to: https://github.com/turbolinks/turbolinks-ios/pull/126/files#diff-db83eae543943038c85ab2f7a4c437adda02ed30411e803d0091f5a7fc32d0acR57 ? Feel free to ignore if I'm way out-of-date on this! |
|
Let's double check with @jayohms who runs Mobile at Basecamp. He's out for a bit, but we'll make sure we're reading this right. |
|
👍 to clarify my thoughts: I don't think that clicking on a same-page anchor in a native app should trigger a visit, Turbo should just perform the scrolling. Navigating back from a same-page visit might be interesting. For iOS, this is probably less of an issue: normal visits get pushed onto a stack, and the back button controls the navigation between these screens. I don't think there'd be any expectation that tapping < Back would handle the screen's scroll position. For Android, with its hardware back button, the expectations might be different(?) |
|
I just wanted to say that I tried this branch on my Rails app that was originally using the following code with Turbolinks to fix hash nav behavior. // Workround for odd hash navigation behavior in turbolinks that messes
// with the pushstate history
document.addEventListener('turbolinks:click', function (event) {
var anchorElement = event.target
var isSamePageAnchor = (
anchorElement.hash &&
anchorElement.origin === window.location.origin &&
anchorElement.pathname === window.location.pathname
)
if (isSamePageAnchor) {
Turbolinks.controller.pushHistoryWithLocationAndRestorationIdentifier(
event.data.url,
Turbolinks.uuid()
)
window.dispatchEvent(new HashChangeEvent("hashchange"))
event.preventDefault()
}
})I first ported the Rails app over to Turbo and then applied this PR. It worked great in the sense that no pages/requests were fired for hash links, but I had some stimulus code ( I forked this PR and just added a quick line of code to fire the event here kolide@fbed8a9#diff-78d8451f964182fd51330bac500ba6e71234f81aad9e8a57e682e723d0f517b3R269. I'm likely going to refactor my tabbed nav in my Rails app to not have to rely on this event being fired, but I felt it was worth dropping a comment here in case you all felt firing the |
@domchristie Agreed! This would be a nice change in the mobile apps.
I don't see this as problematic for iOS or Android. On Android, in most situations now, the system back button should work just like you'd expect the top toolbar back arrow to behave. Both mobile apps can behave the same way here: the same-page scroll is transparent to the apps and will not push onto the navigation stack.
I tested this out in the mobile demo app here: https://github.com/hotwired/turbo-native-demo/compare/same-page-anchor Some additional work will need to be done to prevent the mobile apps from following anchor visit navigation. I haven't spent enough time yet to see what a proper solution will look like, but it doesn't look like it'll be as straightforward as the proposed Turbolinks mobile adapter solutions, since it's wrapped up in the If you have any ideas in mind, let me know! And I'll take I'll take closer look at it this week, too. |
|
The mobile adapters do have access to the Here's a quick spike at the minimal set of changes (with clean up necessary) to support the mobile behavior I think we'd want: --- a/src/core/drive/visit.ts
+++ b/src/core/drive/visit.ts
@@ -328,8 +328,8 @@ export class Visit implements FetchRequestDelegate {
}
}
- scrollToAnchor() {
- const anchor = getAnchor(this.location)
+ scrollToAnchor(location: URL = this.location) {
+ const anchor = getAnchor(location)
if (anchor != null) {
this.view.scrollToAnchor(anchor)
return true
@@ -340,6 +340,11 @@ export class Visit implements FetchRequestDelegate {
this.view.scrollToPosition({ x: 0, y: 0 })
}
+ locationIsSamePageAnchor(location: URL) {
+ return getRequestURL(location) === getRequestURL(this.view.lastRenderedLocation) &&
+ getAnchor(location) != null
+ }
+We can then combine that with this change in the mobile adapter: hotwired/turbo-android#175 One thing to note here is that the mobile apps will not see the new @domchristie Thoughts? |
|
Thanks @jayohms. It's looking like it's doable then :D A possible slight complication here is that determining a same-page visit also requires the Having access to the Another approach might be to go via visitProposedToLocation(location, options) {
if (window.Turbo && Turbo.navigator.locationWithActionIsSamePage(location, options.action)) {
Turbo.navigator.view.scrollToAnchorFromLocation(location)
} else {
TurboSession.visitProposedToLocation(location.toString(), JSON.stringify(options))
}
}A bigger changeset (and the above breaks the law of demeter :/), but might work in terms of code organisation. diff --git a/src/core/drive/navigator.ts b/src/core/drive/navigator.ts
index 998fb51..7b5d71d 100644
--- a/src/core/drive/navigator.ts
+++ b/src/core/drive/navigator.ts
@@ -2,7 +2,7 @@ import { Action, isAction } from "../types"
import { FetchMethod } from "../../http/fetch_request"
import { FetchResponse } from "../../http/fetch_response"
import { FormSubmission } from "./form_submission"
-import { expandURL, Locatable } from "../url"
+import { expandURL, getAnchor, getRequestURL, Locatable } from "../url"
import { Visit, VisitDelegate, VisitOptions } from "./visit"
import { PageSnapshot } from "./page_snapshot"
@@ -119,6 +119,11 @@ export class Navigator {
this.delegate.visitCompleted(visit)
}
+ locationWithActionIsSamePage(location: URL, action: Action): boolean {
+ return getRequestURL(location) === getRequestURL(this.location) &&
+ (getAnchor(location) != null || action == "restore")
+ }
+
// Visits
get location() {
diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
index 5d8808e..2feee5b 100644
--- a/src/core/drive/visit.ts
+++ b/src/core/drive/visit.ts
@@ -2,7 +2,7 @@ import { Adapter } from "../native/adapter"
import { FetchMethod, FetchRequest, FetchRequestDelegate } from "../../http/fetch_request"
import { FetchResponse } from "../../http/fetch_response"
import { History } from "./history"
-import { getAnchor, getRequestURL } from "../url"
+import { getAnchor } from "../url"
import { PageSnapshot } from "./page_snapshot"
import { Action } from "../types"
import { uuid } from "../../util"
@@ -15,6 +15,7 @@ export interface VisitDelegate {
visitStarted(visit: Visit): void
visitCompleted(visit: Visit): void
+ locationWithActionIsSamePage(location: URL, action: Action): boolean
}
export enum TimingMetric {
@@ -90,11 +91,7 @@ export class Visit implements FetchRequestDelegate {
this.referrer = referrer
this.snapshotHTML = snapshotHTML
this.response = response
-
- this.isSamePage = (
- getRequestURL(location) === getRequestURL(this.view.lastRenderedLocation) &&
- (getAnchor(location) != null || this.action == "restore")
- )
+ this.isSamePage = this.delegate.locationWithActionIsSamePage(this.location, this.action)
}
get adapter() {
diff --git a/src/core/session.ts b/src/core/session.ts
index 8374962..7c5fb38 100644
--- a/src/core/session.ts
+++ b/src/core/session.ts
@@ -156,6 +156,10 @@ export class Session implements FormSubmitObserverDelegate, HistoryDelegate, Lin
this.notifyApplicationAfterPageLoad(visit.getTimingMetrics())
}
+ locationWithActionIsSamePage(location: URL, action: Action): boolean {
+ return this.navigator.locationWithActionIsSamePage(location, action)
+ }
+
// Form submit observer delegate
willSubmitForm(form: HTMLFormElement, submitter?: HTMLElement): boolean {
diff --git a/src/core/view.ts b/src/core/view.ts
index e8ee0ae..eb01e55 100644
--- a/src/core/view.ts
+++ b/src/core/view.ts
@@ -1,6 +1,7 @@
import { Renderer } from "./renderer"
import { Snapshot } from "./snapshot"
import { Position } from "./types"
+import { getAnchor } from "./url"
export interface ViewDelegate<S extends Snapshot> {
viewWillRenderSnapshot(snapshot: S, isPreview: boolean): void
@@ -21,7 +22,7 @@ export abstract class View<E extends Element, S extends Snapshot<E> = Snapshot<E
// Scrolling
- scrollToAnchor(anchor: string) {
+ scrollToAnchor(anchor: string | undefined) {
const element = this.snapshot.getElementForAnchor(anchor)
if (element) {
this.scrollToElement(element)
@@ -31,6 +32,10 @@ export abstract class View<E extends Element, S extends Snapshot<E> = Snapshot<E
}
}
+ scrollToAnchorFromLocation(location: URL) {
+ this.scrollToAnchor(getAnchor(location))
+ }
+
scrollToElement(element: Element) {
element.scrollIntoView()
} |
|
@domchristie I prefer the approach in your solution 👍. I realized that the Want to commit your solution and I can try it out and we see if there are any opportunities to improve the PR? |
domchristie
force-pushed
the
skip-link-2
branch
2 times, most recently
from
46184f1
to
40423ed
Compare
@jayohms 👍 Done |
|
@terracatta I've added the hash change event as a todo |
|
@domchristie The new implementation works great with hotwired/turbo-android#175 (and we can make the same change to the iOS adapter). Note that we won't be able to release new versions of the native adapters until this is merged and a new Turbo version is available, since the new navigator API is required. |
| @@ -119,6 +119,11 @@ export class Navigator { | |||
| this.delegate.visitCompleted(visit) | |||
| } | |||
|
|
|||
| locationWithActionIsSamePage(location: URL, action: Action): boolean { | |||
| return getRequestURL(location) === getRequestURL(this.view.lastRenderedLocation) && | |||
| (getAnchor(location) != null || action == "restore") | |||
There was a problem hiding this comment.
Can you elaborate on what situation the || action == "restore" condition is handling? I don't think this should ever affect the native apps, since restores are initiated by the apps themselves, but want to make sure.
There was a problem hiding this comment.
@jayohms sure, this refers to the changes in: bbb4d4b and 0800ec8. It prevents unnecessary requests when navigating back from a same-page anchor to a location with no anchor, when caching is disabled or has been cleared. e.g.
- Visit
example.comwhich has Turbo caching disabled - Click link to
example.com#main - Tap Back
Without this change, Turbo would re-request example.com because the anchor is undefined (and so would not be a same-page visit), and there's nothing in the cache. If the request URLs are equal, and the action is restore, we can be sure that the visit is a same-page one.
|
@domchristie I'm good with this from the native app perspective. Thanks for doing this! I'll make sure we ship native adapter updates once this is available. |
|
So we're good to merge now, @domchristie? |
@dhh just a few things to check off, including triggering the hashchange event, then I think we'll be good. |
|
@dhh @terracatta 7387bad triggers a hashchange event on same-page visits. A couple of point on this:
|
Closes hotwired#42 Navigate with a skip link within our Functional test suite, assert that the correct element is scrolled to, the page's Location path and hash are correct, and that the initial tab stop occurs after the skipped-to content.
getAnchor returns an empty string for blank hashes (#) and undefined when no hash is set at all
when navigating back from an anchored location to a non-anchored location on the same page
when restoring same-page visits with caching disabled. Requests shouldn't be issued when navigating back/forward on same-page visits; the scroll position should just be restored. However, when caching is disabled Turbo does not have a scroll position value to restore to. This change prioritizes checking for same-page visits to prevent unnecessary requests, and when restoring a scroll position, tries to scroll to the anchor if it exists. It's effectively the same outcome as before, without the request.
|
@dhh OK, I've completed some manual tests using a build from this branch, and this demo: https://stripe-root-spatula.glitch.me/ There's still an issue in Firefox when navigating back to the initial location, e.g.:
I have checked that Update: this is a bug with Firefox when using |
|
Excellent work, Dom 👏 |
|
hi @domchristie , I just tried upgrading to latest release and looks like its broken some existing behaviour I have across my app. 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() ? |
|
same to me. Would be good to know how to disable this. |
|
@acetinick @marispro this should be fixed by #324: any visit with |
Confirmed to be fixed in Firefox v92 🎉 |
This is a port of #57 and turbolinks/turbolinks#285 to resolve turbolinks/turbolinks#75, turbolinks/turbolinks#214, turbolinks/turbolinks#192, and #42
The Problem
Clicking on a link which references an anchor on the current page issues an unnecessary request. This can cause problems when linking to content which is lazily loaded (see https://agile-reef-88023.herokuapp.com/posts/1), as well as with analytics and skip links. Disabling Turbo completely on those links prevents the request, but because the view is not cached, this approach breaks the Back button.
The Solution
This PR detects whether the visit is to an anchor on the same page, and if so, prevents the request. It takes a snapshot and performs the scroll, bypassing any rendering and
turbo:loadtriggering. A similar flow handles restorations to same-page anchors. Correct focusing is achieved by temporarily addingtabindex=-1and callingfocuson the target.Notes
chrome on linux - FrameTests - evaluates frame script elements on each render (0.297s) AssertionError: expected 1 to equal 2), which doesn't seem to be related to these changes, and it's passing locally on my machineTodo
Fixes #42
Closes #152