Bug 210041

Summary:

Should find touch-action elements inside non-composited iframes

Product:

WebKit

Reporter:

Daniel Bates <dbates>

Component:

UI Events

Assignee:

Daniel Bates <dbates>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, cdumez, esprehn+autocc, ews-watchlist, fred.wang, glenn, kangil.han, koivisto, kondapallykalyan, pdr, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=200204
https://bugs.webkit.org/show_bug.cgi?id=200205
https://bugs.webkit.org/show_bug.cgi?id=210216
https://bugs.webkit.org/show_bug.cgi?id=210311

Bug Depends on:

Bug Blocks:

209888, 210278

Attachments:

Description	Flags
First attempt	none
First attempt	none
Second attempt	none
For the bots	none
For the bots	none
For the bots	none
For the bots	none
For the bots	none
Patch and tests	none
To land	none
[Alt] To Land w/fix guards in RenderLayer::invalidateEventRegion()	none
[Alt] Baseline: Just change RenderLayer::invalidateEventRegion()	none
To Land	none

Daniel Bates

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

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
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2020-04-05 17:04:37 PDT

<rdar://problem/61323558>

Daniel Bates

Comment 2 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(); ]]

Simon Fraser (smfr)

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

Daniel Bates

Comment 4 2020-04-07 11:21:53 PDT

Created attachment 395713 [details] First attempt

Daniel Bates

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

Daniel Bates

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

Daniel Bates

Comment 7 2020-04-07 11:32:32 PDT

Created attachment 395714 [details] First attempt

Simon Fraser (smfr)

Comment 8 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

Daniel Bates

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

Daniel Bates

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

Daniel Bates

Comment 11 2020-04-07 23:26:17 PDT

Created attachment 395772 [details] Second attempt

Daniel Bates

Comment 12 2020-04-08 00:07:18 PDT

Created attachment 395774 [details] For the bots

Daniel Bates

Comment 13 2020-04-08 00:24:28 PDT

Created attachment 395776 [details] For the bots

Daniel Bates

Comment 14 2020-04-08 00:31:34 PDT

Created attachment 395778 [details] For the bots

Daniel Bates

Comment 15 2020-04-08 00:45:17 PDT

Created attachment 395779 [details] For the bots

Daniel Bates

Comment 16 2020-04-08 01:14:24 PDT

Created attachment 395781 [details] For the bots

Daniel Bates

Comment 17 2020-04-08 08:43:35 PDT

Created attachment 395815 [details] Patch and tests

Simon Fraser (smfr)

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

Daniel Bates

Comment 19 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?

Daniel Bates

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

Daniel Bates

Comment 21 2020-04-08 12:30:05 PDT

Created attachment 395854 [details] To land

Daniel Bates

Comment 22 2020-04-08 12:47:56 PDT

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

Daniel Bates

Comment 23 2020-04-08 13:58:17 PDT

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

Daniel Bates

Comment 24 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())) ]]

Daniel Bates

Comment 25 2020-04-08 15:37:45 PDT

Created attachment 395877 [details] To Land

Daniel Bates

Comment 26 2020-04-08 15:38:41 PDT

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

Note You need to log in before you can comment on or make changes to this bug.