Bug 210041

<rdar://problem/61323558>

Hack patch:

[[
diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
index 3a038ce8a3a..0a757275505 100644
--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
@@ -738,7 +738,20 @@ bool RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType update
         return false;
     }
 
-    if (!m_compositing && (m_forceCompositingMode || (isMainFrameCompositor() && page().pageOverlayController().overlayCount())))
+    auto needsCompositingToUpdateEventRegion = [&] {
+        auto& document = m_renderView.document();
+#if PLATFORM(IOS_FAMILY)
+        if (document.mayHaveElementsWithNonAutoTouchAction())
+            return true;
+#endif
+#if ENABLE(EDITABLE_REGION)
+        if (document.mayHaveEditableElements())
+            return true;
+#endif
+        return false;
+    };
+
+    if (!m_compositing && (m_forceCompositingMode || needsCompositingToUpdateEventRegion() || (isMainFrameCompositor() && page().pageOverlayController().overlayCount())))
         enableCompositingMode(true);
 
     bool isPageScroll = !updateRoot || updateRoot == &rootRenderLayer();
]]

The event-region paint doesn't propagate into the iframe.

Also, we don't have an invalidation code path for touch-action changes inside non-composited (potentially nested) iframes.

Created attachment 395713 [details]
First attempt

Comment on attachment 395713 [details]
First attempt

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

> Source/WebCore/rendering/RenderWidget.cpp:300
> +    // FIXME: Ensure layout performed if need to update event region.
> +    if (paintInfo.phase == PaintPhase::EventRegion && is<FrameView>(m_widget) && downcast<FrameView>(*m_widget).needsLayout())

I got to get rid of this...

Comment on attachment 395713 [details]
First attempt

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

>> Source/WebCore/rendering/RenderWidget.cpp:300
>> +    if (paintInfo.phase == PaintPhase::EventRegion && is<FrameView>(m_widget) && downcast<FrameView>(*m_widget).needsLayout())
> 
> I got to get rid of this...

Yeah, this is not needed.

Created attachment 395714 [details]
First attempt

Comment on attachment 395714 [details]
First attempt

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

> Source/WebCore/page/FrameView.cpp:1262
> +    auto& document = *frame().document();

RefPtr<>? Who knows what cache->postNotification() does.

> Source/WebCore/page/FrameView.cpp:1263
> +    if (!renderView()->isComposited()) {

This renderView()->isComposited() implies knowledge about how event regions are generated, which isn't great. It would be better to delegate this to the compositor() perhaps. It also warrants a comment.

> Source/WebCore/rendering/RenderBlock.cpp:1258
> +        bool needsTraverseDescendants = hasVisualOverflow() || containsFloats() || !paintInfo.eventRegionContext->contains(enclosingIntRect(borderRect)) || view().needsEventRegionUpdateForNonCompositedDescendant();

That's unfortunate and awkward. Not sure of a better way.

> Source/WebCore/rendering/RenderLayer.cpp:6447
> +void RenderLayer::eventRegionsChanged()

Why do we need both this and invalidateEventRegion()?

> Source/WebCore/rendering/RenderLayer.cpp:6452
> +        compositingLayer->renderer().view().setNeedsEventRegionUpdateForNonCompositedDescendant();

This part still bothers me.

> Source/WebCore/rendering/RenderLayer.cpp:6453
> +        compositingLayer->renderer().view().setNeedsRepaintAfterCompositingLayerUpdate();

I don't know why we'd need this.

> Source/WebCore/rendering/RenderLayer.cpp:-7006
> -    auto maintainsEventRegion = [&] {
> -        // UI side scroll overlap testing.
> -        if (!compositingLayer->isRenderViewLayer())
> -            return true;
> -        // UI side touch-action resolution.
> -        if (renderer().document().mayHaveElementsWithNonAutoTouchAction())
> -            return true;
> -        if (renderer().document().mayHaveEditableElements())
> -            return true;
> -        return false;
> -    };

Why remove this logic?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1621
> +    bool needsEventRegionUpdateForNonCompositedDescendant = renderer().view().needsEventRegionUpdateForNonCompositedDescendant();

The "descendant" here needs to be explicit that it's about Frames.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:862
> +        m_renderView.repaintRootContents();

Just to trigger event region generation? That's a huge perf hit.

> Source/WebCore/rendering/RenderWidget.cpp:252
> +        TransformationMatrix transform;

AffineTransform

Comment on attachment 395714 [details]
First attempt

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

Thank for the comments!

>> Source/WebCore/page/FrameView.cpp:1262
>> +    auto& document = *frame().document();
> 
> RefPtr<>? Who knows what cache->postNotification() does.

OK.

>> Source/WebCore/rendering/RenderLayer.cpp:6447
>> +void RenderLayer::eventRegionsChanged()
> 
> Why do we need both this and invalidateEventRegion()?

Yeah, I'm going to push all this into invalidateEventRegion() conditioned on a new enumeration argument due to needing setNeedsEventRegionUpdateForNonCompositedDescendant() and setNeedsRepaintAfterCompositingLayerUpdate().

>> Source/WebCore/rendering/RenderLayer.cpp:6452
>> +        compositingLayer->renderer().view().setNeedsEventRegionUpdateForNonCompositedDescendant();
> 
> This part still bothers me.

Need more info on how to proceed here.

>> Source/WebCore/rendering/RenderLayer.cpp:6453
>> +        compositingLayer->renderer().view().setNeedsRepaintAfterCompositingLayerUpdate();
> 
> I don't know why we'd need this.

Without this then debug overlays are not painted until something triggers a paint.

>> Source/WebCore/rendering/RenderLayer.cpp:-7006
>> -    };
> 
> Why remove this logic?

