Bug 198005

Summary: REGRESSION (r245170): gmail.com inbox table header flickers
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CompositingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: koivisto, simon.fraser, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Testcase
none
Patch koivisto: review+

Simon Fraser (smfr)
Reported 2019-05-17 15:11:14 PDT
After r245471 there's still a bug where the gmail header above the inbox (where it says "Primary") flickers.
Attachments
Testcase (1.33 KB, text/html)
2019-05-17 15:36 PDT, Simon Fraser (smfr) 		no flags
Details
Patch (27.46 KB, patch)
2019-05-17 17:03 PDT, Simon Fraser (smfr) 		koivisto: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1 2019-05-17 15:11:27 PDT
<rdar://problem/50907718>
Simon Fraser (smfr)
Comment 2 2019-05-17 15:36:58 PDT
Created attachment 370164 [details] Testcase
Simon Fraser (smfr)
Comment 3 2019-05-17 15:38:18 PDT
I think the issue is that the layer for "header" uses "paints into composited ancestor" and paints via the root, but that ancestor is obscured behind an intermediate layer (the backdrop). S--------C-c- -- ------ ------ 0x57ef772b0 (0,0) width=875 height=835 (layerID 16) {sc 2} RenderView S----------c- -- ------ ------ + 0x57ef77408 (0,0) width=875 height=328 <html> S-------XC--- -- ------ ------ + 0x57ef77560 (10,10) width=550 height=350 (layerID 21) {vc 3} <div> class='backdrop' ---------C--- -- ------ ------ + 0x57ef776b8 (20,20) width=528 height=328 (layerID 22) <div> class='container' S-O------Cac- -- ------ ------ + 0x57ef77810 (14,14) width=504 height=304 (layerID 28) <div> class='header' ------------- -- ------ ------ + 0x57ef77968 (12,12) width=200 height=100 <div> class='target' S--A-----C--- -- ------ ------ + 0x57ef77ac0 (252,122) width=220 height=120 (layerID 27) <div> class='animating changed'
Simon Fraser (smfr)
Comment 4 2019-05-17 15:38:45 PDT
Odd that we haven't seen this before, since the "paints into composited ancestor" logic has never consulted overlap.
Simon Fraser (smfr)
Comment 5 2019-05-17 16:27:03 PDT
OIC. We used to leave an "overlap" indirect composited reason on the layer which prevented painting into an ancestor, but now with backing sharing that gets cleared. Subtle.
Simon Fraser (smfr)
Comment 6 2019-05-17 17:03:27 PDT
Created attachment 370171 [details] Patch
Antti Koivisto
Comment 7 2019-05-17 17:12:42 PDT
Comment on attachment 370171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370171&action=review > Source/WebCore/ChangeLog:16 > + Mak some logging changes to help diagnose things like this. Typo > Source/WebCore/rendering/RenderLayerCompositor.cpp:935 > + if (layerPaintsIntoProvidedBacking) { > + layerPaintsIntoProvidedBacking = false; > + // layerPaintsIntoProvidedBacking was only true for layers that would otherwise composite because of overlap. If we can > + // no longer share, put this this indirect reason back on the layer so that requiresOwnBackingStore() sees it. > + layer.setIndirectCompositingReason(IndirectCompositingReason::Overlap); It is bit non-obvious that layerPaintsIntoProvidedBacking implies IndirectCompositingReason::Overlap. Maybe rename it to layerPaintsIntoProvidedBackingDueToOverlap or similar to avoid future mistakes?
Simon Fraser (smfr)
Comment 8 2019-05-17 18:29:58 PDT
(In reply to Antti Koivisto from comment #7) > Comment on attachment 370171 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370171&action=review > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:935 > > + if (layerPaintsIntoProvidedBacking) { > > + layerPaintsIntoProvidedBacking = false; > > + // layerPaintsIntoProvidedBacking was only true for layers that would otherwise composite because of overlap. If we can > > + // no longer share, put this this indirect reason back on the layer so that requiresOwnBackingStore() sees it. > > + layer.setIndirectCompositingReason(IndirectCompositingReason::Overlap); > > It is bit non-obvious that layerPaintsIntoProvidedBacking implies > IndirectCompositingReason::Overlap. Maybe rename it to > layerPaintsIntoProvidedBackingDueToOverlap or similar to avoid future > mistakes? I think making "layer needs to overlap" not an IndirectCompositingReason, but a bit on the layer would have helped avoid this, but that can happen later.
Simon Fraser (smfr)
Comment 9 2019-05-17 18:32:29 PDT
https://trac.webkit.org/r245490
Note You need to log in before you can comment on or make changes to this bug.