Bug 195371 - Compositing layer that renders two positioned elements should not hit test
Summary: Compositing layer that renders two positioned elements should not hit test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antti Koivisto
URL:
Keywords: InRadar
: 195370 195372 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-03-06 13:17 PST by Simon Fraser (smfr)
Modified: 2019-03-12 15:36 PDT (History)
6 users (show)

See Also:


Attachments
Testcase (1.25 KB, text/html)
2019-03-06 13:17 PST, Simon Fraser (smfr)
no flags Details
wip (24.52 KB, patch)
2019-03-08 09:55 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (36.15 KB, patch)
2019-03-11 06:14 PDT, Antti Koivisto
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (2.41 MB, application/zip)
2019-03-11 08:02 PDT, EWS Watchlist
no flags Details
patch (36.61 KB, patch)
2019-03-11 08:52 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (36.64 KB, patch)
2019-03-11 09:15 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (36.25 KB, patch)
2019-03-11 09:18 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (36.25 KB, patch)
2019-03-11 09:23 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (36.12 KB, patch)
2019-03-11 09:30 PDT, Antti Koivisto
simon.fraser: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (2.29 MB, application/zip)
2019-03-11 11:30 PDT, EWS Watchlist
no flags Details
patch (36.55 KB, patch)
2019-03-12 02:24 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (37.33 KB, patch)
2019-03-12 06:55 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
followup (2.90 KB, patch)
2019-03-12 14:59 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
followup to fix the test (2.90 KB, patch)
2019-03-12 15:04 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-03-06 13:17:54 PST
Created attachment 363780 [details]
Testcase

If the layer overlapping a scroller has two disjoint RenderLayers, it should not hit test in its entirety.
Comment 1 Radar WebKit Bug Importer 2019-03-06 13:18:23 PST
<rdar://problem/48649586>
Comment 2 Antti Koivisto 2019-03-08 09:55:04 PST
Created attachment 364024 [details]
wip
Comment 3 Simon Fraser (smfr) 2019-03-08 10:10:58 PST
Comment on attachment 364024 [details]
wip

View in context: https://bugs.webkit.org/attachment.cgi?id=364024&action=review

> Source/WebCore/platform/graphics/GraphicsLayer.h:451
> +    virtual void setEventRegion(std::unique_ptr<Region>) { }

std::unique_ptr<Region>&&

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:975
> +    if (region == m_eventRegion)
> +        return;
> +    if (region && m_eventRegion && *region == *m_eventRegion)
> +        return;

Use arePointingToEqualData()?

> Source/WebCore/rendering/RenderLayerBacking.cpp:2591
> +    m_graphicsLayer->setEventRegion(WTFMove(eventRegion));

Doing this in painting won't get you any regions for layers that don't paint (but still need to hit-test), like those with just color backgrounds (bug 195378).

An alternative would be to do this as a RenderLayer tree walk. The relevant renderer hit-testing code is in Render*::nodeAtPoint(). I wonder if we can make a parallel method to every nodeAtPoint(), then call into it from a RenderLayer tree walk which is like what RenderLayer::hitTest/hitTestContents() do? That would be instead of using a paint phase, but it would make the region building code more similar to the hit-testing code. A longer term solution would be to change the hit-testing path to be iterator-like, with options to either do a hit-test, or to gather the region.

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeNode.mm:69
> +    [layer() setValue:nil forKey:WKRemoteLayerTreeNodePropertyKey];

These setValue:forKey: do show up in profiles; it's not very fast.

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeNode.mm:96
> +        fprintf(stderr, "RemoteLayerTreeNode::setEventRegion\n");
> +        for (auto& rect : m_eventRegion->rects())
> +            fprintf(stderr, "rect (%d %d %d %d)\n", rect.x(), rect.y(), rect.width(), rect.height());

Region::dump()

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeNode.mm:103
>      [layer() setValue:@(layerID) forKey:WKLayerIDPropertyKey];
> +    [layer() setValue:[NSValue valueWithPointer:this] forKey:WKRemoteLayerTreeNodePropertyKey];

