Bug 210041 - Should find touch-action elements inside non-composited iframes
Summary: Should find touch-action elements inside non-composited iframes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks: 209888 210278
  Show dependency treegraph
 
Reported: 2020-04-05 17:04 PDT by Daniel Bates
Modified: 2020-04-09 16:27 PDT (History)
13 users (show)

See Also:


Attachments
First attempt (29.25 KB, patch)
2020-04-07 11:21 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
First attempt (29.04 KB, patch)
2020-04-07 11:32 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Second attempt (39.17 KB, patch)
2020-04-07 23:26 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (50.25 KB, patch)
2020-04-08 00:07 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (45.00 KB, patch)
2020-04-08 00:24 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (45.03 KB, patch)
2020-04-08 00:31 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (47.83 KB, patch)
2020-04-08 00:45 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (48.67 KB, patch)
2020-04-08 01:14 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and tests (50.08 KB, patch)
2020-04-08 08:43 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To land (50.55 KB, patch)
2020-04-08 12:30 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
[Alt] To Land w/fix guards in RenderLayer::invalidateEventRegion() (57.62 KB, patch)
2020-04-08 12:47 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
[Alt] Baseline: Just change RenderLayer::invalidateEventRegion() (1.46 KB, patch)
2020-04-08 13:58 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (50.67 KB, patch)
2020-04-08 15:37 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-04-05 17:04:24 PDT
Consider the following test:

[[
<!DOCTYPE html>
<html>
<body>
<iframe srcdoc="<input style='touch-action: none; width: 256px; height: 100px; border:1px solid black'></div>"></iframe>
</body>
</html>
]]

Then there will be **no** touch action region for the <input> because its containing <iframe> is **not** composited. But we should have found the <input>!

Compare to:

[[
<!DOCTYPE html>
<html>
<body>
<iframe srcdoc="<input style='touch-action:none; width: 256px; height: 300px; border:1px solid black'></div>"></iframe>
</body>
</html>
]]

Then there **will be** a touch action region for the <input> because its containing <iframe> **is** composited.

**Keep in mind that touch-action regions should also be updated an element's touch action dynamically changes inside a non-composited  <iframe>***

Also, keep in mind the fix for this bug should **not** be specific to just frame boundaries, but any non-composited things. Though feel free to keep this bug focused on frames and file more bugs.
Comment 1 Radar WebKit Bug Importer 2020-04-05 17:04:37 PDT
<rdar://problem/61323558>
Comment 2 Daniel Bates 2020-04-05 17:15:06 PDT
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();
]]
Comment 3 Simon Fraser (smfr) 2020-04-05 18:13:54 PDT
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.
Comment 4 Daniel Bates 2020-04-07 11:21:53 PDT
Created attachment 395713 [details]
First attempt
Comment 5 Daniel Bates 2020-04-07 11:25:54 PDT
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 6 Daniel Bates 2020-04-07 11:31:35 PDT
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.
Comment 7 Daniel Bates 2020-04-07 11:32:32 PDT
Created attachment 395714 [details]
First attempt
Comment 8 Simon Fraser (smfr) 2020-04-07 14:18:42 PDT
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 9 Daniel Bates 2020-04-07 14:29:36 PDT
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 10 Daniel Bates 2020-04-07 14:37:19 PDT
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.
Comment 11 Daniel Bates 2020-04-07 23:26:17 PDT
Created attachment 395772 [details]
Second attempt
Comment 12 Daniel Bates 2020-04-08 00:07:18 PDT
Created attachment 395774 [details]
For the bots
Comment 13 Daniel Bates 2020-04-08 00:24:28 PDT
Created attachment 395776 [details]
For the bots
Comment 14 Daniel Bates 2020-04-08 00:31:34 PDT
Created attachment 395778 [details]
For the bots
Comment 15 Daniel Bates 2020-04-08 00:45:17 PDT
Created attachment 395779 [details]
For the bots
Comment 16 Daniel Bates 2020-04-08 01:14:24 PDT
Created attachment 395781 [details]
For the bots
Comment 17 Daniel Bates 2020-04-08 08:43:35 PDT
Created attachment 395815 [details]
Patch and tests
Comment 18 Simon Fraser (smfr) 2020-04-08 10:18:13 PDT
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 19 Daniel Bates 2020-04-08 11:00:28 PDT
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 20 Daniel Bates 2020-04-08 12:28:12 PDT
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.
Comment 21 Daniel Bates 2020-04-08 12:30:05 PDT
Created attachment 395854 [details]
To land
Comment 22 Daniel Bates 2020-04-08 12:47:56 PDT
Created attachment 395855 [details]
[Alt] To Land w/fix guards in RenderLayer::invalidateEventRegion()
Comment 23 Daniel Bates 2020-04-08 13:58:17 PDT
Created attachment 395866 [details]
[Alt] Baseline: Just change RenderLayer::invalidateEventRegion()
Comment 24 Daniel Bates 2020-04-08 15:26:16 PDT
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()))
]]
Comment 25 Daniel Bates 2020-04-08 15:37:45 PDT
Created attachment 395877 [details]
To Land
Comment 26 Daniel Bates 2020-04-08 15:38:41 PDT
Committed r259761: <https://trac.webkit.org/changeset/259761>