Bug 172917 - [Mac] Implement basic hit testing for frames
Summary: [Mac] Implement basic hit testing for frames
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 178180 172914 176914 192358
Blocks: 171667 173354
  Show dependency treegraph
 
Reported: 2017-06-05 04:52 PDT by Frédéric Wang (:fredw)
Modified: 2018-12-04 09:16 PST (History)
11 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

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

Extracted from bug 171667
Comment 2 Frédéric Wang (:fredw) 2017-06-08 10:55:20 PDT
Created attachment 312321 [details]
Patch
Comment 3 Alexey Proskuryakov 2017-06-09 22:42:16 PDT
Comment on attachment 312321 [details]
Patch

EWS is red.
Comment 4 Frédéric Wang (:fredw) 2017-06-09 22:46:34 PDT
Comment on attachment 312321 [details]
Patch

Indeed, this applies on top of bug 172914.
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 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.
Comment 9 Frédéric Wang (:fredw) 2017-09-08 10:09:06 PDT
Created attachment 320274 [details]
Patch (WIP)
Comment 10 Frédéric Wang (:fredw) 2017-09-12 08:21:13 PDT
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.
Comment 11 Frédéric Wang (:fredw) 2017-09-13 02:08:06 PDT
Created attachment 320627 [details]
Patch (172914+172917)
Comment 12 Build Bot 2017-09-13 03:25:29 PDT
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
Comment 13 Build Bot 2017-09-13 03:25:30 PDT
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
Comment 14 Frédéric Wang (:fredw) 2017-09-13 10:05:08 PDT
Created attachment 320652 [details]
Patch
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
Created attachment 320919 [details]
Patch
Comment 19 Frédéric Wang (:fredw) 2018-11-22 12:52:02 PST
Created attachment 355486 [details]
Patch

Rebasing...
Comment 20 Frédéric Wang (:fredw) 2018-11-26 06:53:56 PST
Created attachment 355637 [details]
Patch for EWS (includes patch from 172914)
Comment 21 Build Bot 2018-11-26 07:58:17 PST
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
Comment 22 Build Bot 2018-11-26 07:58:19 PST
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 23 Build Bot 2018-11-26 08:09:21 PST
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
Comment 24 Build Bot 2018-11-26 08:09:24 PST
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 25 Build Bot 2018-11-26 08:48:35 PST
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
Comment 26 Build Bot 2018-11-26 08:48:37 PST
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 27 Build Bot 2018-11-26 08:52:10 PST
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
Comment 28 Build Bot 2018-11-26 08:52:21 PST
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
Comment 29 Frédéric Wang (:fredw) 2018-11-29 01:17:13 PST
Created attachment 355986 [details]
Temporary patch to set parentRelativeScrollableRect on scrolling nodes

Extracted from the original patch for bug 172914.
Comment 30 Frédéric Wang (:fredw) 2018-11-29 07:13:48 PST
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 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
Created attachment 356503 [details]
Temporary patch to set parentRelativeScrollableRect on scrolling nodes (applies on top of bug 192358)