Add control to preserve scroll during Drive visits #1040
Conversation
seanpdoyle
force-pushed
the
scroll-preserve
branch
from
3670338 to
a2b0932
Compare
|
@jorgemanrubia in #1019 (comment), you mentioned skepticism related to making scroll preservation configurable:
This PR has the potential to provide a solution for #37 that draws inspiration from other |
| get visitScroll() { | ||
| return this.getSetting("visit-scroll") | ||
| } |
There was a problem hiding this comment.
The current <meta name="turbo-scroll"> is meant to parallel the concept of a "refresh".
Originally, this PR proposed <meta name="turbo-scroll">. An alternative couldbe <meta name="turbo-render-scroll">. Are either of those preferable?
src/core/drive/page_view.js
Outdated
| if (this.isPageRefresh(visit)) { | ||
| return (visit?.refresh?.scroll || this.snapshot.refreshScroll) === "preserve" | ||
| } else { | ||
| return this.snapshot.visitScroll === "preserve" |
There was a problem hiding this comment.
This introduces a subtle bug when using the back button.
Without this change, the back button restores scroll position; with this change it resets scroll to the top.
It's tied to performScroll:
performScroll() {
if (!this.scrolled && !this.view.forceReloaded && !this.view.shouldPreserveScrollPosition(this)) {
if (this.action == "restore") {
this.scrollToRestoredPosition() || this.scrollToAnchor() || this.view.scrollToTop()
} else {
this.scrollToAnchor() || this.view.scrollToTop()
}
this.scrolled = true
}
}Before:
- On Page A
- Go to Page B
- Hit Back button
- Trigger
performScroll:
this.scrolledfalse
this.view.forceReloadedfalse
this.view.shouldPreserveScrollPosition(this)false - All false -> Triggers
this.action === "restore" - Scroll position restored
After:
- On Page A: Has
visitScroll === "preserve" - Go to Page B
- Hit Back button
- Trigger
performScroll:
this.scrolledfalse
this.view.forceReloadedfalse
this.view.shouldPreserveScrollPosition(this)true⚠️ becausethis.snapshot.visitScroll === "preserve" - Not all false -> Doesn't trigger
this.action === "restore" - Scroll position not restored
Possible solution
Not sure if it's the cleanest, but I've added a guard to the top of shouldPreserveScrollPosition:
shouldPreserveScrollPosition(visit) {
if (visit?.action === "restore") return false
if (this.isPageRefresh(visit)) {
return (visit?.refresh?.scroll || this.snapshot.refreshScroll) === "preserve"
} else {
return this.snapshot.visitScroll === "preserve"
}
}There was a problem hiding this comment.
@tvararu thank you for testing this and providing feedback. I've pushed up a change to incorporate the Visit action in the conditional. Could you try it again?
There was a problem hiding this comment.
Tested it locally, works for me!
- Fresh page navigation: Scroll is at top
- Scroll down a bit, refresh: Scroll stays where it is
- Navigate somewhere else: Scroll is at top
- Use back button: Scroll restores to expected location
seanpdoyle
force-pushed
the
scroll-preserve
branch
from
a2b0932 to
20e9941
Compare
In an effort to overcome [test flakiness][], pass `--retries=5` to the Playwright CLI calls in the project's GitHub actions `ci.yml` configuration file. [retires]: https://playwright.dev/docs/test-retries#retries [test flakiness]: https://github.com/hotwired/turbo/actions/runs/19574784533/job/56057106175?pr=1040#step:10:70
seanpdoyle
force-pushed
the
scroll-preserve
branch
from
20e9941 to
047e56d
Compare
Related to [hotwired#37][] Add support for preserving scroll changes from page to page. Controlled by the presence of the `<meta name="turbo-visit-scroll">` element in the `<head>`. To preserve: ```html <meta name="turbo-visit-scroll" content="preserve"> ``` To reset: ```html <meta name="turbo-visit-scroll" content="reset"> ``` [hotwired#37]: hotwired#37
seanpdoyle
force-pushed
the
scroll-preserve
branch
from
047e56d to
8d43b02
Compare
3 participants
Related to #37
Add support for preserving scroll changes from page to page. Controlled
by the presence of the
<meta name="turbo-visit-scroll">element in the<head>.To preserve:
To reset: