Bug 182053

Comment on attachment 332173 [details] Patch (WIP) View in context: https://bugs.webkit.org/attachment.cgi?id=332173&action=review > Source/WebCore/dom/Document.cpp:1441 > + if (settings().scrollingAPIofCSSOMViewEnabled()) { Can we come up with a better name for this? Maybe cssomViewScrollingAPIEnabled?

(In reply to Daniel Bates from comment #1) > Can we come up with a better name for this? Maybe > cssomViewScrollingAPIEnabled? Yes, that's what I initially did. My concern was that some functions would be named cssom*, others Cssom* while the real spec name is CSSOM. Anyway, sure I can change it.

Created attachment 332362 [details] Patch

Comment on attachment 332362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332362&action=review > Source/WebCore/ChangeLog:10 > + It changes the behavior of document.scrollingElement so that it follow the spec when the nit: follows* > Source/WebCore/dom/Document.cpp:1444 > + // - The element is not the HTML body element, or it is and the root elementâs used value of the nit: strange character after 'element' > Source/WebCore/dom/Document.cpp:1446 > + // - The elementâs used value of the overflow-x or overflow-y properties is not visible. ditto

Comment on attachment 332362 [details] Patch Attachment 332362 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6220109 New failing tests: fast/scrolling/arrow-key-scroll-in-rtl-document.html imported/w3c/web-platform-tests/cssom-view/HTMLBody-ScrollArea_quirksmode.html fast/scrolling/scroll-to-focused-element-asynchronously.html fast/overflow/scroll-anchor-in-position-fixed.html fast/overflow/scroll-anchor-in-overflow-in-position-fixed.html fast/events/prevent-default-prevents-interaction-with-scrollbars.html imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html fast/visual-viewport/zoomed-scroll-into-view-fixed.html fast/scrolling/scroll-to-anchor-zoomed-header.html fast/visual-viewport/zoomed-scroll-to-anchor-in-position-fixed.html

Created attachment 332363 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 332362 [details] Patch Attachment 332362 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6220117 New failing tests: fast/scrolling/arrow-key-scroll-in-rtl-document.html imported/w3c/web-platform-tests/cssom-view/HTMLBody-ScrollArea_quirksmode.html fast/scrolling/scroll-to-focused-element-asynchronously.html fast/overflow/scroll-anchor-in-position-fixed.html fast/scrolling/rtl-scrollbars-alternate-body-dir-attr-does-not-update-scrollbar-placement.html fast/overflow/scroll-anchor-in-overflow-in-position-fixed.html fast/events/prevent-default-prevents-interaction-with-scrollbars.html imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html fast/visual-viewport/zoomed-scroll-into-view-fixed.html fast/scrolling/scroll-to-anchor-zoomed-header.html fast/visual-viewport/zoomed-scroll-to-anchor-in-position-fixed.html fast/scrolling/iframe-scrollable-after-back.html

Created attachment 332364 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Comment on attachment 332362 [details] Patch Attachment 332362 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6220128 New failing tests: fast/scrolling/arrow-key-scroll-in-rtl-document.html imported/w3c/web-platform-tests/cssom-view/HTMLBody-ScrollArea_quirksmode.html fast/scrolling/scroll-to-focused-element-asynchronously.html fast/overflow/scroll-anchor-in-position-fixed.html fast/overflow/scroll-anchor-in-overflow-in-position-fixed.html fast/events/prevent-default-prevents-interaction-with-scrollbars.html imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html fast/visual-viewport/zoomed-scroll-into-view-fixed.html fast/scrolling/scroll-to-anchor-zoomed-header.html fast/visual-viewport/zoomed-scroll-to-anchor-in-position-fixed.html

Created attachment 332365 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 332362 [details] Patch Attachment 332362 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6220881 New failing tests: fast/visual-viewport/ios/stable-update-with-keyboard.html imported/w3c/web-platform-tests/cssom-view/HTMLBody-ScrollArea_quirksmode.html fast/scrolling/scroll-to-focused-element-asynchronously.html fast/scrolling/ios/touch-scroll-pointer-events-none.html fast/overflow/scroll-anchor-in-position-fixed.html fast/scrolling/ios/touch-scroll-visibility-hidden.html fast/overflow/scroll-anchor-in-overflow-in-position-fixed.html imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html

Created attachment 332371 [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.12.6

Comment on attachment 332362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332362&action=review > Tools/DumpRenderTree/mac/DumpRenderTree.mm:862 > + [preferences setCSSOMViewScrollingAPIEnabled:YES]; Oops, intent was to keep the flag disabled for now. > Tools/DumpRenderTree/win/DumpRenderTree.cpp:787 > + prefsPrivate->setCSSOMViewScrollingAPIEnabled(TRUE); Ditto.

Created attachment 332402 [details] Patch

Comment on attachment 332402 [details] Patch Attachment 332402 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6224480 New failing tests: fast/scrolling/arrow-key-scroll-in-rtl-document.html imported/w3c/web-platform-tests/cssom-view/HTMLBody-ScrollArea_quirksmode.html fast/scrolling/scroll-to-focused-element-asynchronously.html fast/overflow/scroll-anchor-in-position-fixed.html fast/scrolling/rtl-scrollbars-alternate-body-dir-attr-does-not-update-scrollbar-placement.html fast/overflow/scroll-anchor-in-overflow-in-position-fixed.html fast/events/prevent-default-prevents-interaction-with-scrollbars.html imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html fast/visual-viewport/zoomed-scroll-into-view-fixed.html fast/scrolling/scroll-to-anchor-zoomed-header.html fast/visual-viewport/zoomed-scroll-to-anchor-in-position-fixed.html fast/scrolling/iframe-scrollable-after-back.html

Created attachment 332412 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Comment on attachment 332402 [details] Patch Attachment 332402 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6224532 New failing tests: fast/visual-viewport/ios/stable-update-with-keyboard.html imported/w3c/web-platform-tests/cssom-view/HTMLBody-ScrollArea_quirksmode.html fast/scrolling/scroll-to-focused-element-asynchronously.html fast/scrolling/ios/touch-scroll-pointer-events-none.html fast/overflow/scroll-anchor-in-position-fixed.html fast/scrolling/ios/touch-scroll-visibility-hidden.html fast/overflow/scroll-anchor-in-overflow-in-position-fixed.html imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html

Created attachment 332413 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6

Created attachment 332465 [details] Patch

Created attachment 332466 [details] Patch

Created attachment 332471 [details] Patch

Created attachment 332510 [details] Patch

@cdumez: Can you please review this patch?

Comment on attachment 332510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332510&action=review > Source/WebCore/dom/Document.cpp:1451 > + // FIXME: Use RenderObject::hasOverflowClip() instead of Element::computedStyle()? > + return body.renderer() > + && documentElement()->computedStyle() > + && !documentElement()->computedStyle()->isOverflowVisible() Yeah, we normally don't use computedStyle() for these kinds of things; we just look at the renderer's style() or flags. > Source/WebCore/dom/Document.cpp:1463 > + updateLayout(); Can updateLayout() trigger script which destroyed this Document?

Comment on attachment 332510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332510&action=review >> Source/WebCore/dom/Document.cpp:1451 >> + && !documentElement()->computedStyle()->isOverflowVisible() > > Yeah, we normally don't use computedStyle() for these kinds of things; we just look at the renderer's style() or flags. Unfortunately, as I said in the ChangeLog, this does not seem to work because the OverflowClip on the body depends on the one on the html element, see RenderBox::updateFromStyle() and https://bugs.chromium.org/p/chromium/issues/detail?id=665927#c23. Apparently, Chromium people had to rewrite the inheritance of viewport properties in order to make things work properly, which we might need to do too (see also bug 182292). Since this patch is just supposed to introduce the developer preference with basic scrollingElement support, I thought using computedStyle() was enough for now. >> Source/WebCore/dom/Document.cpp:1463 >> + updateLayout(); > > Can updateLayout() trigger script which destroyed this Document? I am not familiar with this, can you elaborate? Using updateLayoutIgnorePendingStylesheets() as do other Element:: functions, seems enough to pass the tests. Do you think it would be safer?

Created attachment 333273 [details] Patch

Comment on attachment 332510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332510&action=review >>> Source/WebCore/dom/Document.cpp:1463 >>> + updateLayout(); >> >> Can updateLayout() trigger script which destroyed this Document? > > I am not familiar with this, can you elaborate? Using updateLayoutIgnorePendingStylesheets() as do other Element:: functions, seems enough to pass the tests. Do you think it would be safer? I checked again and without updateLayoutIgnorePendingStylesheets(), the test fails. I see that several other functions from Element:: to implement DOM properties (offsetLeft, clientLeft, setScrollTop...) also calls updateLayoutIgnorePendingStylesheets() on the Element's document, so if we could destroy the Document with scrollingElement then I suspect we can already do that with the existing code. Also, Chromium's code has diverged quick a bit but it forces a style and layout update when calling scrollingElement: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?l=1501

Created attachment 340078 [details] Patch Rebasing...

Created attachment 344694 [details] Patch Rebasing...

Created attachment 348497 [details] Patch Rebase...

Comment on attachment 348497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348497&action=review > Source/WebCore/dom/Document.h:1612 > + bool isPotentiallyScrollable(HTMLBodyElement&); This function name is ambiguous. Are you asking of the passed body element is scrollable, or if the document itself is scrollable? Maybe rename to is isBodyPotentiallyScrollable().

Created attachment 348615 [details] Patch for landing

Comment on attachment 348615 [details] Patch for landing Clearing flags on attachment: 348615 Committed r235539: <https://trac.webkit.org/changeset/235539>

All reviewed patches have been landed. Closing bug.

<rdar://problem/43928850>

Comment on attachment 348615 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=348615&action=review > Source/WebCore/page/Settings.yaml:558 > +CSSOMViewScrollingAPIEnabled: > + initial: false Frank, do you recall why you left this off by default here, and only enabled it for WebKit2?

(In reply to Simon Fraser (smfr) from comment #36) > Comment on attachment 348615 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348615&action=review > > > Source/WebCore/page/Settings.yaml:558 > > +CSSOMViewScrollingAPIEnabled: > > + initial: false > > Frank, do you recall why you left this off by default here, and only enabled > it for WebKit2? I'm assuming this question is directed to me? (I can't find any Frank in the cc) IIRC, the biggest backward compatibility concerns were for webviews embedded in apps. So the idea was to keep it disabled by default on (some) Apple ports and Apple would enable it on the Safari side. I'm not really sure about the details, why it is only WK1 and why it had to be disabled by default in trunk instead of doing it on the proprietary side. Maybe there are more explanations in the radar bug.

(In reply to Frédéric Wang (:fredw) from comment #37) > (In reply to Simon Fraser (smfr) from comment #36) > > Comment on attachment 348615 [details] > > Patch for landing > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=348615&action=review > > > > > Source/WebCore/page/Settings.yaml:558 > > > +CSSOMViewScrollingAPIEnabled: > > > + initial: false > > > > Frank, do you recall why you left this off by default here, and only enabled > > it for WebKit2? > > I'm assuming this question is directed to me? (I can't find any Frank in the > cc) > > IIRC, the biggest backward compatibility concerns were for webviews embedded > in apps. So the idea was to keep it disabled by default on (some) Apple > ports and Apple would enable it on the Safari side. I'm not really sure > about the details, why it is only WK1 and why it had to be disabled by > default in trunk instead of doing it on the proprietary side. Maybe there > are more explanations in the radar bug. Sorry Fred, I meant you! We don't normally restrict behavior changes from WebViews in apps unless we have evidence of a compat issue.

(In reply to Simon Fraser (smfr) from comment #38) > Sorry Fred, I meant you! No problem, I guessed it was a mix between Fred and Wang... > We don't normally restrict behavior changes from > WebViews in apps unless we have evidence of a compat issue. OK. I guess it is up to Apple to decide. I'm happy if this is enabled everywhere ;-)

As is usual with such things, I think we have a testing challenge for app compatibility. But I think we had that with both Legacy WebKit and Modern WebKit, and we already took the risk with modern WebKit!

*** Bug 193820 has been marked as a duplicate of this bug. ***