Description
Majid Valipour
2015-08-07 07:40:58 PDT
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. 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. |