Rather than a second setValue:forKey:, can we instead just get to the remoteLayerTreeNode with ID via the RemoteLayerTreeContext?
Comment 4 Antti Koivisto 2019-03-11 05:03:07 PDT
> An alternative would be to do this as a RenderLayer tree walk. The relevant
> renderer hit-testing code is in Render*::nodeAtPoint(). I wonder if we can
> make a parallel method to every nodeAtPoint(), then call into it from a
> RenderLayer tree walk which is like what
> RenderLayer::hitTest/hitTestContents() do? That would be instead of using a
> paint phase, but it would make the region building code more similar to the
> hit-testing code. A longer term solution would be to change the hit-testing
> path to be iterator-like, with options to either do a hit-test, or to gather
> the region.

I don't really want to add another hittest/paint-like tree-walk code path. Lets use a paint phase for now, it is not complex and can be easily factored into something else later.

> Rather than a second setValue:forKey:, can we instead just get to the
> remoteLayerTreeNode with ID via the RemoteLayerTreeContext?

I moved the id to the RemoteLayerTreeNode too, eliminating the need for two separate key values. We could further improve this with custom layers that contain the back pointers as a regular members (most of the layers are under our control).
Comment 5 Antti Koivisto 2019-03-11 06:14:44 PDT
Created attachment 364249 [details]
patch
Comment 6 EWS Watchlist 2019-03-11 08:02:41 PDT
Comment on attachment 364249 [details]
patch

Attachment 364249 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11455439

New failing tests:
displaylists/extent-includes-transforms.html
Comment 7 EWS Watchlist 2019-03-11 08:02:42 PDT
Created attachment 364252 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 8 Antti Koivisto 2019-03-11 08:52:16 PDT
Created attachment 364256 [details]
patch
Comment 9 Antti Koivisto 2019-03-11 09:15:05 PDT
Created attachment 364258 [details]
patch
Comment 10 Antti Koivisto 2019-03-11 09:18:04 PDT
Created attachment 364260 [details]
patch
Comment 11 Antti Koivisto 2019-03-11 09:23:53 PDT
Created attachment 364261 [details]
patch
Comment 12 Antti Koivisto 2019-03-11 09:30:02 PDT
Created attachment 364262 [details]
patch
Comment 13 Simon Fraser (smfr) 2019-03-11 11:04:24 PDT
Comment on attachment 364262 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364262&action=review

> Source/WebCore/rendering/RenderLayerBacking.cpp:2587
> +    if (eventRegion->contains(roundedIntRect(compositedBounds())))
> +        eventRegion = nullptr;

When does this happen? Is this the case for a simple layer which is fully hit-testable?

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp:884
> +    m_properties.eventRegion = std::make_unique<WebCore::Region>(*eventRegion);

Is it worth comparing regions? Not doing so will trigger region encoding on every paint, right?
Comment 14 EWS Watchlist 2019-03-11 11:30:09 PDT
Comment on attachment 364262 [details]
patch

Attachment 364262 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11457257

New failing tests:
displaylists/extent-includes-transforms.html
Comment 15 EWS Watchlist 2019-03-11 11:30:10 PDT
Created attachment 364273 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 16 Antti Koivisto 2019-03-11 12:23:48 PDT
> When does this happen? Is this the case for a simple layer which is fully
> hit-testable?

Yeah. Even trivial Region object is large so I don't want to keep it around in that case. We should probably optimize Region so that it does not allocate main data structure until something that can't be described by a single rect is added.

> > Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp:884
> > +    m_properties.eventRegion = std::make_unique<WebCore::Region>(*eventRegion);
> 
> Is it worth comparing regions? Not doing so will trigger region encoding on
> every paint, right?

