Bug 195371 – Compositing layer that renders two positioned elements should not hit test

• Component: Scrolling
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Created attachment 363780 [details]
Testcase

If the layer overlapping a scroller has two disjoint RenderLayers, it should not hit test in its entirety.

[Radar WebKit Bug Importer]

<rdar://problem/48649586>

[Antti Koivisto]

Created attachment 364024 [details]
wip

[Simon Fraser (smfr)]

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?

[Antti Koivisto]

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

[Antti Koivisto]

Created attachment 364249 [details]
patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antti Koivisto]

Created attachment 364256 [details]
patch

[Antti Koivisto]

Created attachment 364258 [details]
patch

[Antti Koivisto]

Created attachment 364260 [details]
patch

[Antti Koivisto]

Created attachment 364261 [details]
patch

[Antti Koivisto]

Created attachment 364262 [details]
patch

[Simon Fraser (smfr)]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antti Koivisto]

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

[Simon Fraser (smfr)]

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

[Antti Koivisto]

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

[Antti Koivisto]

Created attachment 364370 [details]
patch

[Antti Koivisto]

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

[Antti Koivisto]

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

[Antti Koivisto]

Created attachment 364385 [details]
patch

[WebKit Commit Bot]

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: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 364385 [details]
patch

Clearing flags on attachment: 364385

Committed r242794: <https://trac.webkit.org/changeset/242794>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Antti Koivisto]

Strange. Looking.

[Shawn Roberts]

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

[Antti Koivisto]

Created attachment 364452 [details]
followup

[Antti Koivisto]

Created attachment 364456 [details]
followup to fix the test

[Antti Koivisto]

Follow-up in https://trac.webkit.org/r242830