1155730 - Implement API to allow disabling of scroll restoration on history navigation

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Majid Valipour]

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

Comment 1

[Jan Paul Posma]

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: scrollrestoration_4.diff

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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: scrollrestoration_5.diff

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.

Comment 4

[Johnny Stenback (: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

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: scrollrestoration_6.diff

fixed those whitespace nits

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: scrollrestoration_7.diff

and missing uuid change

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84163eb4992b

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84163eb4992b

Comment 10

[Binyamin]

Missing it in https://developer.mozilla.org/en-US/Firefox/Releases/46 documentation.
Related status https://github.com/Fyrd/caniuse/issues/1889

Comment 11

[Binyamin]

Firefox Nightly 46 fails testcase "Navigation within document" http://majido.github.io/scroll-restoration-proposal/tests/scroll-restoration-push-replace.html

Comment 12

[Binyamin]

Need to reopen the issue till passed all testcases http://majido.github.io/scroll-restoration-proposal/tests/

Comment 13

[Majid Valipour]

I believe it is the test that is broken. It should use |window.scrollX| instead of |document.body.scrollTop|. I will fix the test.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 15

[Binyamin]

(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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(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. )

Comment 17

[Majid Valipour]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Thanks, let me file a bug then.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 20

[Majid Valipour]

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'.