Skip to content

Add sessionStorage based scroll memory for Dartdoc's scrolling divs #1917

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

Merged
merged 25 commits into from
Feb 4, 2019

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Jan 31, 2019

Fixes #1915, #1288, #1372, #779, #1870 and probably a host of other complaints in other repositories about Dartdoc's suboptimal scrolling behavior.

Use sessionStorage connected to some JavaScript that saves and restores positions for all three of Dartdoc's scrolling divs. We start with all three divs visibility: hidden and only display them after any restorative scrolling has occurred, so the page loading is seamless.

My Javascript is extremely primitive (since I don't actually know JavaScript) but seems to work for local browsing, including @Hixie's case of multiple tabs with the same page but different scroll positions.

@jcollins-g jcollins-g requested a review from devoncarew January 31, 2019 23:39
@googlebot googlebot added the cla: yes Google CLA check succeeded. label Jan 31, 2019
@coveralls
Copy link

coveralls commented Feb 1, 2019

Coverage Status

Coverage remained the same at 93.966% when pulling 1659ded on style-refresh+javascript-scroll-memory into 2aa6060 on master.

@Hixie
Copy link
Contributor

Hixie commented Feb 1, 2019

This is great!

One suggestion: We should use sessionStorage rather that localStorage. That way, if you open two tabs on the same page, then navigate both, then return from both, it'll work correctly.

@jcollins-g
Copy link
Contributor Author

It was unclear to me that the definition of a session included being able to navigate away from a page (though it makes sense now that I think about it). I will try using sessionStorage. I can discard lscache if that works, as there won't be a need to avoid collisions between different URIs then.

@jcollins-g jcollins-g changed the title Add localStorage based scroll memory for Dartdoc's scrolling divs Add sessionStorage based scroll memory for Dartdoc's scrolling divs Feb 1, 2019
@jcollins-g
Copy link
Contributor Author

@Hixie Indeed that both works better and simplifies the implementation.

PTAL.

@jcollins-g jcollins-g requested a review from pq February 1, 2019 20:50
Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small concern, but I don't have any better alternatives, and it's likely not a real issue.

@@ -79,6 +79,7 @@ main {
overflow-y: scroll;
padding: 20px 0 15px 30px;
margin: 5px 20px 0 0;
visibility: hidden; /* shown by Javascript after scroll position restore */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks correct, but I can't help but wonder if there are places where this could fall off the rails. If you javascript script didn't run, or threw an exceptions for some reason before it could restore visibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a risk. If the javascript doesn't run due to an exception, the screen becomes blank (indeed, I experienced this when writing the code and accidentally introducing bugs).

The only thing I can think of to reduce that risk is to wrap all our JavaScript in the try of a try/finally with the visibility switch-on occurring in the finally block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've wrapped the restore part that can be wrapped in a try-finally block, and moved it ahead of all the other javascript initialization. This means the page visibly appears slightly faster and should be more resilient to exceptions.

@jcollins-g jcollins-g merged commit 733992b into master Feb 4, 2019
@jcollins-g jcollins-g deleted the style-refresh+javascript-scroll-memory branch February 4, 2019 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants