-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add scroll restoration preference to history traversal #278
Conversation
- Define history.scrollRestoration and update IDLs - Add a new persisted user state restoration algorithm
@domenic: It is basically the draft spec but with the right terminology and proper linking. FYI there is also a test suite for the API that I am going to try to pull into web-platform-tests once this is in spec proper. /cc @RByers |
Great, thanks for submitting this!! I'm actually on vacation; could one of the other editors volunteer to review and merge this in the next few days? But if nobody does, I'll do it :). I know with TPAC happening it's a busy time for everyone. |
<p>An entry's <dfn>scroll restoration preference</dfn> indicates whether the user agent should | ||
restore the persisted scroll position (if any) when traversing to it.</p> | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A newline too many here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in ed411e9.
I've poked around a bit but am not finished. Now the weekend is here, so if anyone wants to take care of this, I will not be offended. |
- Separare definition of scrollRestoration from scroll restoration mode - Use scroll restoration mode everywhere - Remove unnecessary newline and data-x - Multiple small correction and cleanup
@RByers is right that I am OOO for few days. But I took a small break to update this pull request. I think I have addressed most of @foolip feedbacks. As for an better example here is simple one that shows how |
<div class="example"> | ||
<p>Most applications want to use the same scroll restoration mode value for all of their history | ||
entries. To achieve this they should set the <code data-x="dom-history-scroll-restoration">scrollRestoration</code> | ||
attribute as soon as possible (e.g., in first script tag in the document's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/first script tag/the first <code>script</code>
element/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9a825ec
- Make it clear that ```history.scrollRestoration``` reflects the "current entry of the session history" - Make sure the default value is defined for the concept not the attribute - Fix minor link issues in example description
@@ -80353,6 +80353,9 @@ x === this; // true</pre> | |||
This prevents values from being displayed incorrectly after a history traversal when the user had | |||
originally entered the values with an explicit, non-default directionality.</p> | |||
|
|||
<p>An entry's <dfn>scroll restoration mode</dfn> indicates whether the user agent should | |||
restore the persisted scroll position (if any) when traversing to it.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also say above that a "session history entry" consists of this. So explicitly cross-reference this definition from there.
- Reference scroll restoration mode in definition of history entry - Use 'must' and 'Optionally' to reflect appropriate spec language - Add necessary Otherwise in algorithm - Fix wrapping and spacing issues - Fix incorrect usage of auto/manual
Thanks @zcorpan for review :). I have updated a new patch that should address your feedback. Let me know if there is anything else missing. |
|
||
<li><p>If <var>scrollRestoration</var> is "<code | ||
data-x="dom-ScrollRestoration-manual">manual</code>" the user agent should not restore the scroll | ||
position for the document otherwise it may.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an incomplete sentence. Can you make it ", otherwise, it may do so."
Other than that nit, LGTM |
Fixed the nit and squashed; committed as dd6a34e. Thank you! |
I had missed to |
- Define history.scrollRestoration and update IDLs - Add a new persisted user state restoration algorithm Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28553. Closes #278.
My first whatwg/html contribution \o/. Thanks @zcorpan @foolip @annevk for your review feedback and help! |
Thank you @majido! (You might want to associate majidvp@chromium.org with your GitHub account so you get GitHub credits for this work.) |
The API is shipped in latest stable Chrome (since M46). The API design
was discussed on WhatWG mailing list. The patch does the following:
Once merge I will close the original W3C bug.