[CSSOM View] Handle the scrollingElement in Element::scroll(Left/Top/Width/Height/To)

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

.

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

Created attachment 332512 [details]
Patch (WIP)

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

Created attachment 332513 [details]
182053+182230 for EWS

[EWS Watchlist]

Attachment 332513 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Element.cpp:56:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 332513 [details]
182053+182230 for EWS

Attachment 332513 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6246447

New failing tests:
tiled-drawing/scrolling/frames/fixed-inside-frame.html
tiled-drawing/scrolling/fast-scroll-mainframe-zoom.html
http/tests/navigation/anchor-frames.html
fast/dom/window-scroll-scaling.html
fast/multicol/scrolling-overflow.html
fast/dom/Element/body-scrollLeft.html
fast/dom/Element/documentElement-scrollTop.html
http/tests/navigation/anchor-frames-gbk.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/scroll-to-fragid/003.html
fast/dom/Element/scrollTop.html
fast/scrolling/latching/scroll-div-no-latching.html
fast/dom/Element/body-scrollTop.html
fast/events/scroll-in-scaled-page-with-overflow-hidden.html
fast/dom/Element/scrollLeft.html
tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html
fast/scrolling/latching/scroll-nested-iframe.html
fast/dom/Element/scrolling-funtions-on-body.html
tiled-drawing/scrolling/iframe_in_iframe.html
fast/dom/Element/documentElement-scrollLeft.html
fast/scrolling/latching/iframe_in_iframe.html
fast/scrolling/latching/scroll-latched-nested-div.html

[EWS Watchlist]

Created attachment 332516 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332513 [details]
182053+182230 for EWS

Attachment 332513 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6246462

New failing tests:
fast/dom/Element/scrollLeft.html
fast/multicol/scrolling-overflow.html
fast/dom/Element/body-scrollLeft.html
platform/ios/ios/fast/coordinates/page-offsets.html
fast/dom/Element/documentElement-scrollTop.html
fast/dom/Element/scrollTop.html
fast/dom/Element/body-scrollTop.html
fast/dom/Element/scrolling-funtions-on-body.html
imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/scroll-to-fragid/003.html
fast/dom/Element/documentElement-scrollLeft.html

[EWS Watchlist]

Created attachment 332519 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332513 [details]
182053+182230 for EWS

Attachment 332513 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6246483

New failing tests:
http/tests/navigation/anchor-frames-same-origin.html
http/tests/navigation/anchor-frames.html
fast/dom/window-scroll-scaling.html
fast/multicol/scrolling-overflow.html
fast/dom/Element/body-scrollLeft.html
fast/dom/Element/documentElement-scrollTop.html
http/tests/navigation/anchor-frames-gbk.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/scroll-to-fragid/003.html
fast/scrolling/latching/scroll-div-no-latching.html
fast/dom/Element/body-scrollTop.html
fast/events/scroll-in-scaled-page-with-overflow-hidden.html
fast/dom/Element/scrollLeft.html
fast/scrolling/latching/scroll-nested-iframe.html
fast/dom/Element/scrolling-funtions-on-body.html
fast/dom/Element/scrollTop.html
fast/dom/Element/documentElement-scrollLeft.html
fast/scrolling/latching/iframe_in_iframe.html
fast/scrolling/latching/scroll-latched-nested-div.html

[EWS Watchlist]

Created attachment 332520 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332513 [details]
182053+182230 for EWS

Attachment 332513 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6247288

New failing tests:
http/tests/navigation/anchor-frames-same-origin.html
http/tests/navigation/anchor-frames.html
fast/dom/window-scroll-scaling.html
fast/multicol/scrolling-overflow.html
fast/dom/Element/body-scrollLeft.html
fast/dom/Element/documentElement-scrollTop.html
http/tests/navigation/anchor-frames-gbk.html
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/scroll-to-fragid/003.html
fast/scrolling/latching/scroll-div-no-latching.html
fast/dom/Element/body-scrollTop.html
fast/events/scroll-in-scaled-page-with-overflow-hidden.html
fast/dom/Element/scrollLeft.html
fast/scrolling/latching/scroll-nested-iframe.html
fast/dom/Element/scrolling-funtions-on-body.html
fast/dom/Element/scrollTop.html
fast/dom/Element/documentElement-scrollLeft.html
fast/scrolling/latching/iframe_in_iframe.html
fast/scrolling/latching/scroll-latched-nested-div.html

[EWS Watchlist]

Created attachment 332529 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

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

Created attachment 332645 [details]
Patch (182053+182230+182241) for EWS

[EWS Watchlist]

Comment on attachment 332645 [details]
Patch (182053+182230+182241) for EWS

Attachment 332645 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6259872

New failing tests:
imported/w3c/web-platform-tests/cssom-view/scrolling-quirks-vs-nonquirks.html

[EWS Watchlist]

Created attachment 332650 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

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

Created attachment 332653 [details]
Patch

This patch applies after the dependencies.

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

Comment on attachment 332653 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332653&action=review

