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: 172914 176914 178180
Blocks: 171667 173354
  Show dependency treegraph
 
Reported: 2017-06-05 04:52 PDT by Frédéric Wang (:fredw)
Modified: 2018-05-10 01:07 PDT (History)
10 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

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