10000 Fix Scrolling Issues by domchristie · Pull Request #420 · turbolinks/turbolinks · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Sep 25, 2021. It is now read-only.

Fix Scrolling Issues #420

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

domchristie
Copy link
Collaborator
@domchristie domchristie commented Sep 28, 2018

This is an attempt at fixing #181 and #241.

When a page is invalidated, either by tracked asset changes or via the turbolinks-visit-control directive, it is reloaded but the user is returned to the previous scroll position :/ This can be jarring when moving between long pages with different layouts—someone will click a link and be dumped some way down the page when it loads. This is discussed in #181, and I have outlined a couple of solutions in #181 (comment). This pull request aims to implement the "improved solution" in a more structured way.

First, we set the scrollRestoration flag to manual to indicate to the browser that we'll be handling scroll positions when navigating via the history. This seemingly impacts the scroll position on reload, too. It's great for this bug as it means that if a page is invalidated and reloaded, we do nothing. However, we also don't want to break the refresh button :/ So to fix this we store the scroll position in session storage, and restore it on reload.

Configuring scrollRestoration has the nice side-effect of fixing scrolling flicker when navigation Back/Forward #241. The downside is that for a complete solution we need to persist scroll values across full page loads (on beforeunload), through session storage, which I'm not 100% convinced about.

App which demonstrates the unexpected/buggy behaviour: https://secret-bayou-73715.herokuapp.com

App which demonstrates this fix: http://sheltered-reef-10818.herokuapp.com

Fixes #181
Fixes #241


scrollRestoration is not supported in IE/Edge, so I would welcome some testers in those browsers in particular. Also, I should add some automated tests for this, but I'm having some difficulty in setting up the recent test framework. (I think Intern starts the server then immediately stops with no error.) Thanks to @jason0x43 for his help on this. My old version of Java was not compatible with Intern.

@kinduff
Copy link
kinduff commented Oct 16, 2018

Love this. Please merge 👍

@panoply
Copy link
panoply commented Feb 10, 2019

Can someone merge this? Really need.

@domchristie
Copy link
Collaborator Author

@panoply feel free to checkout and build https://github.com/domchristie/turbolinks/tree/scroll_on_load and report any feedback. Thanks!

@domchristie
Copy link
Collaborator Author
domchristie commented Feb 15, 2019

for a complete solution we need to persist scroll values across full page loads (on beforeunload), through session storage, which I'm not 100% convinced about.

This might prove to be useful in more situations as we could determine how a page was loaded (initial page load, manual reload, page invalidated reload, or turbolinks load), and then annotate load events accordingly.

Related: #441 #115 #286

@panoply
Copy link
panoply commented Feb 17, 2019

@domchristie No problems my end Safari / Chrome / Firefox

@jasonhr13
Copy link

Ran into this issue today, would love this to get merged.

@domchristie
Copy link
Collaborator Author

According to the spec, the scrollRestoration mode is added to each session history entry. This pull request sets the mode to manual on start up, and is then used in all entries. I wonder if it might be cleaner to set this property according to the situation? (or if that would even work?)

@goshatch
Copy link

It would be really nice to get this merged!

@zokioki
Copy link
zokioki commented Sep 5, 2020

Running into this issue as well, a merge would be awesome 🙏

@andersonkrs
Copy link

Running into this issue here. 🤞 🙏

@megeek
Copy link
megeek commented Oct 9, 2020

also running into this...please merge! 👍

@danjenuil
Copy link

Running to this issue too! Please have it merged!

@rsl
Copy link
rsl commented Dec 7, 2020

this seems to still be alive, right?

@rsl
Copy link
rsl commented Jan 14, 2021

it doesn't look like this is going to be merged in any time soon. is there a way to use a fork of this to solve the problem? since this is just the javascript and is loaded by a rubygem, i honestly have no idea how to do this. looks like turbolinks itself has just been declared as superceded by turbo, in fact. would appreciate very much if anyone can help me get this fixed on a [rails 5] application using forks. crossing fingers and tia.

@herunan
Copy link
herunan commented Feb 27, 2021

Can you please finish checking. It would great to have it merged! Thank you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrolling makes content blink in Safari Unable to avoid Scroll Position Lock on page transition
0