> LayoutTests/fast/events/scroll-in-scaled-page-with-overflow-hidden.html:21
> +        internals.settings.setCSSOMViewScrollingAPIEnabled(false);

This does not work well. I handled it better in bug 182287 instead.

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

Created attachment 332857 [details]
Patch (182053+182287+182230) for EWS

Rebasing over bug 182287.

[EWS Watchlist]

Comment on attachment 332857 [details]
Patch (182053+182287+182230) for EWS

Attachment 332857 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6313714

New failing tests:
tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html

[EWS Watchlist]

Created attachment 332864 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

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

Created attachment 332874 [details]
Patch

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

Created attachment 340079 [details]
Patch (applies on top of 182053)

Rebasing...

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

Created attachment 344695 [details]
Patch (applies on top of 182053)

Rebasing...

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

Created attachment 348498 [details]
Patch (applies on top of 182053)

Rebase...

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

Created attachment 348619 [details]
Patch

Rebasing...

[EWS Watchlist]

Comment on attachment 348619 [details]
Patch

Attachment 348619 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9047905

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html

[EWS Watchlist]

Created attachment 348621 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 348619 [details]
Patch

Attachment 348619 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9048026

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html

[EWS Watchlist]

Created attachment 348623 [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 348619 [details]
Patch

Attachment 348619 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9047989

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html

[EWS Watchlist]

Created attachment 348625 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 348619 [details]
Patch

Attachment 348619 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9048008

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html

[EWS Watchlist]

Created attachment 348627 [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.13.4

[EWS Watchlist]

Comment on attachment 348619 [details]
Patch

Attachment 348619 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9048802

New failing tests:
imported/w3c/web-platform-tests/css/cssom-view/HTMLBody-ScrollArea_quirksmode.html
imported/w3c/web-platform-tests/css/cssom-view/scrollingElement.html
imported/w3c/web-platform-tests/css/cssom-view/scrolling-quirks-vs-nonquirks.html

[EWS Watchlist]

Created attachment 348634 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

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

The new failures are due to the fact that the cssom-view test suite has been moved into wpt/css (I did not notice it because the old version is still present). See bug 189241.

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

Created attachment 348769 [details]
Patch

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

Comment on attachment 348769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348769&action=review

> LayoutTests/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html:53
> +    var pageScrollPositionAfter = document.scrollingELement.scrollTop;

oops, this should be s/scrollingELement/scrollingElement/

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

Created attachment 348772 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 348772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348772&action=review

> Source/WebCore/dom/Document.cpp:1466
> +    if (settings().CSSOMViewScrollingAPIEnabled() && inQuirksMode())

inQuirksModeI() is the more efficient check: put it first.

> Source/WebCore/dom/Document.h:453
>      WEBCORE_EXPORT Element* scrollingElement();
> +    // When calling from C++ code, use this method. scrollingElement() is just for the web IDL implementation.
> +    Element* scrollingElementNoLayout();

It might be clearer to flip the naming here; rename scrollingElement() to scrollingElementForAPI or scrollingElementUpdatingLayout() or something, and make the no-layout version just be scrollingElement().

> Source/WebCore/dom/Element.cpp:817
> +static int adjustForZoom(int value, const Frame& frame)

This should say what is being adjusted. What is 'value'? Is it a scroll position or offset?

> Source/WebCore/dom/Element.cpp:824
> +    // Needed because of truncation (rather than rounding) when scaling up.
> +    if (zoomFactor > 1)
> +        value++;

Weird. Why not just floor/ceil after the division by zoomFactor?

> Source/WebCore/dom/Element.cpp:979
> +        RefPtr<Frame> frame = document().frame();
> +        if (!frame)
> +            return 0;
> +        RefPtr<FrameView> view = frame->view();
> +        if (!view)
> +            return 0;

This appears 6 times. Share it.

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

(In reply to Simon Fraser (smfr) from comment #39)
> inQuirksModeI() is the more efficient check: put it first.
> 
> It might be clearer to flip the naming here; rename scrollingElement() to
> scrollingElementForAPI or scrollingElementUpdatingLayout() or something, and
> make the no-layout version just be scrollingElement().

Good points, I'll do it.

> This should say what is being adjusted. What is 'value'? Is it a scroll
> position or offset?
>
> Weird. Why not just floor/ceil after the division by zoomFactor?
> 
> This appears 6 times. Share it.

Note that I only moved existing code from HTMLBodyElement.cpp. Maybe these changes should be handled separately to make them clearer in the history.

[Simon Fraser (smfr)]

(In reply to Frédéric Wang (:fredw) from comment #40)
> (In reply to Simon Fraser (smfr) from comment #39)
> > inQuirksModeI() is the more efficient check: put it first.
> > 
> > It might be clearer to flip the naming here; rename scrollingElement() to
> > scrollingElementForAPI or scrollingElementUpdatingLayout() or something, and
> > make the no-layout version just be scrollingElement().
> 
> Good points, I'll do it.
> 
> > This should say what is being adjusted. What is 'value'? Is it a scroll
> > position or offset?
> >
> > Weird. Why not just floor/ceil after the division by zoomFactor?
> > 
> > This appears 6 times. Share it.
> 
> Note that I only moved existing code from HTMLBodyElement.cpp. Maybe these
> changes should be handled separately to make them clearer in the history.

I think it's OK to do a refactor when moving code.

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

Comment on attachment 348772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348772&action=review

>> Source/WebCore/dom/Element.cpp:817
>> +static int adjustForZoom(int value, const Frame& frame)
> 
> This should say what is being adjusted. What is 'value'? Is it a scroll position or offset?

The current code uses this for both offset and size.

>> Source/WebCore/dom/Element.cpp:824
>> +        value++;
> 
> Weird. Why not just floor/ceil after the division by zoomFactor?

I've opened bug 189397 for this one as there are several zooming functions with different implementation. I prefer not risk unrelated behavior changes in this bug.

>> Source/WebCore/dom/Element.cpp:979
>> +            return 0;
> 
> This appears 6 times. Share it.

I can do that one in this bug.

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

Created attachment 349118 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 349118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349118&action=review

Makes sure it builds!

> Source/WebCore/dom/Element.cpp:861
> +static int adjustForZoom(int value, const Frame& frame)

Still needs a better name.

> Source/WebCore/dom/Element.cpp:866
> +    // FIXME(https://webkit.org/b/189397): Why can't we just ceil/floor?

Space after FIXME, and you can just say webkit.org/b/189397.

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

Created attachment 349170 [details]
Patch

[EWS Watchlist]

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

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

Created attachment 349174 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 349174 [details]
Patch

Clearing flags on attachment: 349174

Committed r235806: <https://trac.webkit.org/changeset/235806>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44237945>

[Truitt Savell]

Looks like this patch https://trac.webkit.org/changeset/235806/webkit

has caused https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-iframe-latched-mainframe.html

to become flakey timeout. 

Test History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-iframe-latched-mainframe.html

I reproduced this flakiness using command:

run-webkit-tests tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe.html --iterations 500 -f

on a build of 235805 and no timeouts occurred. running this on 235806 and the failure reproduces easily.

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

(In reply to Truitt Savell from comment #51)
> Looks like this patch https://trac.webkit.org/changeset/235806/webkit

Thanks, I wonder whether it's related to the current implementation of Document::isBodyPotentiallyScrollable (see bug 182292).

I guess a workaround for now would be to use documentElement instead of scrollingElement in LayoutTests/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html

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

Created attachment 349311 [details]
Patch (follow-up fix)

(In reply to Frédéric Wang (:fredw) from comment #52)
> (In reply to Truitt Savell from comment #51)
> > Looks like this patch https://trac.webkit.org/changeset/235806/webkit
> 
> Thanks, I wonder whether it's related to the current implementation of
> Document::isBodyPotentiallyScrollable (see bug 182292).
> 
> I guess a workaround for now would be to use documentElement instead of
> scrollingElement in
> LayoutTests/tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html

I stand corrected. I thought the issue was with tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html...
For fast-scroll-iframe-latched-mainframe.html, just replacing document.body with document.scrollingElement avoids the flackiness for me.

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

Committed r235855: <https://trac.webkit.org/changeset/235855>

[Truitt Savell]

Looks like there were a total of 3 tests effected by this change all with similar names. 

tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe-with-handler.html 
tiled-drawing/scrolling/fast-scroll-select-latched-mainframe.html 
tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe.html

Combined History:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-iframe-latched-mainframe-with-handler.html%20%20tiled-drawing%2Fscrolling%2Ffast-scroll-select-latched-mainframe.html%20%20tiled-drawing%2Fscrolling%2Ffast-scroll-iframe-latched-mainframe.html

[Truitt Savell]

Another Timeout:
tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-div-latched-mainframe-with-handler.html

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

(In reply to Truitt Savell from comment #56)
> Another Timeout:
> tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=tiled-drawing%2Fscrolling%2Ffast-scroll-div-
> latched-mainframe-with-handler.html

This is the history tiled-drawing/scrolling/fast-scroll-div-latched-mainframe-with-handler.html ; tiled-drawing/scrolling/fast-scroll-div-latched-mainframe.html had already been fixed in the initial patch and does not look flaky.

(In reply to Truitt Savell from comment #55)
> tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe-with-handler.
> html 
> tiled-drawing/scrolling/fast-scroll-select-latched-mainframe.html 
> tiled-drawing/scrolling/fast-scroll-iframe-latched-mainframe.html

OK, actually it seems a bunch of new tests using document.body instead of document.scrolingElement have been added since the patch for bug 182241 landed. I'll take care of it.

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

(In reply to Frédéric Wang (:fredw) from comment #57)
> OK, actually it seems a bunch of new tests using document.body instead of
> document.scrolingElement have been added since the patch for bug 182241
> landed. I'll take care of it.

Hopefully, bug 189495 will address the remaining flaky results.