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