May have jumped the gun.... will revert and patch up to consult renderer().view().needsEventRegionUpdateForNonCompositedDescendant()()

>> Source/WebCore/rendering/RenderLayerBacking.cpp:1621
>> +    bool needsEventRegionUpdateForNonCompositedDescendant = renderer().view().needsEventRegionUpdateForNonCompositedDescendant();
> 
> The "descendant" here needs to be explicit that it's about Frames.

okiedokkie

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:862
>> +        m_renderView.repaintRootContents();
> 
> Just to trigger event region generation? That's a huge perf hit.

No, not for event generation. Just to paint the debug paint overlays.

>> Source/WebCore/rendering/RenderWidget.cpp:252
>> +        TransformationMatrix transform;
> 
> AffineTransform

yep, ahead of you here.

Comment on attachment 395714 [details]
First attempt

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

>>> Source/WebCore/rendering/RenderLayer.cpp:6453
>>> +        compositingLayer->renderer().view().setNeedsRepaintAfterCompositingLayerUpdate();
>> 
>> I don't know why we'd need this.
> 
> Without this then debug overlays are not painted until something triggers a paint.

Could condition this on the debug overlay setting being enabled.

Created attachment 395772 [details]
Second attempt

Created attachment 395774 [details]
For the bots

Created attachment 395776 [details]
For the bots

Created attachment 395778 [details]
For the bots

Created attachment 395779 [details]
For the bots

Created attachment 395781 [details]
For the bots

Created attachment 395815 [details]
Patch and tests

Comment on attachment 395815 [details]
Patch and tests

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

> Source/WebCore/dom/Document.cpp:4237
> +void Document::invalidateEventRegionsForIFrame(HTMLFrameOwnerElement& element)

This might be a Frame too so call it invalidateEventRegionsForFrame()

> Source/WebCore/dom/Document.cpp:4247
> +    if (auto* ownerElement = this->ownerElement())
> +        ownerElement->document().invalidateEventRegionsForIFrame(*ownerElement);

You should always find a layer above (the RenderVIew always has one) so I don't think you need this clause, and it takes brainspace.

> Source/WebCore/page/Frame.cpp:309
> +#if PLATFORM(IOS)

We don't like platform #ifdefs.

Also async overflow scrolling on macOS uses event regions so this is wrong.

> Source/WebCore/page/Frame.cpp:314
> +    if (!m_doc->renderView()->compositor().viewNeedsToInvalidateEventRegionOfEnclosingCompositingLayerForRepaint())

Should probably null-check m_doc->renderView().

> Source/WebCore/page/Frame.cpp:317
> +    if (m_ownerElement)
> +        m_ownerElement->document().invalidateEventRegionsForIFrame(*m_ownerElement);

Actually why doesn't compositor() just do this part.

> Source/WebCore/page/FrameView.cpp:1265
> +    frame().invalidateContentEventRegionsIfNeeded();

Please move this down next to invalidateRenderingDependentRegions() because they are related.

> Source/WebCore/rendering/RenderLayer.cpp:7011
>  #if PLATFORM(IOS_FAMILY)

Possible a wrong #ifdef.

> Source/WebCore/rendering/RenderLayer.cpp:7032
> +        compositingLayer->renderer().view().setNeedsEventRegionUpdateForNonCompositedIFrame();

compositingLayer->renderer().view() is always just renderer().view(); fetch it once.

> Source/WebCore/rendering/RenderLayer.cpp:7035
> +        compositingLayer->compositor().scheduleCompositingLayerUpdate();

This can be view->compositior()

> Source/WebCore/rendering/RenderLayer.h:928
> +    bool invalidateEventRegion(EventRegionInvalidationReason);

What does the return value mean? Add a comment.