Maybe, though complex cases are probably rare. I'll see about it.
Comment 17 Simon Fraser (smfr) 2019-03-11 13:56:08 PDT
(In reply to Antti Koivisto from comment #16)
> > When does this happen? Is this the case for a simple layer which is fully
> > hit-testable?
> 
> Yeah. Even trivial Region object is large so I don't want to keep it around
> in that case. We should probably optimize Region so that it does not
> allocate main data structure until something that can't be described by a
> single rect is added.

It's a bit confusing that a null region in the UI process means "entire layer is the event region"; I have to think about whether the whole layer is clickable or not clickable. Some region-or-rect thing would be good here.

> > > Source/WebKit/WebProcess/WebPage/RemoteLayerTree/PlatformCALayerRemote.cpp:884
> > > +    m_properties.eventRegion = std::make_unique<WebCore::Region>(*eventRegion);
> > 
> > Is it worth comparing regions? Not doing so will trigger region encoding on
> > every paint, right?
> 
> Maybe, though complex cases are probably rare. I'll see about it.
Comment 18 Antti Koivisto 2019-03-12 01:56:10 PDT
> Some region-or-rect thing would be good here.

I think Region can be the region-or-rect thing with some optimisation. It almost is already.
Comment 19 Antti Koivisto 2019-03-12 02:24:54 PDT
Created attachment 364370 [details]
patch
Comment 20 Antti Koivisto 2019-03-12 02:25:41 PDT
*** Bug 195372 has been marked as a duplicate of this bug. ***
Comment 21 Antti Koivisto 2019-03-12 02:26:24 PDT
*** Bug 195370 has been marked as a duplicate of this bug. ***
Comment 22 Antti Koivisto 2019-03-12 06:55:07 PDT
Created attachment 364385 [details]
patch
Comment 23 WebKit Commit Bot 2019-03-12 07:44:37 PDT
The commit-queue encountered the following flaky tests while processing attachment 364385 [details]:

legacy-animation-engine/imported/blink/animations/animation-events-prefixed-01.html bug 195621 (authors: dino@apple.com and graouts@apple.com)
The commit-queue is continuing to process your patch.
Comment 24 WebKit Commit Bot 2019-03-12 07:45:38 PDT
Comment on attachment 364385 [details]
patch

Clearing flags on attachment: 364385

Committed r242794: <https://trac.webkit.org/changeset/242794>
Comment 25 WebKit Commit Bot 2019-03-12 07:45:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Ryan Haddad 2019-03-12 13:36:30 PDT
fast/scrolling/ios/overflow-scroll-overlap-2.html (added with this change) is consistently failing on iOS Simulator:

--- /Volumes/Data/slave/ios-simulator-12-debug-tests-wk2/build/layout-test-results/fast/scrolling/ios/overflow-scroll-overlap-2-expected.txt
+++ /Volumes/Data/slave/ios-simulator-12-debug-tests-wk2/build/layout-test-results/fast/scrolling/ios/overflow-scroll-overlap-2-actual.txt
@@ -1,9 +1,9 @@
 Test that scrollable areas with non-trivial overlap are correctly targeted.
 
 case 1: 
-case 2: Scrollable 2 
-case 3: Scrollable 3 
-case 4: Scrollable 4 
-case 5: Scrollable 5 
+case 2: 
+case 3: 
+case 4: 
+case 5: 
 case 6: 
 
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=fast%2Fscrolling%2Fios%2Foverflow-scroll-overlap-2.html
Comment 27 Antti Koivisto 2019-03-12 14:12:55 PDT
Strange. Looking.
Comment 28 Shawn Roberts 2019-03-12 14:49:49 PDT
This is also causing a layout test image diff failure on Mac WK2

Reproduced locally with :

run-webkit-tests animations/stacking-context-not-fill-forwards.html --iterations 500 -f --no-retry

Failing ~11 times per 500 iteration run.
Passes 1500 iterations in versions prior to 242794

Flakiness Dashboard :

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=animations%2Fstacking-context-not-fill-forwards.html

Diff:

https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r242797%20(3024)/animations/stacking-context-not-fill-forwards-diffs.html
Comment 29 Antti Koivisto 2019-03-12 14:59:59 PDT
Created attachment 364452 [details]
followup
Comment 30 Antti Koivisto 2019-03-12 15:04:00 PDT
Created attachment 364456 [details]
followup to fix the test
Comment 31 Antti Koivisto 2019-03-12 15:36:21 PDT
Follow-up in https://trac.webkit.org/r242830