Currently there is no reliable and cross-browser solution for developers to opt out the automatic scroll restoration behavior or control any of its aspects. The automatic restoration works well for document style web sites but it is often not appropriate for single-page web applications where they are re-constructing the page content onpopstate every time and often asynchronously or need to control the visual transition between states. Here is more the problem background and existing workarounds: http://majido.github.io/scroll-restoration-proposal/ We in blink team have proposed a solution to this issue on WHATWG and there is some consensus (with spec editors and Mozilla experts) around a simple API based on a new attribute added to the history object. I would like to know if Webkit has any feedback on this solution and API and help make the consensus more inclusive if possible. Unofficial spec: http://majido.github.io/scroll-restoration-proposal/history-based-api.html Web platform tests: http://majido.github.io/scroll-restoration-proposal/tests/ WhatWG discussion thread: https://lists.w3.org/Archives/Public/public-whatwg-archive/2015Mar/0070.html WICG discourse: http://discourse.wicg.io/t/disabling-automatic-scroll-restoration-on-navigation/1012/2 Also you can find a summary of alternative APIs and solutions that were considered before reaching the current design here: http://discourse.wicg.io/t/disabling-automatic-scroll-restoration-on-navigation/1012/4?u=majidvp
Here is an example of an issue that benefits from this API: https://bugs.webkit.org/show_bug.cgi?id=51899 Here is a few other documented cases: * https://bugzilla.mozilla.org/show_bug.cgi?id=679458 * https://github.com/rackt/react-router/issues/707 * http://andrz.me/blog/scrollx-scroll-why-history * https://aerotwist.com/blog/some-gotchas-that-got-me/#body-scrolling-is-impossible-to-stop
We also have this problem for some of the javascript served on Google Search. We have a couple work arounds that all seem to rely on making sure everything happens before the popState to avoid a scroll position flash to the user. This mechanism has always been finicky, and it would be awesome to see a cross platform solution to this.
<rdar://problem/22614568>
Note this is now part of the HTML5 spec: https://html.spec.whatwg.org/multipage/browsers.html#dom-history-scroll-restoration and is being implemented in Firefox (in addition to shipping in Chromium).
*** Bug 51899 has been marked as a duplicate of this bug. ***
Created attachment 299471 [details] Patch
Created attachment 299473 [details] Patch
Comment on attachment 299473 [details] Patch Attachment 299473 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2931605 New failing tests: fast/history/history-scroll-restoration.html imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-basic.html fast/dom/Window/window-appendages-cleared.html media/audio-concurrent-supported.html
Created attachment 299476 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 299473 [details] Patch Attachment 299473 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2931616 New failing tests: imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-basic.html fast/dom/Window/window-appendages-cleared.html imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-cross-origin.html
Created attachment 299477 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 299473 [details] Patch Attachment 299473 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2931632 New failing tests: fast/history/history-scroll-restoration.html imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-basic.html fast/dom/Window/window-appendages-cleared.html imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-cross-origin.html
Created attachment 299478 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 299473 [details] Patch Attachment 299473 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2932014 New failing tests: fast/history/history-scroll-restoration.html imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-basic.html fast/dom/Window/window-appendages-cleared.html
Created attachment 299485 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 299499 [details] Patch
Comment on attachment 299499 [details] Patch Attachment 299499 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2933700 New failing tests: fast/history/history-scroll-restoration.html
Created attachment 299505 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 299499 [details] Patch Attachment 299499 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2933709 New failing tests: imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-cross-origin.html
Created attachment 299506 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 299499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299499&action=review > Source/WebCore/history/HistoryItem.h:221 > + float m_pageScaleFactor { 0 }; // 0 indicates "unset". Should this be changed to an Optional<float> to make it more clear? > Source/WebCore/page/History.cpp:77 > +ExceptionOr<History::ScrollRestoration> History::scrollRestoration() const > +{ > + auto* historyItem = m_frame->loader().history().currentItem(); > + if (!historyItem) > + return ScrollRestoration::Auto; > + > + return historyItem->shouldRestoreScrollPosition() ? ScrollRestoration::Auto : ScrollRestoration::Manual; > +} > + > +ExceptionOr<void> History::setScrollRestoration(ScrollRestoration scrollRestoration) > +{ > + auto* historyItem = m_frame->loader().history().currentItem(); > + if (historyItem) > + historyItem->setShouldRestoreScrollPosition(scrollRestoration == ScrollRestoration::Auto); > + > + return { }; > +} Neither of these seem to ever throw an exception. Why ExceptionOr?
(In reply to comment #21) > Comment on attachment 299499 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299499&action=review > > > Source/WebCore/history/HistoryItem.h:221 > > + float m_pageScaleFactor { 0 }; // 0 indicates "unset". > > Should this be changed to an Optional<float> to make it more clear? It should, yes (existing code). > > Source/WebCore/page/History.cpp:77 > > +ExceptionOr<History::ScrollRestoration> History::scrollRestoration() const > > +{ > > + auto* historyItem = m_frame->loader().history().currentItem(); > > + if (!historyItem) > > + return ScrollRestoration::Auto; > > + > > + return historyItem->shouldRestoreScrollPosition() ? ScrollRestoration::Auto : ScrollRestoration::Manual; > > +} > > + > > +ExceptionOr<void> History::setScrollRestoration(ScrollRestoration scrollRestoration) > > +{ > > + auto* historyItem = m_frame->loader().history().currentItem(); > > + if (historyItem) > > + historyItem->setShouldRestoreScrollPosition(scrollRestoration == ScrollRestoration::Auto); > > + > > + return { }; > > +} > > Neither of these seem to ever throw an exception. Why ExceptionOr? The spec says 'If this History object is associated with a Document that is not fully active, both getting and setting must instead throw a "SecurityError" DOMException." but I wasn't sure if we reflect that state.
(In reply to comment #22) > (In reply to comment #21) > > Comment on attachment 299499 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=299499&action=review > > > > > Source/WebCore/history/HistoryItem.h:221 > > > + float m_pageScaleFactor { 0 }; // 0 indicates "unset". > > > > Should this be changed to an Optional<float> to make it more clear? > > It should, yes (existing code). > > > > Source/WebCore/page/History.cpp:77 > > > +ExceptionOr<History::ScrollRestoration> History::scrollRestoration() const > > > +{ > > > + auto* historyItem = m_frame->loader().history().currentItem(); > > > + if (!historyItem) > > > + return ScrollRestoration::Auto; > > > + > > > + return historyItem->shouldRestoreScrollPosition() ? ScrollRestoration::Auto : ScrollRestoration::Manual; > > > +} > > > + > > > +ExceptionOr<void> History::setScrollRestoration(ScrollRestoration scrollRestoration) > > > +{ > > > + auto* historyItem = m_frame->loader().history().currentItem(); > > > + if (historyItem) > > > + historyItem->setShouldRestoreScrollPosition(scrollRestoration == ScrollRestoration::Auto); > > > + > > > + return { }; > > > +} > > > > Neither of these seem to ever throw an exception. Why ExceptionOr? > > The spec says 'If this History object is associated with a Document that is > not fully active, both getting and setting must instead throw a > "SecurityError" DOMException." but I wasn't sure if we reflect that state. We reflect that as a Document without a Frame pointer.
I am excited to see this happen \o/. Also great to see most of web-platform-tests passing which hopefully should mean good interop behavior. The only exception seems to be |scroll-restoration-fragment-scrolling-same-origin.html|. It may not be relevant to webkit but in Blink I had to change the FrameLoader::processFragment logic to respect the manual scroll restoration to make that work I think. [1] [1] https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?dr=Ss&q=scrollToFragment&sq=package:chromium&l=1549
Ah, thanks for the hint!
Created attachment 301291 [details] Patch
Comment on attachment 301291 [details] Patch Attachment 301291 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3071866 New failing tests: fast/history/history-scroll-restoration.html
Created attachment 301294 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 301291 [details] Patch Attachment 301291 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3071871 New failing tests: imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-cross-origin.html
Created attachment 301295 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 301291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301291&action=review > Source/WebCore/page/History.cpp:77 > +ExceptionOr<History::ScrollRestoration> History::scrollRestoration() const > +{ > + auto* historyItem = m_frame->loader().history().currentItem(); > + if (!historyItem) > + return ScrollRestoration::Auto; > + > + return historyItem->shouldRestoreScrollPosition() ? ScrollRestoration::Auto : ScrollRestoration::Manual; > +} > + > +ExceptionOr<void> History::setScrollRestoration(ScrollRestoration scrollRestoration) > +{ > + auto* historyItem = m_frame->loader().history().currentItem(); > + if (historyItem) > + historyItem->setShouldRestoreScrollPosition(scrollRestoration == ScrollRestoration::Auto); > + > + return { }; > +} Unless you are going to start throwing exceptions here, I would remove the ExceptionOr return types. When/if an exception is needed, they can be added back.
Testing is blocked by bug 127676.
Still need to fix iOS WK2.
Created attachment 301392 [details] patching test for CORS
Created attachment 302314 [details] Patch
Comment on attachment 302314 [details] Patch Attachment 302314 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3168633 New failing tests: fast/history/history-scroll-restoration.html
Created attachment 302330 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 302314 [details] Patch Attachment 302314 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3168558 New failing tests: imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-navigation-samedoc.html
Created attachment 302331 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 302314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302314&action=review > Source/WebCore/page/DOMWindow.cpp:1335 > + WTFLogAlways("DOMWindow::scrollY returning early %d", scrollY); Probably want to remove this. > Source/WebCore/page/DOMWindow.cpp:1342 > + WTFLogAlways("DOMWindow::scrollY returning %d", result); Ditto. > Source/WebCore/page/History.cpp:62 > +ExceptionOr<History::ScrollRestoration> History::scrollRestoration() const Do you disagree with the feedback I have given in previous reviews about removing the ExceptionOr? > Source/WebKit2/Platform/Logging.cpp:61 > + WebKit2LogVisibleRects.state = WTFLogChannelOn; > + WebKit2LogViewState.state = WTFLogChannelOn; > + WebKit2LogViewGestures.state = WTFLogChannelOn; I don't think you meant to keep this in.
(In reply to comment #40) > Do you disagree with the feedback I have given in previous reviews about > removing the ExceptionOr? No, this patch was meant to be just for EWS :)
(In reply to comment #41) > (In reply to comment #40) > > > Do you disagree with the feedback I have given in previous reviews about > > removing the ExceptionOr? > > No, this patch was meant to be just for EWS :) Ah. I usually don't set the review=? flag in those cases. If you are using webkit-patch post, you can use the --no-review flag (but note, you will have to click to start the EWS).
Created attachment 303533 [details] Patch
Created attachment 303541 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 303541 [details] Patch Attachment 303541 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3254192 New failing tests: fast/history/history-scroll-restoration.html media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html
Created attachment 303551 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 303541 [details] Patch Attachment 303541 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3254224 New failing tests: fast/history/history-scroll-restoration.html imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-cross-origin.html
Created attachment 303557 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 303623 [details] Patch
Comment on attachment 303623 [details] Patch Attachment 303623 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3257127 New failing tests: fast/history/history-scroll-restoration.html
Created attachment 303630 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 303726 [details] Patch
Comment on attachment 303726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303726&action=review > Source/WebCore/ChangeLog:9 > + Add Mac support for history.scrollRestoration, per spec: Is this really just Mac? > Source/WebCore/loader/FrameLoader.cpp:2871 > +static bool itemAllowsScrollRestoration(HistoryItem *historyItem) * on the wrong side.
https://trac.webkit.org/r213590