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: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL: https://w3c-test.org/css/cssom-view/s...
Keywords: InRadar
: 193820 (view as bug list)
Depends on: 143609
Blocks: 5991 182230
  Show dependency treegraph
 
Reported: 2018-01-24 09:34 PST by Frédéric Wang (:fredw)
Modified: 2019-11-23 05:01 PST (History)
16 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, EWS Watchlist
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, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (2.98 MB, application/zip)
2018-01-26 04:09 PST, EWS Watchlist
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, EWS Watchlist
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, EWS Watchlist
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, EWS Watchlist
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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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 EWS Watchlist 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>
Comment 36 Simon Fraser (smfr) 2019-07-22 11:00:00 PDT
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?
Comment 37 Frédéric Wang (:fredw) 2019-07-22 14:06:21 PDT
(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.
Comment 38 Simon Fraser (smfr) 2019-07-22 14:10:55 PDT
(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.
Comment 39 Frédéric Wang (:fredw) 2019-07-22 14:35:55 PDT
(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 ;-)
Comment 40 Darin Adler 2019-07-22 15:17:18 PDT
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!
Comment 41 Robbert Brak 2019-11-23 05:01:47 PST
*** Bug 193820 has been marked as a duplicate of this bug. ***