Bug 195378

Summary: Layer with no backing store should still hit-test over a scroller
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: ScrollingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, koivisto, ryanhaddad, simon.fraser, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=195902
Bug Depends on: 195926    
Bug Blocks:    
Attachments:
Description Flags
Testcase
none
patch
none
patch
none
patch none

Description Simon Fraser (smfr) 2019-03-06 14:14:43 PST
Layers can have no backing store but still either have background-color, or even be totally empty, and they should still hit-test over a scroller.
Comment 1 Simon Fraser (smfr) 2019-03-06 14:15:01 PST
Created attachment 363791 [details]
Testcase
Comment 2 Radar WebKit Bug Importer 2019-03-06 14:15:42 PST
<rdar://problem/48652078>
Comment 3 Antti Koivisto 2019-03-18 10:26:55 PDT
Created attachment 365026 [details]
patch
Comment 4 Antti Koivisto 2019-03-18 10:39:26 PDT
Created attachment 365028 [details]
patch
Comment 5 Simon Fraser (smfr) 2019-03-18 11:13:53 PDT
Comment on attachment 365028 [details]
patch

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

r- to fix the pointer-events toggle issue (if it happens to work, please add a test). Don't forget you can use immediateScrollElementAtContentPointToOffset now).

> Source/WebCore/rendering/RenderLayerBacking.cpp:768
> +        updateEventRegion();

Here you're assuming that a layer configuration update will always happen when something changes that affects where renderers are relative to their enclosing compositing layer. This assumption relies on the fact that RenderLayerBacking::setContentsNeedDisplay() calls setNeedsCompositingConfigurationUpdate(), but if future optimizations allow us to update layer configuration in fewer cases, that might break.

In fact, this might break now with something like:

  <div class="composited"><div id="child"></div>

and dynamic toggle of "pointer-events" style on child. That won't issue a repaint (hopefully) and won't update event regions.

You should also add a testcase that just moves elements around and tests event regions, so we can detect regressions caused by future compositing optimizations.
Comment 6 Antti Koivisto 2019-03-18 12:15:06 PDT
https://bugs.webkit.org/show_bug.cgi?id=195902 for pointe-events issue
Comment 7 WebKit Commit Bot 2019-03-18 12:43:04 PDT
Comment on attachment 365028 [details]
patch

Clearing flags on attachment: 365028

Committed r243092: <https://trac.webkit.org/changeset/243092>
Comment 8 WebKit Commit Bot 2019-03-18 12:43:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Ryan Haddad 2019-03-18 13:13:06 PDT
This change broke the Windows build:

c:\cygwin\home\buildbot\worker\win10-release\build\source\webcore\rendering\renderlayerbacking.cpp(1437): error C2668: 'WebCore::GraphicsContext::GraphicsContext': ambiguous call to overloaded function (compiling source file C:\cygwin\home\buildbot\worker\win10-release\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-043dd90b-8.cpp) [C:\cygwin\home\buildbot\worker\win10-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
  c:\cygwin\home\buildbot\worker\win10-release\build\source\webcore\platform\graphics\graphicscontext.h(266): note: could be 'WebCore::GraphicsContext::GraphicsContext(PlatformGraphicsContext *)' (compiling source file C:\cygwin\home\buildbot\worker\win10-release\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-043dd90b-8.cpp)
  c:\cygwin\home\buildbot\worker\win10-release\build\source\webcore\platform\graphics\graphicscontext.h(522): note: or       'WebCore::GraphicsContext::GraphicsContext(HDC,bool)' (compiling source file C:\cygwin\home\buildbot\worker\win10-release\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-043dd90b-8.cpp)
  c:\cygwin\home\buildbot\worker\win10-release\build\source\webcore\rendering\renderlayerbacking.cpp(1437): note: while trying to match the argument list '(nullptr)' (compiling source file C:\cygwin\home\buildbot\worker\win10-release\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-043dd90b-8.cpp)

https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/builds/2965
Comment 10 Antti Koivisto 2019-03-18 13:29:30 PDT
Windows build fix https://trac.webkit.org/changeset/243096/webkit
Comment 11 WebKit Commit Bot 2019-03-18 18:53:40 PDT
Re-opened since this is blocked by bug 195926
Comment 12 Simon Fraser (smfr) 2019-03-18 18:56:19 PDT
This cause assertions like:

 ASSERTION FAILED: clipRectsContext.rootLayer == m_clipRectsCache->m_clipRectsRoot[clipRectsType] 

0   com.apple.JavaScriptCore      	0x000000070f1be55e WTFCrash + 14 (Assertions.cpp:305)
1   com.apple.WebCore             	0x00000006f84d14eb WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x00000006fbba5629 WebCore::RenderLayer::updateClipRects(WebCore::RenderLayer::ClipRectsContext const&) + 345 (RenderLayer.cpp:5349)
3   com.apple.WebCore             	0x00000006fbba6acf WebCore::RenderLayer::parentClipRects(WebCore::RenderLayer::ClipRectsContext const&) const + 255
4   com.apple.WebCore             	0x00000006fbba6bbf WebCore::RenderLayer::backgroundClipRect(WebCore::RenderLayer::ClipRectsContext const&) const::$_1::operator()() const + 223 (RenderLayer.cpp:5499)
5   com.apple.WebCore             	0x00000006fbb9a77e WebCore::RenderLayer::backgroundClipRect(WebCore::RenderLayer::ClipRectsContext const&) const + 158 (RenderLayer.cpp:5507)
6   com.apple.WebCore             	0x00000006fbba10b0 WebCore::RenderLayer::calculateRects(WebCore::RenderLayer::ClipRectsContext const&, WebCore::LayoutRect const&, WebCore::LayoutRect&, WebCore::ClipRect&, WebCore::ClipRect&, WebCore::LayoutSize const&) const + 112 (RenderLayer.cpp:5519)
7   com.apple.WebCore             	0x00000006fbb9ec0f WebCore::RenderLayer::collectFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul>&, WebCore::RenderLayer const*, WebCore::LayoutRect const&, WebCore::RenderLayer::PaginationInclusionMode, WebCore::ClipRectsType, WebCore::OverlayScrollbarSizeRelevancy, WebCore::ShouldRespectOverflowClip, WebCore::LayoutSize const&, WebCore::LayoutRect const*, WebCore::ShouldApplyRootOffsetToFragments) + 303 (RenderLayer.cpp:4516)
8   com.apple.WebCore             	0x00000006fbb9ba05 WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) + 2885 (RenderLayer.cpp:4339)

because the paint triggered by RenderLayerBacking::updateEventRegion() caches the wrong clip rect roots.

Also, why are we paying the cost for this event region computation on macOS?
Comment 13 Simon Fraser (smfr) 2019-03-18 19:03:48 PDT
compositing/repaint/requires-backing-repaint.html is a test that hit this assertion, on macOS.
Comment 14 Antti Koivisto 2019-03-19 01:46:16 PDT
Created attachment 365138 [details]
patch

Don't collect the region for layers that have paintsIntoCompositedAncestor() set.
Comment 15 WebKit Commit Bot 2019-03-19 04:07:48 PDT
Comment on attachment 365138 [details]
patch

Clearing flags on attachment: 365138

Committed r243134: <https://trac.webkit.org/changeset/243134>
Comment 16 WebKit Commit Bot 2019-03-19 04:07:50 PDT
All reviewed patches have been landed.  Closing bug.