Bug 182230

Created attachment 332512 [details] Patch (WIP)

Created attachment 332513 [details] 182053+182230 for EWS

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.

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

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

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

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

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

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

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

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

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

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

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

Created attachment 332653 [details] Patch This patch applies after the dependencies.

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.

Created attachment 332857 [details] Patch (182053+182287+182230) for EWS Rebasing over bug 182287.

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

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

Created attachment 332874 [details] Patch

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

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

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

Created attachment 348619 [details] Patch Rebasing...

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

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

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

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

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

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

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

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

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

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

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.

Created attachment 348769 [details] Patch

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/

Created attachment 348772 [details] Patch

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.

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

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

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.

Created attachment 349118 [details] Patch

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.

Created attachment 349170 [details] Patch

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

Created attachment 349174 [details] Patch

Comment on attachment 349174 [details] Patch Clearing flags on attachment: 349174 Committed r235806: <https://trac.webkit.org/changeset/235806>

All reviewed patches have been landed. Closing bug.

<rdar://problem/44237945>

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.

(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

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.

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

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

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

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

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