Bug 172917 – [Mac] Implement basic hit testing in the scrolling tree

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

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

After bug 172914, we can implement basic hit testing (which does not take into account overlapping or transformations for now).

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

Created attachment 312007 [details]
Patch

Extracted from bug 171667

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

Created attachment 312321 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 312321 [details]
Patch

EWS is red.

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

Comment on attachment 312321 [details]
Patch

Indeed, this applies on top of bug 172914.

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

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

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

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

[Radar WebKit Bug Importer]

<rdar://problem/34215516>

[Simon Fraser (smfr)]

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.

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

Created attachment 320274 [details]
Patch (WIP)

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

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.

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

Created attachment 320627 [details]
Patch (172914+172917)

[Build Bot]

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

[Build Bot]

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

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

Created attachment 320652 [details]
Patch

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

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

[Simon Fraser (smfr)]

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

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

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

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

Created attachment 320919 [details]
Patch

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

Created attachment 355486 [details]
Patch

Rebasing...

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

Created attachment 355637 [details]
Patch for EWS (includes patch from 172914)

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

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

Created attachment 355986 [details]
Temporary patch to set parentRelativeScrollableRect on scrolling nodes

Extracted from the original patch for bug 172914.

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

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.

[Simon Fraser (smfr)]

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

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

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.

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

Created attachment 356149 [details]
Patch

Addressing review comments and improving how the code is written (not tested yet).

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

Created attachment 356503 [details]
Temporary patch to set parentRelativeScrollableRect on scrolling nodes (applies on top of bug 192358)

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

Created attachment 358490 [details]
Temporary patch to set parentRelativeScrollableRect on scrolling nodes

Rebasing...

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

Created attachment 358491 [details]
Patch

[Simon Fraser (smfr)]

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.

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

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.

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

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.

[Simon Fraser (smfr)]

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?

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

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

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

Adding the geometry info via bug 194002. I'll keep this for the hit testing.

[Simon Fraser (smfr)]

This is next on my list after bug 194002 lands.

[Simon Fraser (smfr)]

Created attachment 360651 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 360657 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 360668 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 360677 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Simon Fraser (smfr)]

Comment on attachment 360677 [details]
Patch

This breaks rubber-banding.

[Simon Fraser (smfr)]

Created attachment 360691 [details]
Patch

[Antti Koivisto]

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?

[Antti Koivisto]

Still r+ because most of what this patch does is good.

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

https://trac.webkit.org/r240787

[Antti Koivisto]

> 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?

[Simon Fraser (smfr)]

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

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

> > 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?