Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2382563002:
Revert changes is the use of CompositingReasonOutOfFlowClipping
|
Created:
4 years, 2 months ago by Stephen Chennney Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, Xianzhu, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRevert changes in the use of CompositingReasonOutOfFlowClipping
Patch https://codereview.chromium.org/2259493004 changed the logic
for applying CompositingReasonOutOfFlowClipping, resulting in
broken rendering of the gmail Send bar for long messages.
This patch restores the origin logic, updated for the new compositing
of opaque scrollers.
R=flackr@chromium.org
BUG=650446
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add check for composited scrolling ancestor #
Messages
Total messages: 19 (9 generated)
Description was changed from ========== Revert changes is the use of CompositingReasonOutOfFlowClipping Patch https://codereview.chromium.org/2259493004 changed the logic for applying CompositingReasonOutOfFlowClipping, resulting in broken rendering of the gmail Send bar for long messages. This patch restores the origin logic, updated for the new compositing of opaque scrollers. R= BUG=650446 ========== to ========== Revert changes is the use of CompositingReasonOutOfFlowClipping Patch https://codereview.chromium.org/2259493004 changed the logic for applying CompositingReasonOutOfFlowClipping, resulting in broken rendering of the gmail Send bar for long messages. This patch restores the origin logic, updated for the new compositing of opaque scrollers. R= BUG=650446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Revert changes is the use of CompositingReasonOutOfFlowClipping Patch https://codereview.chromium.org/2259493004 changed the logic for applying CompositingReasonOutOfFlowClipping, resulting in broken rendering of the gmail Send bar for long messages. This patch restores the origin logic, updated for the new compositing of opaque scrollers. R= BUG=650446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Revert changes is the use of CompositingReasonOutOfFlowClipping Patch https://codereview.chromium.org/2259493004 changed the logic for applying CompositingReasonOutOfFlowClipping, resulting in broken rendering of the gmail Send bar for long messages. This patch restores the origin logic, updated for the new compositing of opaque scrollers. R= BUG=650446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Not ready to land. I need to verify the prefer-compositing-to-lcd-text codepath and I need to try to come up with a test that reflects the Gmail breakage.
flackr@chromium.org changed reviewers: + flackr@chromium.org
https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (checkForClipParent && layer->clipParent()) To restore the original logic we only need to check (m_compositingTriggers & OverflowScrollTrigger) && layer->clipParent() right? In order to handle the new clip promotion cases correctly you need to know if an ancestor overflow scroller has been promoted, but I believe with line 142 you are only setting checkForClipParent if the current layer is promoted.
https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (checkForClipParent && layer->clipParent()) Still working on this, but ... On 2016/09/29 14:21:20, flackr wrote: > To restore the original logic we only need to check (m_compositingTriggers & > OverflowScrollTrigger) && layer->clipParent() right? The original logic would have made the clipParent check regardless of whether you were scrolling or not, which means it did the check for composited scrollers too. With the new path we will be compositing only the scrollers, but I presume that because we needed it for all composited layers before, we now also need it for the specific case of composited scrollers. > In order to handle the new clip promotion cases correctly you need to know if an > ancestor overflow scroller has been promoted, but I believe with line 142 you > are only setting checkForClipParent if the current layer is promoted. Do we have a call that checks for any composited ancestor scroller?
flackr@chromium.org changed reviewers: + vollick@chromium.org
https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (checkForClipParent && layer->clipParent()) On 2016/09/30 at 17:12:25, Stephen Chennney wrote: > Still working on this, but ... > > On 2016/09/29 14:21:20, flackr wrote: > > To restore the original logic we only need to check (m_compositingTriggers & > > OverflowScrollTrigger) && layer->clipParent() right? > > The original logic would have made the clipParent check regardless of whether you were scrolling or not, which means it did the check for composited scrollers too. With the new path we will be compositing only the scrollers, but I presume that because we needed it for all composited layers before, we now also need it for the specific case of composited scrollers. +vollick, I'm having a hard time putting together a test case, but as I understand it, the clipParent check is intended to promote elements which need to be stacked differently than the scrolling contents (i.e. not part of the same stacking tree) but still clipped by the scroll. My concern with this approach is that it seems with the CompositeOpaqueScrollers flag (i.e. OverflowScrollTrigger is false, right?) we will only test whether overflow scrollers need to be promoted due to a clip and only if they have already been promoted. > > > In order to handle the new clip promotion cases correctly you need to know if an > > ancestor overflow scroller has been promoted, but I believe with line 142 you > > are only setting checkForClipParent if the current layer is promoted. > > Do we have a call that checks for any composited ancestor scroller? You should be able to check if it has a layer->ancestorScrollingLayer and if that layer has been promoted.
On 2016/09/30 20:13:17, flackr wrote: > https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... > File > third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp > (right): > > https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: > if (checkForClipParent && layer->clipParent()) > On 2016/09/30 at 17:12:25, Stephen Chennney wrote: > > Still working on this, but ... > > > > On 2016/09/29 14:21:20, flackr wrote: > > > To restore the original logic we only need to check (m_compositingTriggers & > > > OverflowScrollTrigger) && layer->clipParent() right? > > > > The original logic would have made the clipParent check regardless of whether > you were scrolling or not, which means it did the check for composited scrollers > too. With the new path we will be compositing only the scrollers, but I presume > that because we needed it for all composited layers before, we now also need it > for the specific case of composited scrollers. > > > +vollick, I'm having a hard time putting together a test case, but as I > understand it, the clipParent check is intended to promote elements which need > to be stacked differently than the scrolling contents (i.e. not part of the same > stacking tree) but still clipped by the scroll. My concern with this approach is > that it seems with the CompositeOpaqueScrollers flag (i.e. OverflowScrollTrigger > is false, right?) we will only test whether overflow scrollers need to be > promoted due to a clip and only if they have already been promoted. > > > > > > In order to handle the new clip promotion cases correctly you need to know > if an > > > ancestor overflow scroller has been promoted, but I believe with line 142 > you > > > are only setting checkForClipParent if the current layer is promoted. > > > > Do we have a call that checks for any composited ancestor scroller? > > You should be able to check if it has a layer->ancestorScrollingLayer and if > that layer has been promoted. So the gmail bug no longer reproduces for me on trunk, either on Mac or on Linux with the necessary change to make it composite the scroller. Does anyone know what's up with that? Also seems to work on current Mac Canary. flackr, you landed something about promoting fixed position content, right? The "Send" bar is a fixed position div. I can't get a test case to break either.
On 2016/10/03 at 19:54:58, schenney wrote: > On 2016/09/30 20:13:17, flackr wrote: > > https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... > > File > > third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp > > (right): > > > > https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: > > if (checkForClipParent && layer->clipParent()) > > On 2016/09/30 at 17:12:25, Stephen Chennney wrote: > > > Still working on this, but ... > > > > > > On 2016/09/29 14:21:20, flackr wrote: > > > > To restore the original logic we only need to check (m_compositingTriggers & > > > > OverflowScrollTrigger) && layer->clipParent() right? > > > > > > The original logic would have made the clipParent check regardless of whether > > you were scrolling or not, which means it did the check for composited scrollers > > too. With the new path we will be compositing only the scrollers, but I presume > > that because we needed it for all composited layers before, we now also need it > > for the specific case of composited scrollers. > > > > > > +vollick, I'm having a hard time putting together a test case, but as I > > understand it, the clipParent check is intended to promote elements which need > > to be stacked differently than the scrolling contents (i.e. not part of the same > > stacking tree) but still clipped by the scroll. My concern with this approach is > > that it seems with the CompositeOpaqueScrollers flag (i.e. OverflowScrollTrigger > > is false, right?) we will only test whether overflow scrollers need to be > > promoted due to a clip and only if they have already been promoted. > > > > > > > > > In order to handle the new clip promotion cases correctly you need to know > > if an > > > > ancestor overflow scroller has been promoted, but I believe with line 142 > > you > > > > are only setting checkForClipParent if the current layer is promoted. > > > > > > Do we have a call that checks for any composited ancestor scroller? > > > > You should be able to check if it has a layer->ancestorScrollingLayer and if > > that layer has been promoted. > > So the gmail bug no longer reproduces for me on trunk, either on Mac or on Linux with the necessary change to make it composite the scroller. Does anyone know what's up with that? Also seems to work on current Mac Canary. > > flackr, you landed something about promoting fixed position content, right? The "Send" bar is a fixed position div. My change for promoting fixed position content is behind the CompositeOpaqueFixedPosition flag so shouldn't be affecting canary. > > I can't get a test case to break either. I think vollick might have a better idea when this case comes up. I too was having a hard time producing a test case for this that didn't trigger another compositing reason ending up looking correct anyways.
https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2382563002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (checkForClipParent && layer->clipParent()) On 2016/09/30 20:13:16, flackr wrote: > On 2016/09/30 at 17:12:25, Stephen Chennney wrote: > > Still working on this, but ... > > > > On 2016/09/29 14:21:20, flackr wrote: > > > To restore the original logic we only need to check (m_compositingTriggers & > > > OverflowScrollTrigger) && layer->clipParent() right? > > > > The original logic would have made the clipParent check regardless of whether > you were scrolling or not, which means it did the check for composited scrollers > too. With the new path we will be compositing only the scrollers, but I presume > that because we needed it for all composited layers before, we now also need it > for the specific case of composited scrollers. > > > +vollick, I'm having a hard time putting together a test case, but as I > understand it, the clipParent check is intended to promote elements which need > to be stacked differently than the scrolling contents (i.e. not part of the same > stacking tree) but still clipped by the scroll. My concern with this approach is > that it seems with the CompositeOpaqueScrollers flag (i.e. OverflowScrollTrigger > is false, right?) we will only test whether overflow scrollers need to be > promoted due to a clip and only if they have already been promoted. > > > > > > In order to handle the new clip promotion cases correctly you need to know > if an > > > ancestor overflow scroller has been promoted, but I believe with line 142 > you > > > are only setting checkForClipParent if the current layer is promoted. > > > > Do we have a call that checks for any composited ancestor scroller? > > You should be able to check if it has a layer->ancestorScrollingLayer and if > that layer has been promoted. Some background on clip parent (because it's confusing). You get a clip parent if you're positioned below a clipping layer in the composited layer tree, but you don't actually want to inherit that clip. Consider a fixed-pos element within a non-cb-inducing overflow scroller. Even if the fixed-pos element's composited layer is below the scroll clip, it needs to ignore it, so we assign a clipParent to that layer to indicate that we should ignore all clips up to the clip parent (or, put another way, that the layer is only clipped by stuff from the clipParent up). So I'm a bit confused by the code requiring that the layer needs to be a composited scroller to get a promoted due to having a clipParent.
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
Description was changed from ========== Revert changes is the use of CompositingReasonOutOfFlowClipping Patch https://codereview.chromium.org/2259493004 changed the logic for applying CompositingReasonOutOfFlowClipping, resulting in broken rendering of the gmail Send bar for long messages. This patch restores the origin logic, updated for the new compositing of opaque scrollers. R= BUG=650446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Revert changes in the use of CompositingReasonOutOfFlowClipping Patch https://codereview.chromium.org/2259493004 changed the logic for applying CompositingReasonOutOfFlowClipping, resulting in broken rendering of the gmail Send bar for long messages. This patch restores the origin logic, updated for the new compositing of opaque scrollers. R=flackr@chromium.org BUG=650446 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Added the check for a composited scrolling ancestor, but only for the path where the layer itself is not scrolling. Maybe land it as is to fix the regression, and see what falls out.
On 2016/10/04 at 19:06:29, schenney wrote: > Added the check for a composited scrolling ancestor, but only for the path where the layer itself is not scrolling. > > Maybe land it as is to fix the regression, and see what falls out. I'd suggest readding the original code to fix the regression, i.e.: if ((m_compositingTriggers & OverflowScrollTrigger) && layer->clipParent()) directReasons |= CompositingReasonOutOfFlowClipping and the change to CompositingRequirementsUpdater.cpp This should fix any regression on high dpi right? And then we could follow up with fixing it for the composite opaque scrollers case with appropriate tests since this case is harder - we promote some but not all scrollers.
On 2016/10/04 19:13:33, flackr wrote: > On 2016/10/04 at 19:06:29, schenney wrote: > > Added the check for a composited scrolling ancestor, but only for the path > where the layer itself is not scrolling. > > > > Maybe land it as is to fix the regression, and see what falls out. > > I'd suggest readding the original code to fix the regression, i.e.: > if ((m_compositingTriggers & OverflowScrollTrigger) && layer->clipParent()) > directReasons |= CompositingReasonOutOfFlowClipping > and the change to CompositingRequirementsUpdater.cpp > > This should fix any regression on high dpi right? > > And then we could follow up with fixing it for the composite opaque scrollers > case with appropriate tests since this case is harder - we promote some but not > all scrollers. Seems reasonable. Gmail is not getting promoted scrolling on low-DPI anyway, for reasons I am still yet to investigate.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
