Closed Bug 1155730 Opened 5 years ago Closed 4 years ago

Implement API to allow disabling of scroll restoration on history navigation

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: majidvp, Assigned: smaug)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2368.0 Safari/537.36

Steps to reproduce:

I am working on a small extension to existing History API that will enable web applications to explicitly disable default scroll restoration for any history entry they are creating. 

The default scroll restoration behaviour has been a pain point for single-page web applications. See bug 679458, bug 666792 for examples of the issues web authors are facing. I have a more detailed analysis of the issue various hacks being used to work around this issue and their negative impacts on UX, application stability here: https://docs.google.com/document/d/1Tiu8PjvBtNOAgeh6yrs7bOrXxQcavQLiNtRJ_ToLlVM

I am hoping to start implementing this for Chromium soon and want to propose that Firefox also considers implementing it to address this issue and resolve above mentioned bugs. I like to work with Firefox team on a solution that works well for all vendors.

The proposal is being discussed at WHATWG the moment. Here are the relevant links:
 - Blink intent-to-implement: https://groups.google.com/a/chromium.org/d/topic/blink-dev/U1e2lmGs4tM/discussion
 - WHATWG mailing list: https://lists.w3.org/Archives/Public/public-whatwg-archive/2015Apr/0075.html
 - Unofficial specification document:  https://docs.google.com/document/d/1Tiu8PjvBtNOAgeh6yrs7bOrXxQcavQLiNtRJ_ToLlVM
Component: Untriaged → Document Navigation
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
Any thoughts from the Firefox team on this yet? 

It seems that Firefox and IE are currently the only major browsers for which it is hard to implement custom scroll restoration, since Webkit and Blink (since [1]) restore the scroll position after firing popstate, which makes it possible to record the previous scroll position. This is against the spec, and has been previously closed as wontfix in bug 666792, but might be worth considering until this bug 1155730 is resolved, as Blink did.

In any case, allowing to disable scroll restoration would solve all this anyway, so I'd love to see an intent-to-implement from Firefox.

[1] http://src.chromium.org/viewvc/blink?view=revision&revision=194167
Attached patch scrollrestoration_4.diff (obsolete) — Splinter Review
https://html.spec.whatwg.org/multipage/browsers.html#dom-history-scroll-restoration

bz is on vacation, and we're a bit short of docshell peers.

LayoutHistoryState handling is not too nice, the explicit reset of its scrolling state, but since it is used by layout after page load to restore scrolling state and such if frame is restored or so, 
explicit reset is the safest option I could find here now. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c054d459a306
Hopefully that one assertion doesn't fire.
Assignee: nobody → bugs
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8698760 - Flags: review?(jst)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4c36b18cf56

There was some odd unicode character in SessionHistory.jsm, no other change in this patch comparing to the previous one.
Attachment #8698760 - Attachment is obsolete: true
Attachment #8698760 - Flags: review?(jst)
Attachment #8698937 - Flags: review?(jst)
Comment on attachment 8698937 [details] [diff] [review]
scrollrestoration_5.diff

Surprising how complex this stuff is, but I don't see an easier way to do this easily. r=jst
Attachment #8698937 - Flags: review?(jst) → review+
Comment on attachment 8698937 [details] [diff] [review]
scrollrestoration_5.diff

Review of attachment 8698937 [details] [diff] [review]:
-----------------------------------------------------------------

Two small cosmetic nits.

::: docshell/base/nsIDocShell.idl
@@ +1094,5 @@
> +  /**
> +   * Sets/gets the current scroll restoration mode.
> +   * @see https://html.spec.whatwg.org/#dom-history-scroll-restoration
> +  */
> +  

This blank line probably should be removed.

::: docshell/test/navigation/file_scrollRestoration.html
@@ +114,5 @@
> +      }
> +
> +      window.addEventListener("pageshow",
> +          function(e) {
> +            setTimeout(test, 0, e); 

Here is a tailing space should be removed.
Attached patch scrollrestoration_6.diff (obsolete) — Splinter Review
fixed those whitespace nits
and missing uuid change
Attachment #8702048 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/84163eb4992b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1235492
Firefox Nightly 46 fails testcase "Navigation within document" http://majido.github.io/scroll-restoration-proposal/tests/scroll-restoration-push-replace.html
Need to reopen the issue till passed all testcases http://majido.github.io/scroll-restoration-proposal/tests/
Flags: needinfo?(cbook)
I believe it is the test that is broken. It should use |window.scrollX| instead of |document.body.scrollTop|. I will fix the test.
We don't open closed bugs because some external test (especially possibly invalid test :) ) is failing.
If you see some issue, please file a new bug.

(While implementing this I reported some bugs in blink implementation and reported them to Majid.)
(In reply to Majid Valipour from comment #13)
> I believe it is the test that is broken. It should use |window.scrollX|
> instead of |document.body.scrollTop|. I will fix the test.

Chrome passes all those tests.
(which might hint that Chrome has a bug if it passes invalid test ;) But I haven't looked at the test yet. Maybe there is a bug in FF, or in Chrome or in the test or in all those 3. I wouldn't be surprised about the last one. )
Nah, I just fixed the test issue (which was broken in Chrome too) but there is still a failure in FF nightly 46. Sorry for a confusion due to the change.

FF implementation seems to behave differently than the spec in the case where navigating from a history entry with manual scroll restoration mode to an entry with auto scroll restoration mode within the same document. In this case, scroll position should be restored as per spec but it is not. The link in #11 should reproduce it.

BTW, I am upstreaming a cleaned up version of these tests to web-platform-tests [1] to cover the spec changes. Comment/Review/Feedback on tests is welcome :).

[1] https://github.com/w3c/web-platform-tests/pull/2437
Thanks, let me file a bug then.
Depends on: 1237075
Well, 'auto' doesn't mean scrolling. Scrolling is always up to the browser. 'manual' just prevents that possible scrolling per spec. As the spec says "it may do so." So the test is still buggy.
Per our offline discussion, you are correct and we should not test the scrolling behaviour for 'auto' rather we should only test *not* scrolling behaviour for 'manual'. Of course, each UA will probably have its own tests covering its particular scroll behvaiour for 'auto'.
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.