|
Description
Frédéric Wang (:fredw)
2017-06-05 04:52:58 PDT
Created attachment 312007 [details] Patch Extracted from bug 171667 Created attachment 312321 [details]
Patch
Comment on attachment 312321 [details]
Patch
EWS is red.
Comment on attachment 312321 [details] Patch Indeed, this applies on top of bug 172914. Comment on attachment 312321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312321&action=review > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:171 > + for (auto& child : *children()) { Note: the scrolling tree is in the order of the layer tree, so we should really select the latest (upmost) frame containing the mouse pointer. That still does not solve the issue of overlapping with layers outside the scrolling tree... Comment on attachment 312321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312321&action=review > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:185 > + return this; More notes: - We should verify whether the localPosition is inside the frame rect before trying to check the children (otherwise e.g. on attachment 311465 [details] the blue rect is accessible from outside its parent frame). - The frame rect is not correct, see https://bugs.webkit.org/show_bug.cgi?id=172914#c8 Comment on attachment 312321 [details]
Patch
This is hit-testing in the wrong order (try overlapping frames). It needs to hit-test front-to-back, not back-to-front.
Created attachment 320274 [details]
Patch (WIP)
Created attachment 320548 [details]
Patch (WIP)
This new version addresses the points mentioned here, but I suspect I should add more tests after these changes.
Created attachment 320627 [details]
Patch (172914+172917)
Comment on attachment 320627 [details] Patch (172914+172917) Attachment 320627 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4532464 New failing tests: fast/scrolling/latching/fast-scroll-iframe-latched-mainframe.html tiled-drawing/scrolling/frames/coordinated-frame-gain-scrolling-ancestor.html Created attachment 320631 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 320652 [details]
Patch
Created attachment 320752 [details] Patch OK, it seems I had forgotten to rename the option "AsyncFrameScrollingEnabled" in tests. Now I realized that hit testing for overlapping elements has random pass/fail result. I believe this is due to the same issue I noticed in bug 172914: the order of iframe nodes in the layer tree is random (probably in the order they are loaded) so we can't rely on that order to determine which iframe is above and which one is below. Additionally, I added a test for axis-aligned transforms but it is currently failing (see bug 172914 comment 29). (In reply to Frédéric Wang (:fredw) from comment #15) > Created attachment 320752 [details] > Patch > > OK, it seems I had forgotten to rename the option > "AsyncFrameScrollingEnabled" in tests. Now I realized that hit testing for > overlapping elements has random pass/fail result. I believe this is due to > the same issue I noticed in bug 172914: the order of iframe nodes in the > layer tree is random (probably in the order they are loaded) so we can't > rely on that order to determine which iframe is above and which one is below. We need to fix that; they have to be in z-order (which matches compositing layer and RenderLayer z-index order). I guess when adding nodes to the tree we need to do ordered insertion. (In reply to Simon Fraser (smfr) from comment #16) > We need to fix that; they have to be in z-order (which matches compositing > layer and RenderLayer z-index order). I guess when adding nodes to the tree > we need to do ordered insertion. I opened bug 176914 for that. Created attachment 320919 [details]
Patch
Created attachment 355486 [details]
Patch
Rebasing...
Created attachment 355637 [details]
Patch for EWS (includes patch from 172914)
Comment on attachment 355637 [details] Patch for EWS (includes patch from 172914) Attachment 355637 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10154623 New failing tests: fast/scrolling/iframe-maximum-scroll-position.html fast/scrolling/iframe-hit-testing-axis-aligned-transforms.html Created attachment 355639 [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 on attachment 355637 [details] Patch for EWS (includes patch from 172914) Attachment 355637 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10154639 New failing tests: fast/scrolling/iframe-maximum-scroll-position.html fast/scrolling/iframe-hit-testing-axis-aligned-transforms.html Created attachment 355640 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 355637 [details] Patch for EWS (includes patch from 172914) Attachment 355637 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10154686 New failing tests: fast/scrolling/iframe-maximum-scroll-position.html fast/scrolling/iframe-hit-testing-axis-aligned-transforms.html Created attachment 355641 [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 355637 [details] Patch for EWS (includes patch from 172914) Attachment 355637 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10154791 New failing tests: fast/scrolling/iframe-maximum-scroll-position.html fast/scrolling/iframe-hit-testing-axis-aligned-transforms.html Created attachment 355642 [details]
Archive of layout-test-results from ews203 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews203 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 355986 [details] Temporary patch to set parentRelativeScrollableRect on scrolling nodes Extracted from the original patch for bug 172914. Created attachment 356005 [details] Patch Untested patch. This is slightly refactored in order to make easier to implement overscroll behavior (bug 176454) for asynchronous scrolling in the future. Comment on attachment 356005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356005&action=review > Source/WebCore/page/scrolling/ScrollingTree.cpp:109 > + target = target->parent(); So this is going to propagate scrolling in z-order tree order (when we have overflow scroll). handleWheelEventInAppropriateEnclosingBox() oddly uses containingBlock order, and normal events are in DOM order, so this is all really inconsistent. > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:178 > +ScrollingTreeScrollingNode* ScrollingTreeScrollingNode::scrollingTargetForWheelEvent(const PlatformWheelEvent& wheelEvent, LayoutPoint position) wheelEvent is not used by this function; why pass it? > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:186 > + LayoutPoint localPosition = position + -parentRelativeScrollableRect().location(); + -! > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:78 > + ScrollingTreeScrollingNode* scrollingTargetForWheelEvent(const PlatformWheelEvent&, LayoutPoint position); Maybe call this scrollingNodeForPoint() Comment on attachment 356005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356005&action=review >> Source/WebCore/page/scrolling/ScrollingTree.cpp:109 >> + target = target->parent(); > > So this is going to propagate scrolling in z-order tree order (when we have overflow scroll). handleWheelEventInAppropriateEnclosingBox() oddly uses containingBlock order, and normal events are in DOM order, so this is all really inconsistent. Not sure I understand the comment about z-order, I thought it is going to be containingBlock order. >> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:178 >> +ScrollingTreeScrollingNode* ScrollingTreeScrollingNode::scrollingTargetForWheelEvent(const PlatformWheelEvent& wheelEvent, LayoutPoint position) > > wheelEvent is not used by this function; why pass it? Right, this was used in previous patches and I forgot to removed it. >> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:186 >> + LayoutPoint localPosition = position + -parentRelativeScrollableRect().location(); > > + -! I need to check if the - operator is supported for LayoutPoint. >> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:78 >> + ScrollingTreeScrollingNode* scrollingTargetForWheelEvent(const PlatformWheelEvent&, LayoutPoint position); > > Maybe call this scrollingNodeForPoint() Yes, it makes sense now that I'm no longer using the wheel event here. Created attachment 356149 [details]
Patch
Addressing review comments and improving how the code is written (not tested yet).
Created attachment 356503 [details] Temporary patch to set parentRelativeScrollableRect on scrolling nodes (applies on top of bug 192358) Created attachment 358490 [details]
Temporary patch to set parentRelativeScrollableRect on scrolling nodes
Rebasing...
Created attachment 358491 [details]
Patch
Comment on attachment 358491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358491&action=review > LayoutTests/fast/scrolling/iframe-hit-testing-axis-aligned-transforms.html:29 > +function checkForInnerFrameScroll() { JS functions should have brace on new line. > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:183 > + for (auto iterator = children()->rbegin(), end = children()->rend(); iterator != end; iterator++) { Does this take into account scrolling offsets that happened since parentRelativeScrollableRect was computed? > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:188 > + if (is<ScrollingTreeScrollingNode>(**iterator)) { > + auto& scrollingNode = downcast<ScrollingTreeScrollingNode>(**iterator); > + if (scrollingNode.parentRelativeScrollableRect().contains(localPosition)) > + return scrollingNode.scrollingNodeForPoint(localPosition); > + } This seems like it will never hit scrolling nodes nested inside of fixed or sticky nodes. Comment on attachment 358491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358491&action=review Hi Simon. Thanks for the feedback. I'm wondering what's your plan exactly for the parentRelativeScrollableRect property? I understood that you wanted to do something while refactoring RenderLayerCompositor and the part I extracted (attachment 358490 [details]) to set parentRelativeScrollableRect was just a temporary solution until you're done with your work. Also, I expected that parentRelativeScrollableRect would just be properly updated when scrolling happens. Are these assumptions correct? >> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:183 >> + for (auto iterator = children()->rbegin(), end = children()->rend(); iterator != end; iterator++) { > > Does this take into account scrolling offsets that happened since parentRelativeScrollableRect was computed? I just tried with a basic iframe in the main view and indeed the updateScrollCoordinationForThisFrame is generally not called for the subframe when scrolling the main frame (exceptions are e.g. when the iframe goes out of the visible rect). So parentRelativeScrollableRect will not be updated for the subframe with attachment 358490 [details]. I understand that ideally update should have happened during scrolling or maybe you mean scrollingNodeForPoint should be aware of potential offset changes? >> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:188 >> + } > > This seems like it will never hit scrolling nodes nested inside of fixed or sticky nodes. Yes, this seems wrong indeed (I actually checked with a test case too). I guess we should have a generic implementation of scrollingNodeForPoint that just forwards the call to descendants. Created attachment 358703 [details]
Patch
This version handles a bit better fixed/sticky nodes but I think the other review comment really has to be addressed to make things properly work.
I've taken the patch here and have it mostly working (need to test overflow inside fixed etc). Fred, do you mind if I take the bug? (In reply to Simon Fraser (smfr) from comment #40) > I've taken the patch here and have it mostly working (need to test overflow > inside fixed etc). > > Fred, do you mind if I take the bug? No, feel free to take it. *** Bug 193880 has been marked as a duplicate of this bug. *** Adding the geometry info via bug 194002. I'll keep this for the hit testing. This is next on my list after bug 194002 lands. Created attachment 360651 [details]
Patch
Created attachment 360657 [details]
Patch
Created attachment 360668 [details]
Patch
Created attachment 360677 [details]
Patch
Comment on attachment 360677 [details] Patch Attachment 360677 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10963749 New failing tests: tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-vertical-then-horizontal.html Created attachment 360689 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 360677 [details]
Patch
This breaks rubber-banding.
Created attachment 360691 [details]
Patch
Comment on attachment 360691 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360691&action=review > Source/WebCore/ChangeLog:22 > + Nodes in the scrolling tree implement scrollingNodeForPoint() to implement hit testing. > + Two helper functions exist to simplify coordinate conversion: parentToLocalPoint() > + and localToContentsPoint(). Child nodes are hit-testing in reverse order to find nodes > + hightest in Z first. Only scrolling nodes are returned (not sure if we'll ever need > + to hit-test non-scrolling nodes). Nodes use parentRelativeScrollableRect and scroll positions > + to do these point mappings. It is still bit unclear to me why we want to do it like this. Wouldn't it be simpler and more robust to hit test in the layer tree, then find the corresponding ScrollingNode for the layer hit (several approaches for this)? Does that not work in some case? Still r+ because most of what this patch does is good. (In reply to Antti Koivisto from comment #53) > Comment on attachment 360691 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=360691&action=review > > > Source/WebCore/ChangeLog:22 > > + Nodes in the scrolling tree implement scrollingNodeForPoint() to implement hit testing. > > + Two helper functions exist to simplify coordinate conversion: parentToLocalPoint() > > + and localToContentsPoint(). Child nodes are hit-testing in reverse order to find nodes > > + hightest in Z first. Only scrolling nodes are returned (not sure if we'll ever need > > + to hit-test non-scrolling nodes). Nodes use parentRelativeScrollableRect and scroll positions > > + to do these point mappings. > > It is still bit unclear to me why we want to do it like this. Wouldn't it be > simpler and more robust to hit test in the layer tree, then find the > corresponding ScrollingNode for the layer hit (several approaches for this)? > Does that not work in some case? We have to hit-test on the scrolling thread on macOS. The GraphicsLayer tree is not thread-safe, and is not synchronized with commits (layout can nuke the graphics layer tree). CALayers don't have th right info for hit testing (there can be CALayers whose bounds don't reflect the right hit-testing areas, and some CALayers should be transparent to hit testing). > We have to hit-test on the scrolling thread on macOS. The GraphicsLayer tree
> is not thread-safe, and is not synchronized with commits (layout can nuke
> the graphics layer tree). CALayers don't have th right info for hit testing
> (there can be CALayers whose bounds don't reflect the right hit-testing
> areas, and some CALayers should be transparent to hit testing).
Scrolling thread :(
In case of UI side scrolling, it would be easy to include any amount of hit testing meta data along with the layers (in RemoteLayerTreeNodes).
Doesn't maintaining correct hit testing areas in ScrollingTree become complex in presence of transforms etc?
(In reply to Antti Koivisto from comment #57) > > We have to hit-test on the scrolling thread on macOS. The GraphicsLayer tree > > is not thread-safe, and is not synchronized with commits (layout can nuke > > the graphics layer tree). CALayers don't have th right info for hit testing > > (there can be CALayers whose bounds don't reflect the right hit-testing > > areas, and some CALayers should be transparent to hit testing). > > Scrolling thread :( > > In case of UI side scrolling, it would be easy to include any amount of hit > testing meta data along with the layers (in RemoteLayerTreeNodes). Right, and that's what I plan to do. For macOS with the scrolling thread, either I'll have to push "overlap" layers into the scrolling tree, or somehow compute overlapped regions for each scroller, which gets hard. > Doesn't maintaining correct hit testing areas in ScrollingTree become > complex in presence of transforms etc? yes. > > Doesn't maintaining correct hit testing areas in ScrollingTree become > > complex in presence of transforms etc? > > yes. @Simon: Is bug 173354 still valid? Should we close it or are you working on it? |