Bug 172917 - [Mac] Implement basic hit testing in the scrolling tree
Summary: [Mac] Implement basic hit testing in the scrolling tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 178180 172914 176914 192358 194002
Blocks: 171667 173354 194080
  Show dependency treegraph
 
Reported: 2017-06-05 04:52 PDT by Frédéric Wang (:fredw)
Modified: 2019-02-21 08:23 PST (History)
12 users (show)

See Also:


Attachments
Patch (3.50 KB, patch)
2017-06-05 05:05 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (22.72 KB, patch)
2017-06-08 10:55 PDT, Frédéric Wang (:fredw)
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (WIP) (24.53 KB, patch)
2017-09-08 10:09 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (WIP) (25.48 KB, patch)
2017-09-12 08:21 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (172914+172917) (65.78 KB, patch)
2017-09-13 02:08 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.61 MB, application/zip)
2017-09-13 03:25 PDT, Build Bot
no flags Details
Patch (31.05 KB, patch)
2017-09-13 10:05 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (37.24 KB, patch)
2017-09-14 02:42 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (37.24 KB, patch)
2017-09-15 09:59 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (37.19 KB, patch)
2018-11-22 12:52 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS (includes patch from 172914) (69.48 KB, patch)
2018-11-26 06:53 PST, Frédéric Wang (:fredw)
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.45 MB, application/zip)
2018-11-26 07:58 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-sierra-wk2 (3.05 MB, application/zip)
2018-11-26 08:09 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (2.19 MB, application/zip)
2018-11-26 08:48 PST, Build Bot
no flags Details
Archive of layout-test-results from ews203 for win-future (12.50 MB, application/zip)
2018-11-26 08:52 PST, Build Bot
no flags Details
Temporary patch to set parentRelativeScrollableRect on scrolling nodes (10.75 KB, patch)
2018-11-29 01:17 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (37.08 KB, patch)
2018-11-29 07:13 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (36.67 KB, patch)
2018-11-29 23:14 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Temporary patch to set parentRelativeScrollableRect on scrolling nodes (applies on top of bug 192358) (10.03 KB, patch)
2018-12-04 09:15 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Temporary patch to set parentRelativeScrollableRect on scrolling nodes (24.02 KB, patch)
2019-01-07 03:32 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (36.65 KB, patch)
2019-01-07 03:32 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (38.58 KB, patch)
2019-01-09 07:40 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (61.53 KB, patch)
2019-01-30 17:06 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (63.06 KB, patch)
2019-01-30 17:38 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (63.06 KB, patch)
2019-01-30 18:42 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (65.92 KB, patch)
2019-01-30 19:28 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.69 MB, application/zip)
2019-01-30 21:11 PST, Build Bot
no flags Details
Patch (65.94 KB, patch)
2019-01-30 22:15 PST, Simon Fraser (smfr)
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2017-06-05 04:52:58 PDT
After bug 172914, we can implement basic hit testing (which does not take into account overlapping or transformations for now).
Comment 1 Frédéric Wang (:fredw) 2017-06-05 05:05:48 PDT Comment hidden (obsolete)
Comment 2 Frédéric Wang (:fredw) 2017-06-08 10:55:20 PDT Comment hidden (obsolete)
Comment 3 Alexey Proskuryakov 2017-06-09 22:42:16 PDT Comment hidden (obsolete)
Comment 4 Frédéric Wang (:fredw) 2017-06-09 22:46:34 PDT Comment hidden (obsolete)
Comment 5 Frédéric Wang (:fredw) 2017-06-15 07:59:59 PDT
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 6 Frédéric Wang (:fredw) 2017-06-20 08:19:12 PDT
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 7 Radar WebKit Bug Importer 2017-09-01 14:22:49 PDT
<rdar://problem/34215516>
Comment 8 Simon Fraser (smfr) 2017-09-05 08:47:03 PDT Comment hidden (obsolete)
Comment 9 Frédéric Wang (:fredw) 2017-09-08 10:09:06 PDT Comment hidden (obsolete)
Comment 10 Frédéric Wang (:fredw) 2017-09-12 08:21:13 PDT Comment hidden (obsolete)
Comment 11 Frédéric Wang (:fredw) 2017-09-13 02:08:06 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-09-13 03:25:29 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-09-13 03:25:30 PDT Comment hidden (obsolete)
Comment 14 Frédéric Wang (:fredw) 2017-09-13 10:05:08 PDT Comment hidden (obsolete)
Comment 15 Frédéric Wang (:fredw) 2017-09-14 02:42:57 PDT
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).
Comment 16 Simon Fraser (smfr) 2017-09-14 08:21:53 PDT
(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.
Comment 17 Frédéric Wang (:fredw) 2017-09-14 09:22:27 PDT
(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.
Comment 18 Frédéric Wang (:fredw) 2017-09-15 09:59:19 PDT Comment hidden (obsolete)
Comment 19 Frédéric Wang (:fredw) 2018-11-22 12:52:02 PST Comment hidden (obsolete)
Comment 20 Frédéric Wang (:fredw) 2018-11-26 06:53:56 PST Comment hidden (obsolete)
Comment 21 Build Bot 2018-11-26 07:58:17 PST Comment hidden (obsolete)
Comment 22 Build Bot 2018-11-26 07:58:19 PST Comment hidden (obsolete)
Comment 23 Build Bot 2018-11-26 08:09:21 PST Comment hidden (obsolete)
Comment 24 Build Bot 2018-11-26 08:09:24 PST Comment hidden (obsolete)
Comment 25 Build Bot 2018-11-26 08:48:35 PST Comment hidden (obsolete)
Comment 26 Build Bot 2018-11-26 08:48:37 PST Comment hidden (obsolete)
Comment 27 Build Bot 2018-11-26 08:52:10 PST Comment hidden (obsolete)
Comment 28 Build Bot 2018-11-26 08:52:21 PST Comment hidden (obsolete)
Comment 29 Frédéric Wang (:fredw) 2018-11-29 01:17:13 PST Comment hidden (obsolete)
Comment 30 Frédéric Wang (:fredw) 2018-11-29 07:13:48 PST Comment hidden (obsolete)
Comment 31 Simon Fraser (smfr) 2018-11-29 10:44:14 PST
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 32 Frédéric Wang (:fredw) 2018-11-29 11:10:21 PST
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.
Comment 33 Frédéric Wang (:fredw) 2018-11-29 23:14:36 PST
Created attachment 356149 [details]
Patch

Addressing review comments and improving how the code is written (not tested yet).
Comment 34 Frédéric Wang (:fredw) 2018-12-04 09:15:29 PST Comment hidden (obsolete)
Comment 35 Frédéric Wang (:fredw) 2019-01-07 03:32:17 PST Comment hidden (obsolete)
Comment 36 Frédéric Wang (:fredw) 2019-01-07 03:32:50 PST
Created attachment 358491 [details]
Patch
Comment 37 Simon Fraser (smfr) 2019-01-07 15:55:03 PST
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 38 Frédéric Wang (:fredw) 2019-01-09 02:39:48 PST
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.
Comment 39 Frédéric Wang (:fredw) 2019-01-09 07:40:22 PST Comment hidden (obsolete)
Comment 40 Simon Fraser (smfr) 2019-01-19 21:15:59 PST
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?
Comment 41 Frédéric Wang (:fredw) 2019-01-19 21:26:08 PST
(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.
Comment 42 Simon Fraser (smfr) 2019-01-29 20:26:39 PST
*** Bug 193880 has been marked as a duplicate of this bug. ***
Comment 43 Simon Fraser (smfr) 2019-01-29 20:28:50 PST
Adding the geometry info via bug 194002. I'll keep this for the hit testing.
Comment 44 Simon Fraser (smfr) 2019-01-29 20:42:29 PST
This is next on my list after bug 194002 lands.
Comment 45 Simon Fraser (smfr) 2019-01-30 17:06:39 PST Comment hidden (obsolete)
Comment 46 Simon Fraser (smfr) 2019-01-30 17:38:03 PST Comment hidden (obsolete)
Comment 47 Simon Fraser (smfr) 2019-01-30 18:42:28 PST Comment hidden (obsolete)
Comment 48 Simon Fraser (smfr) 2019-01-30 19:28:29 PST Comment hidden (obsolete)
Comment 49 Build Bot 2019-01-30 21:11:09 PST Comment hidden (obsolete)
Comment 50 Build Bot 2019-01-30 21:11:11 PST Comment hidden (obsolete)
Comment 51 Simon Fraser (smfr) 2019-01-30 21:59:35 PST
Comment on attachment 360677 [details]
Patch

This breaks rubber-banding.
Comment 52 Simon Fraser (smfr) 2019-01-30 22:15:13 PST
Created attachment 360691 [details]
Patch
Comment 53 Antti Koivisto 2019-01-30 23:48:41 PST
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?
Comment 54 Antti Koivisto 2019-01-31 00:17:18 PST
Still r+ because most of what this patch does is good.
Comment 55 Simon Fraser (smfr) 2019-01-31 08:01:41 PST
(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).
Comment 56 Simon Fraser (smfr) 2019-01-31 08:30:40 PST
https://trac.webkit.org/r240787
Comment 57 Antti Koivisto 2019-01-31 08:40:53 PST
> 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?
Comment 58 Simon Fraser (smfr) 2019-01-31 10:51:42 PST
(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.
Comment 59 Frédéric Wang (:fredw) 2019-02-21 08:23:34 PST
> > 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?