Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: always save the initial scroll position even if no scrolling occurs

Merged
merged 3 commits into from Oct 26, 2019

Conversation

hedgepigdaniel
Copy link
Contributor

@hedgepigdaniel hedgepigdaniel commented Oct 4, 2019

Browsers (at least the latest chrome) don't dispatch the scroll event unless the scroll position is set to a different value. This gives rise to a bug where if the scroll position is not [0, 0] and shouldUpdateScroll requests to set the scroll position to what it already is, then the scroll event will never happen and the scroll position will not be saved for the new location.

The fix is to save the scroll position immediately after setting it (after a transition) and when adding a scrollable element. At this time the position may not have updated yet, but if it does update then the saved position will be overridden..

@taion
Copy link
Owner

taion commented Oct 11, 2019

Sorry for taking so long to get to this. Is there a reason we need to save the new position, though? If e.g. we don't have a saved position after a transition, then the "saved position is missing" handler will still calculate the same [0, 0], no?

How does this come up?

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Oct 12, 2019

The feature I am trying to implement in an app is the following

  • The app has a header which is the same in a certain group of routes, only the content below the header changes
  • I want for the scroll position to be handled/restored nicely
  • when the scroll position is changed, I don't want the window to be scrolled up so far that it moves the header. e.g. when the page is reset to the top, if the header was already scrolled out of view, I want the page to be scroll up only up to the bottom of the header. Or, if the header is half in view, I want to remain that way (since the top of the page content area is already visible)

The logic for this is in the app, but the effect of it is that shouldUpdateScroll often returns a position which is not the default [0, 0], but is also the same as the current (previous route's) scroll position. This happens if the page is scrolled halfway down the header, and the user changes routes. The scroll position is left as is (shouldUpdateScroll returns the existing position) because none of the page content was scrolled out of view (but due to this bug, it is not saved). If the user then pushes another route, and then uses the back button, the scroll position is restored to the saved position which is absent and defaults to [0, 0], when it should instead restore to the position selected by shouldUpdateScroll which half of the header out of view.

This is the change to Rudy where I'm adding scroll restoration support: respond-framework/rudy#65

The relevant part is that readScrollPosition is exposed to the user, so in this.options.shouldUpdateScroll (provided by the user), they can refer to previous saved scroll positions in the stack in order to implement a behaviour such as this one, which depends on previous scroll positions.

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Oct 14, 2019

I've updated this - the code to save the initial position now waits until the window scroll position is updated in the case that this is asynchronous

@hedgepigdaniel hedgepigdaniel force-pushed the fix/initial-position branch 2 times, most recently from 50e6cb2 to 90683a5 Compare Oct 16, 2019
@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Oct 16, 2019

OK, I've added a failing test which this change fixes - hopefully that clarifies the situation.

taion
taion approved these changes Oct 26, 2019
@taion
Copy link
Owner

taion commented Oct 26, 2019

thanks!

@taion taion merged commit 407050f into taion:master Oct 26, 2019
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants