Bug 182053 - [CSSOM View] Implement standard behavior for scrollingElement
Summary: [CSSOM View] Implement standard behavior for scrollingElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 143609
Blocks: 5991 182230
  Show dependency treegraph
 
Reported: 2018-01-24 09:34 PST by Frédéric Wang (:fredw)
Modified: 2018-08-31 00:53 PDT (History)
15 users (show)

See Also:


Attachments
Patch (WIP) (13.09 KB, patch)
2018-01-24 09:34 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (28.34 KB, patch)
2018-01-26 02:39 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.30 MB, application/zip)
2018-01-26 03:48 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.68 MB, application/zip)
2018-01-26 03:53 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (2.98 MB, application/zip)
2018-01-26 04:09 PST, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (8.44 MB, application/zip)
2018-01-26 06:21 PST, Build Bot
no flags Details
Patch (27.76 KB, patch)
2018-01-26 12:25 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.04 MB, application/zip)
2018-01-26 13:35 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (8.27 MB, application/zip)
2018-01-26 14:01 PST, Build Bot
no flags Details
Patch (33.82 KB, patch)
2018-01-27 02:06 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (33.81 KB, patch)
2018-01-27 02:36 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (33.82 KB, patch)
2018-01-27 07:37 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (31.81 KB, patch)
2018-01-28 22:17 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (31.29 KB, patch)
2018-02-07 01:58 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (31.93 KB, patch)
2018-05-10 02:03 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (31.88 KB, patch)
2018-07-10 07:42 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (31.80 KB, patch)
2018-08-30 08:09 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for landing (31.41 KB, patch)
2018-08-31 00:11 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-01-24 09:34:55 PST
Created attachment 332173 [details]
Patch (WIP)

First patch for bug 5991, which introduces a development flag.
Comment 1 Daniel Bates 2018-01-24 20:53:33 PST
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?
Comment 2 Frédéric Wang (:fredw) 2018-01-25 00:05:21 PST
(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.
Comment 3 Frédéric Wang (:fredw) 2018-01-26 02:39:20 PST
Created attachment 332362 [details]
Patch
Comment 4 Antonio Gomes 2018-01-26 03:20:53 PST
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 5 Build Bot 2018-01-26 03:48:25 PST
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
Comment 6 Build Bot 2018-01-26 03:48:26 PST
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 7 Build Bot 2018-01-26 03:53:10 PST
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
Comment 8 Build Bot 2018-01-26 03:53:11 PST
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 9 Build Bot 2018-01-26 04:09:20 PST
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
Comment 10 Build Bot 2018-01-26 04:09:21 PST
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 11 Build Bot 2018-01-26 06:21:07 PST
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
Comment 12 Build Bot 2018-01-26 06:21:09 PST
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 13 Frédéric Wang (:fredw) 2018-01-26 12:19:36 PST
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.
Comment 14 Frédéric Wang (:fredw) 2018-01-26 12:25:29 PST
Created attachment 332402 [details]
Patch
Comment 15 Build Bot 2018-01-26 13:35:21 PST
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
Comment 16 Build Bot 2018-01-26 13:35:22 PST
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 17 Build Bot 2018-01-26 14:01:05 PST
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
Comment 18 Build Bot 2018-01-26 14:01:06 PST
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
Comment 19 Frédéric Wang (:fredw) 2018-01-27 02:06:26 PST
Created attachment 332465 [details]
Patch
Comment 20 Frédéric Wang (:fredw) 2018-01-27 02:36:59 PST
Created attachment 332466 [details]
Patch
Comment 21 Frédéric Wang (:fredw) 2018-01-27 07:37:39 PST
Created attachment 332471 [details]
Patch
Comment 22 Frédéric Wang (:fredw) 2018-01-28 22:17:48 PST
Created attachment 332510 [details]
Patch
Comment 23 Frédéric Wang (:fredw) 2018-02-01 07:58:23 PST
@cdumez: Can you please review this patch?
Comment 24 Simon Fraser (smfr) 2018-02-01 10:35:06 PST
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 25 Frédéric Wang (:fredw) 2018-02-05 02:29:13 PST
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?
Comment 26 Frédéric Wang (:fredw) 2018-02-07 01:58:07 PST
Created attachment 333273 [details]
Patch
Comment 27 Frédéric Wang (:fredw) 2018-02-27 03:27:21 PST
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
Comment 28 Frédéric Wang (:fredw) 2018-05-10 02:03:42 PDT
Created attachment 340078 [details]
Patch

Rebasing...
Comment 29 Frédéric Wang (:fredw) 2018-07-10 07:42:08 PDT
Created attachment 344694 [details]
Patch

Rebasing...
Comment 30 Frédéric Wang (:fredw) 2018-08-30 08:09:24 PDT
Created attachment 348497 [details]
Patch

Rebase...
Comment 31 Simon Fraser (smfr) 2018-08-30 10:42:22 PDT
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().
Comment 32 Frédéric Wang (:fredw) 2018-08-31 00:11:50 PDT
Created attachment 348615 [details]
Patch for landing
Comment 33 WebKit Commit Bot 2018-08-31 00:49:41 PDT
Comment on attachment 348615 [details]
Patch for landing

Clearing flags on attachment: 348615

Committed r235539: <https://trac.webkit.org/changeset/235539>
Comment 34 WebKit Commit Bot 2018-08-31 00:49:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Radar WebKit Bug Importer 2018-08-31 00:53:28 PDT
<rdar://problem/43928850>