Comment on attachment 395815 [details]
Patch and tests

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

>> Source/WebCore/dom/Document.cpp:4247
>> +        ownerElement->document().invalidateEventRegionsForIFrame(*ownerElement);
> 
> You should always find a layer above (the RenderVIew always has one) so I don't think you need this clause, and it takes brainspace.

I don't understand. No need to recurse on ownerElement? If so, then there is a bug in enclosingCompositingLayerForRepaint() because it can return null for a the middle iframe in pointerevents/ios/touch-action-none-inside-nested-iframe.html. If not, then you mean no need for owner element null check?

Comment on attachment 395815 [details]
Patch and tests

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

>> Source/WebCore/dom/Document.cpp:4237
>> +void Document::invalidateEventRegionsForIFrame(HTMLFrameOwnerElement& element)
> 
> This might be a Frame too so call it invalidateEventRegionsForFrame()

yep

>>> Source/WebCore/dom/Document.cpp:4247
>>> +        ownerElement->document().invalidateEventRegionsForIFrame(*ownerElement);
>> 
>> You should always find a layer above (the RenderVIew always has one) so I don't think you need this clause, and it takes brainspace.
> 
> I don't understand. No need to recurse on ownerElement? If so, then there is a bug in enclosingCompositingLayerForRepaint() because it can return null for a the middle iframe in pointerevents/ios/touch-action-none-inside-nested-iframe.html. If not, then you mean no need for owner element null check?

Clarified with Simon on stash. Code correct as-is.

>> Source/WebCore/page/Frame.cpp:309
>> +#if PLATFORM(IOS)
> 
> We don't like platform #ifdefs.
> 
> Also async overflow scrolling on macOS uses event regions so this is wrong.

OK, removed #ifdef. Will need to audit RenderLayer::invalidateEventRegion() though....

>> Source/WebCore/page/Frame.cpp:314
>> +    if (!m_doc->renderView()->compositor().viewNeedsToInvalidateEventRegionOfEnclosingCompositingLayerForRepaint())
> 
> Should probably null-check m_doc->renderView().

yep

>> Source/WebCore/page/Frame.cpp:317
>> +        m_ownerElement->document().invalidateEventRegionsForIFrame(*m_ownerElement);
> 
> Actually why doesn't compositor() just do this part.

I hope you don't mind that I defer this for now.

>> Source/WebCore/page/FrameView.cpp:1265
>> +    frame().invalidateContentEventRegionsIfNeeded();
> 
> Please move this down next to invalidateRenderingDependentRegions() because they are related.

OK

>> Source/WebCore/rendering/RenderLayer.cpp:7011
>>  #if PLATFORM(IOS_FAMILY)
> 
> Possible a wrong #ifdef.

I hope you don't mind I do this in a follow up because it likely requires an audit of this code. I might post an experiment patch shortly.

>> Source/WebCore/rendering/RenderLayer.cpp:7032
>> +        compositingLayer->renderer().view().setNeedsEventRegionUpdateForNonCompositedIFrame();
> 
> compositingLayer->renderer().view() is always just renderer().view(); fetch it once.

OK

>> Source/WebCore/rendering/RenderLayer.cpp:7035
>> +        compositingLayer->compositor().scheduleCompositingLayerUpdate();
> 
> This can be view->compositior()

OK

>> Source/WebCore/rendering/RenderLayer.h:928
>> +    bool invalidateEventRegion(EventRegionInvalidationReason);
> 
> What does the return value mean? Add a comment.

Comment added:

// Invalidation can fail if there is no enclosing compositing layer (e.g. nested iframe)
// or the layer does not maintain an event region.

Created attachment 395854 [details]
To land

Created attachment 395855 [details]
[Alt] To Land w/fix guards in RenderLayer::invalidateEventRegion()

Created attachment 395866 [details]
[Alt] Baseline: Just change RenderLayer::invalidateEventRegion()

Comment on attachment 395854 [details]
To land

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

> Source/WebCore/rendering/RenderWidget.cpp:295
> +    if (paintInfo.phase != PaintPhase::Foreground && paintInfo.phase != PaintPhase::EventRegion)

This is wrong. Need to ensure view() doesn't need layout if phase is EventRegion. Otherwise, will hit assertion in FrameView::paintContents(). Note if view().needsEventRegionUpdateForNonCompositedFrame() is set and painting EventRegion then we are guaranteed to have done a layout of this frame (because that bit was set from FrameView::didLayout()).

Changing code to:

[[
if (paintInfo.phase != PaintPhase::Foreground && (paintInfo.phase != PaintPhase::EventRegion || view().needsLayout()))
]]

Created attachment 395877 [details]
To Land

Committed r259761: <https://trac.webkit.org/changeset/259761>