Bug 182053 – [CSSOM View] Implement standard behavior for scrollingElement

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Frédéric Wang (:fredw)]

Created attachment 332173 [details]
Patch (WIP)

First patch for bug 5991, which introduces a development flag.

[Daniel Bates]

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?

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

Created attachment 332362 [details]
Patch

[Antonio Gomes]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Created attachment 332402 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 332465 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 332466 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 332471 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 332510 [details]
Patch

[Frédéric Wang (:fredw)]

@cdumez: Can you please review this patch?

[Simon Fraser (smfr)]

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?

[Frédéric Wang (:fredw)]

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?

[Frédéric Wang (:fredw)]

Created attachment 333273 [details]
Patch

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

Created attachment 340078 [details]
Patch

Rebasing...

[Frédéric Wang (:fredw)]

Created attachment 344694 [details]
Patch

Rebasing...

[Frédéric Wang (:fredw)]

Created attachment 348497 [details]
Patch

Rebase...

[Simon Fraser (smfr)]

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().

[Frédéric Wang (:fredw)]

Created attachment 348615 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 348615 [details]
Patch for landing

Clearing flags on attachment: 348615

Committed r235539: <https://trac.webkit.org/changeset/235539>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43928850>

[Simon Fraser (smfr)]

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?

[Frédéric Wang (:fredw)]

(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.

[Simon Fraser (smfr)]

(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.

[Frédéric Wang (:fredw)]

(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 ;-)

[Darin Adler]

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!

[Robbert Brak]

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