Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845613002:
Fix the bug that sticky element may not be correctly invalidated due to non-promotion (Closed)
DescriptionFix the bug that sticky element may not be correctly invalidated due to
non-promotion.
When a non composited sticky element is nested in a compositied ancestor
which requires repaint on scroll, the sticky element should also be
composited to have correct paint invalidation.
TEST=compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html;
BUG=698358
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2845613002
Cr-Commit-Position: refs/heads/master@{#472947}
Committed: https://chromium.googlesource.com/chromium/src/+/e44021ea6e3e18bc59854b4c8d30999f1fc4379f
Patch Set 1 #
Total comments: 12
Patch Set 2 : Logic update && add layout test #
Total comments: 10
Patch Set 3 : Update tests #
Total comments: 7
Patch Set 4 : Address comments #
Total comments: 2
Patch Set 5 : Remove paint invalidation related logic #Patch Set 6 : bug fix && nit #Patch Set 7 : Update compositing trigger #Patch Set 8 : bug fix #
Total comments: 2
Patch Set 9 : Address comments #
Total comments: 2
Patch Set 10 : Add comments #Patch Set 11 : Update test due to spv2 failure #
Total comments: 6
Patch Set 12 : Remove promotion logic and set appropriate ingore_lcd_text instead #Patch Set 13 : Bug fix #
Total comments: 4
Patch Set 14 : Address comments #
Total comments: 4
Patch Set 15 : Address comments #
Messages
Total messages: 64 (30 generated)
Description was changed from ========== Fix position:sticky <th> disappears if table is composited but the <th> isn't. We should have a paint invalidation for the non-composited <th> if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html BUG=698358 ========== to ========== Fix position:sticky <th> disappears if table is composited but the <th> isn't. We should have a paint invalidation for the non-composited <th> if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Fix position:sticky <th> disappears if table is composited but the <th> isn't. We should have a paint invalidation for the non-composited <th> if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix position:sticky <th> disappears if table is composited but the <th> isn't. We should have a paint invalidation for the non-composited <th> if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
yigu@chromium.org changed reviewers: + flackr@chromium.org
Hi Rob. PTAL at my fix. Thanks!
https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor-expected.html (right): https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor-expected.html:37: window.requestAnimationFrame(function() { There's no need to delay the scroll for the expectation. https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html (right): https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:18: background: green; If the sticky element is known opaque we may promote it automatically. https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:35: for (let scroller of document.querySelectorAll('.scroller')) { There's only a single scroller, can just querySelector('.scroller') or use an id. https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:41: window.requestAnimationFrame(function() { Doesn't this need to be declared an async test that waits until done? See https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css/... https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:48: <table class="composited"> A simpler test would just be a large composited div containing a sticky non-composited div. Does it need a table? https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.cpp:845: !new_offset.IsZero()) { This will invalidate as long as the offset is not zero, but we only need to invalidate if the offset changed. I think you should add some invalidation tests that show that the sticky position element doesn't invalidate if it hasn't moved (i.e. hasn't passed the starting constraint yet). See for example https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv...
Thanks Rob. I've updated the patch. PTAL. https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor-expected.html (right): https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor-expected.html:37: window.requestAnimationFrame(function() { On 2017/04/26 15:21:00, flackr wrote: > There's no need to delay the scroll for the expectation. Done. https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html (right): https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:18: background: green; On 2017/04/26 15:21:01, flackr wrote: > If the sticky element is known opaque we may promote it automatically. Done. https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:35: for (let scroller of document.querySelectorAll('.scroller')) { On 2017/04/26 15:21:01, flackr wrote: > There's only a single scroller, can just querySelector('.scroller') or use an > id. Done. https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:41: window.requestAnimationFrame(function() { On 2017/04/26 15:21:00, flackr wrote: > Doesn't this need to be declared an async test that waits until done? See > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css/... Done. https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:48: <table class="composited"> On 2017/04/26 15:21:01, flackr wrote: > A simpler test would just be a large composited div containing a sticky > non-composited div. Does it need a table? Done. https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2845613002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.cpp:845: !new_offset.IsZero()) { On 2017/04/26 15:21:01, flackr wrote: > This will invalidate as long as the offset is not zero, but we only need to > invalidate if the offset changed. I think you should add some invalidation tests > that show that the sticky position element doesn't invalidate if it hasn't moved > (i.e. hasn't passed the starting constraint yet). See for example > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv... Done.
Description was changed from ========== Fix position:sticky <th> disappears if table is composited but the <th> isn't. We should have a paint invalidation for the non-composited <th> if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix position:sticky <th> disappears if table is composited but the <th> isn't. We should have a paint invalidation for the non-composited <th> if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint.html BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor-expected.html (right): https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor-expected.html:11: position: sticky; Just to avoid testing sticky with sticky, let's use position: relative, fixed or absolute and put the box where it should be at that scroll position. https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html (right): https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:14: background: rgba(255, 0, 0, 0.5); nit: Use a greenish color - i.e. rgba(0, 255, 0, 0.9); https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:24: background: green; nit: Let's just make this gray or something similar. https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:30: <div class="box"></div> While not necessary, the nice way to represent this would be an outline or box showing where we expect the sticky box to be (e.g. <div class="indicator"></div> .indicator { position: absolute; top: ##px; height: 50px; background: red; /* covered up by .box when working */ } https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint-expected.txt (right): https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint-expected.txt:28: "name": "LayoutBlockFlow DIV class='composited container'", Shouldn't we see a paint invalidation in this container when the sticky position element moves?
yigu@chromium.org changed reviewers: + chrishtr@chromium.org
yigu@chromium.org changed required reviewers: + chrishtr@chromium.org
Hi Chris. PTAL at this bug fix. Thanks! https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor-expected.html (right): https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor-expected.html:11: position: sticky; On 2017/04/27 14:22:56, flackr wrote: > Just to avoid testing sticky with sticky, let's use position: relative, fixed or > absolute and put the box where it should be at that scroll position. Done. https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html (right): https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:14: background: rgba(255, 0, 0, 0.5); On 2017/04/27 14:22:56, flackr wrote: > nit: Use a greenish color - i.e. rgba(0, 255, 0, 0.9); Done. https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:24: background: green; On 2017/04/27 14:22:56, flackr wrote: > nit: Let's just make this gray or something similar. Done. https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:30: <div class="box"></div> On 2017/04/27 14:22:56, flackr wrote: > While not necessary, the nice way to represent this would be an outline or box > showing where we expect the sticky box to be (e.g. <div class="indicator"></div> > .indicator { > position: absolute; > top: ##px; > height: 50px; > background: red; /* covered up by .box when working */ > } Done. https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint-expected.txt (right): https://codereview.chromium.org/2845613002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint-expected.txt:28: "name": "LayoutBlockFlow DIV class='composited container'", On 2017/04/27 14:22:56, flackr wrote: > Shouldn't we see a paint invalidation in this container when the sticky position > element moves? This box hasn't yet passed the starting constraint so no need to invalidate it. Have added another box which moves and needs to be invalidated. https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint.html (right): https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint.html:36: <div class="spacer"></div> If this spacer is not composited, e.g. we move it out of the composited container, we will invalidate it as scroll *if we disable sp invalidation* (layout test failure in virtual/disable-spinvalidation/). Is that expected? chrishtr@ WDYT?
https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html (right): https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:20: width: 100%; nit: Shouldn't this be the same width as the indicator so it perfectly covers it up? https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint-expected.txt (right): https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint-expected.txt:33: "reason": "full" Shouldn't the invalidation reasons for the sticky box be "location change"? https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:841: if (GetLayoutObject().Style()->GetPosition() == EPosition::kSticky && Remove the check for position sticky, I don't think this needs to be special cased. If we remove this check, does it fire if a position: relative; element gets a new offset applied?
Thanks Rob. PTAL. https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html (right): https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html:20: width: 100%; On 2017/04/28 17:14:57, flackr wrote: > nit: Shouldn't this be the same width as the indicator so it perfectly covers it > up? Done. https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint-expected.txt (right): https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint-expected.txt:33: "reason": "full" On 2017/04/28 17:14:57, flackr wrote: > Shouldn't the invalidation reasons for the sticky box be "location change"? I applied InvalidatePaintIncludingNonCompositingDescendants(). Now set the location change reason. https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2845613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:841: if (GetLayoutObject().Style()->GetPosition() == EPosition::kSticky && On 2017/04/28 17:14:57, flackr wrote: > Remove the check for position sticky, I don't think this needs to be special > cased. If we remove this check, does it fire if a position: relative; element > gets a new offset applied? Done.
https://codereview.chromium.org/2845613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2845613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:841: if (!HasCompositedLayerMapping() && This is not in general the right place to be doing paint invalidations like this. But maybe there is something new to add...could you explain the core issue thatis happening in these cases? I'm not yet sure what it is. Please also update the title of the patch to match the description of the core issue.
Description was changed from ========== Fix position:sticky <th> disappears if table is composited but the <th> isn't. We should have a paint invalidation for the non-composited <th> if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint.html BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix the bug that non composited sticky element may not be correctly invalidated. We should have a paint invalidation for the non-composited element if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint.html BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Thanks Chris. Please see explanation below. https://codereview.chromium.org/2845613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2845613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:841: if (!HasCompositedLayerMapping() && On 2017/04/28 20:11:43, chrishtr wrote: > This is not in general the right place to be doing paint invalidations like > this. > But maybe there is something new to add...could you explain the core issue > thatis happening in these cases? I'm not yet sure what it is. > > Please also update the title of the patch to match the description of the core > issue. Suppose we have a region A that needs repaint on scroll and it has a composited but main thread scrolled child B. When we recursively do paint invalidation from A we may stop at the composited child B so B's non-composited children cannot get invalidated. Here we invalidate the children if their location changed.
This is not just a problem for position:sticky. Changing the compositing trigger
in
non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html
to:
.composited {
backface-visibility: hidden;
isolation: isolate;
}
will cause the position:absolute element to appear to scroll, even though it
should not,
because its containing block is the body element.
This will only reproduce for position:absolute on low-DPI screens, because
otherwise
kCompositingReasonOutOfFlowClipping kicks in. I don't know why exactly that
trigger
is limited to high-DPI screens but I think it should not be.
Proposed fix:
1. Get rid of kOverflowScroll trigger in the conditional here
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
(fixes bug with position:absolute)
Promote position:sticky elements with compositing scrolling ancestors (maybe
unless the constraints don't involve
the scroller). This can be detected by using the
has_composited_scrolling_ancestor_
variable in CompositingRequirementsUpdater.
On 2017/04/29 01:43:00, chrishtr wrote:
> This is not just a problem for position:sticky. Changing the compositing
trigger
> in
> non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html
> to:
>
> .composited {
> backface-visibility: hidden;
> isolation: isolate;
> }
>
> will cause the position:absolute element to appear to scroll, even though it
> should not,
> because its containing block is the body element.
>
> This will only reproduce for position:absolute on low-DPI screens, because
> otherwise
> kCompositingReasonOutOfFlowClipping kicks in. I don't know why exactly that
> trigger
> is limited to high-DPI screens but I think it should not be.
>
> Proposed fix:
>
> 1. Get rid of kOverflowScroll trigger in the conditional here
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> (fixes bug with position:absolute)
>
> Promote position:sticky elements with compositing scrolling ancestors (maybe
> unless the constraints don't involve
> the scroller). This can be detected by using the
> has_composited_scrolling_ancestor_
> variable in CompositingRequirementsUpdater.
Thanks Chris for the feedback! We do have the position:absolute problem as you
mentioned and the proposed fix works. Maybe I should add another patch to
address this problem instead of fixing it in this patch?
To composite the position:sticky element, it seems that we shouldn't rely on
"has_composited_scrolling_ancestor_" because its ancestor is composited but
scrolled on the main thread, meaning that the ancestor doesn't have a composited
scrolling layer. The proposed fix is adding another member
"has_composited_ancestor_" into RecursionData to handle this situation. WDYT?
Thanks!
On 2017/05/01 15:35:04, yigu wrote:
> On 2017/04/29 01:43:00, chrishtr wrote:
> > This is not just a problem for position:sticky. Changing the compositing
> trigger
> > in
> >
non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html
> > to:
> >
> > .composited {
> > backface-visibility: hidden;
> > isolation: isolate;
> > }
> >
> > will cause the position:absolute element to appear to scroll, even though it
> > should not,
> > because its containing block is the body element.
> >
> > This will only reproduce for position:absolute on low-DPI screens, because
> > otherwise
> > kCompositingReasonOutOfFlowClipping kicks in. I don't know why exactly that
> > trigger
> > is limited to high-DPI screens but I think it should not be.
> >
> > Proposed fix:
> >
> > 1. Get rid of kOverflowScroll trigger in the conditional here
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> > (fixes bug with position:absolute)
> >
> > Promote position:sticky elements with compositing scrolling ancestors (maybe
> > unless the constraints don't involve
> > the scroller). This can be detected by using the
> > has_composited_scrolling_ancestor_
> > variable in CompositingRequirementsUpdater.
>
> Thanks Chris for the feedback! We do have the position:absolute problem as you
> mentioned and the proposed fix works. Maybe I should add another patch to
> address this problem instead of fixing it in this patch?
>
> To composite the position:sticky element, it seems that we shouldn't rely on
> "has_composited_scrolling_ancestor_" because its ancestor is composited but
> scrolled on the main thread, meaning that the ancestor doesn't have a
composited
> scrolling layer. The proposed fix is adding another member
> "has_composited_ancestor_" into RecursionData to handle this situation. WDYT?
> Thanks!
When checking composited ancestors we should exclude the root layer which I
didn't in the last patch. PTAL. Thanks.
On 2017/05/02 17:19:53, yigu wrote:
> On 2017/05/01 15:35:04, yigu wrote:
> > On 2017/04/29 01:43:00, chrishtr wrote:
> > > This is not just a problem for position:sticky. Changing the compositing
> > trigger
> > > in
> > >
> non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html
> > > to:
> > >
> > > .composited {
> > > backface-visibility: hidden;
> > > isolation: isolate;
> > > }
> > >
> > > will cause the position:absolute element to appear to scroll, even though
it
> > > should not,
> > > because its containing block is the body element.
> > >
> > > This will only reproduce for position:absolute on low-DPI screens, because
> > > otherwise
> > > kCompositingReasonOutOfFlowClipping kicks in. I don't know why exactly
that
> > > trigger
> > > is limited to high-DPI screens but I think it should not be.
> > >
> > > Proposed fix:
> > >
> > > 1. Get rid of kOverflowScroll trigger in the conditional here
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> > > (fixes bug with position:absolute)
> > >
> > > Promote position:sticky elements with compositing scrolling ancestors
(maybe
> > > unless the constraints don't involve
> > > the scroller). This can be detected by using the
> > > has_composited_scrolling_ancestor_
> > > variable in CompositingRequirementsUpdater.
> >
> > Thanks Chris for the feedback! We do have the position:absolute problem as
you
> > mentioned and the proposed fix works. Maybe I should add another patch to
> > address this problem instead of fixing it in this patch?
> >
> > To composite the position:sticky element, it seems that we shouldn't rely on
> > "has_composited_scrolling_ancestor_" because its ancestor is composited but
> > scrolled on the main thread, meaning that the ancestor doesn't have a
> composited
> > scrolling layer. The proposed fix is adding another member
> > "has_composited_ancestor_" into RecursionData to handle this situation.
WDYT?
> > Thanks!
>
> When checking composited ancestors we should exclude the root layer which I
> didn't in the last patch. PTAL. Thanks.
Actually, trying to composite the sticky element from
CompositingRequirementsUpdater may be difficult. In the layout test, we have
something like:
<div id='a' class='scroller'>
<div id='b' style='will-change:transform'>
<div id='sticky'></div>
</div>
</div>.
When calling UpdateRecursive in this example, the processing order is "a,
sticky, b" so we cannot know that the sticky element has a composited ancestor.
Changing the style of b to 'backface-visibility:hidden;' will have the correct
processing order "a, b, sticky" because we no longer create a new stacking
content for b. Any suggestions? Thanks!
FYI I haven't forgotten about this CL, just need more time to think it through.
On 2017/05/02 at 19:51:58, yigu wrote:
> On 2017/05/02 17:19:53, yigu wrote:
> > On 2017/05/01 15:35:04, yigu wrote:
> > > On 2017/04/29 01:43:00, chrishtr wrote:
> > > > This is not just a problem for position:sticky. Changing the compositing
> > > trigger
> > > > in
> > > >
> >
non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html
> > > > to:
> > > >
> > > > .composited {
> > > > backface-visibility: hidden;
> > > > isolation: isolate;
> > > > }
> > > >
> > > > will cause the position:absolute element to appear to scroll, even
though it
> > > > should not,
> > > > because its containing block is the body element.
> > > >
> > > > This will only reproduce for position:absolute on low-DPI screens,
because
> > > > otherwise
> > > > kCompositingReasonOutOfFlowClipping kicks in. I don't know why exactly
that
> > > > trigger
> > > > is limited to high-DPI screens but I think it should not be.
> > > >
> > > > Proposed fix:
> > > >
> > > > 1. Get rid of kOverflowScroll trigger in the conditional here
> > > >
> > >
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> > > > (fixes bug with position:absolute)
> > > >
> > > > Promote position:sticky elements with compositing scrolling ancestors
(maybe
> > > > unless the constraints don't involve
> > > > the scroller). This can be detected by using the
> > > > has_composited_scrolling_ancestor_
> > > > variable in CompositingRequirementsUpdater.
> > >
> > > Thanks Chris for the feedback! We do have the position:absolute problem as
you
> > > mentioned and the proposed fix works. Maybe I should add another patch to
> > > address this problem instead of fixing it in this patch?
> > >
> > > To composite the position:sticky element, it seems that we shouldn't rely
on
> > > "has_composited_scrolling_ancestor_" because its ancestor is composited
but
> > > scrolled on the main thread, meaning that the ancestor doesn't have a
> > composited
> > > scrolling layer. The proposed fix is adding another member
> > > "has_composited_ancestor_" into RecursionData to handle this situation.
WDYT?
> > > Thanks!
> >
> > When checking composited ancestors we should exclude the root layer which I
> > didn't in the last patch. PTAL. Thanks.
>
> Actually, trying to composite the sticky element from
CompositingRequirementsUpdater may be difficult. In the layout test, we have
something like:
> <div id='a' class='scroller'>
> <div id='b' style='will-change:transform'>
> <div id='sticky'></div>
> </div>
> </div>.
>
> When calling UpdateRecursive in this example, the processing order is "a,
sticky, b" so we cannot know that the sticky element has a composited ancestor.
Changing the style of b to 'backface-visibility:hidden;' will have the correct
processing order "a, b, sticky" because we no longer create a new stacking
content for b. Any suggestions? Thanks!
Isn't it the other way around? UpdateRecursive follows the recursive order of
stacking contexts. if 'b' has will-change:transform, then
a will call UpdateRecursive next on 'b', which will then call UpdateRecursive on
'c'.
More generally, I think what is needed is a compositing trigger when an element
can move relative to its
paint invalidation container (see LayoutObject::ContainerForPaintInvalidation)
due to a scrolling effect
outside of that container. In the case of a position:sticky element, the
movement would be the sticky
counter-movement on scroll; for a position:absolute or position:fixed
element it would be if it has a different scrolling ancestor than the paint
invalidation container.
Since CompositingRequirementsUpdater follows stacking context recursion order,
it should always be possible
to find out the paint invalidation container of a PaintLayer during that walk.
This can then be used to decide
whether the PaintLayer moves with respect to that paint invalidation container.
e.g. you can check
whether the paint invalidation container is between a position:sticky element
and the scroll it sticks to,
or whether the paint invalidation container has a different scrolling ancestor
than a position:absolute or
position:fixed element.
Description was changed from ========== Fix the bug that non composited sticky element may not be correctly invalidated. We should have a paint invalidation for the non-composited element if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; third_party/WebKit/LayoutTests/paint/invalidation/compositing/non-composited-sticky-element-repaint.html BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix the bug that non composited sticky element may not be correctly invalidated. We should have a paint invalidation for the non-composited element if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
On 2017/05/05 18:16:31, chrishtr wrote:
> On 2017/05/02 at 19:51:58, yigu wrote:
> > On 2017/05/02 17:19:53, yigu wrote:
> > > On 2017/05/01 15:35:04, yigu wrote:
> > > > On 2017/04/29 01:43:00, chrishtr wrote:
> > > > > This is not just a problem for position:sticky. Changing the
compositing
> > > > trigger
> > > > > in
> > > > >
> > >
> non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html
> > > > > to:
> > > > >
> > > > > .composited {
> > > > > backface-visibility: hidden;
> > > > > isolation: isolate;
> > > > > }
> > > > >
> > > > > will cause the position:absolute element to appear to scroll, even
> though it
> > > > > should not,
> > > > > because its containing block is the body element.
> > > > >
> > > > > This will only reproduce for position:absolute on low-DPI screens,
> because
> > > > > otherwise
> > > > > kCompositingReasonOutOfFlowClipping kicks in. I don't know why exactly
> that
> > > > > trigger
> > > > > is limited to high-DPI screens but I think it should not be.
> > > > >
> > > > > Proposed fix:
> > > > >
> > > > > 1. Get rid of kOverflowScroll trigger in the conditional here
> > > > >
> > > >
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> > > > > (fixes bug with position:absolute)
> > > > >
> > > > > Promote position:sticky elements with compositing scrolling ancestors
> (maybe
> > > > > unless the constraints don't involve
> > > > > the scroller). This can be detected by using the
> > > > > has_composited_scrolling_ancestor_
> > > > > variable in CompositingRequirementsUpdater.
> > > >
> > > > Thanks Chris for the feedback! We do have the position:absolute problem
as
> you
> > > > mentioned and the proposed fix works. Maybe I should add another patch
to
> > > > address this problem instead of fixing it in this patch?
> > > >
> > > > To composite the position:sticky element, it seems that we shouldn't
rely
> on
> > > > "has_composited_scrolling_ancestor_" because its ancestor is composited
> but
> > > > scrolled on the main thread, meaning that the ancestor doesn't have a
> > > composited
> > > > scrolling layer. The proposed fix is adding another member
> > > > "has_composited_ancestor_" into RecursionData to handle this situation.
> WDYT?
> > > > Thanks!
> > >
> > > When checking composited ancestors we should exclude the root layer which
I
> > > didn't in the last patch. PTAL. Thanks.
> >
> > Actually, trying to composite the sticky element from
> CompositingRequirementsUpdater may be difficult. In the layout test, we have
> something like:
> > <div id='a' class='scroller'>
> > <div id='b' style='will-change:transform'>
> > <div id='sticky'></div>
> > </div>
> > </div>.
> >
> > When calling UpdateRecursive in this example, the processing order is "a,
> sticky, b" so we cannot know that the sticky element has a composited
ancestor.
> Changing the style of b to 'backface-visibility:hidden;' will have the correct
> processing order "a, b, sticky" because we no longer create a new stacking
> content for b. Any suggestions? Thanks!
>
> Isn't it the other way around? UpdateRecursive follows the recursive order of
> stacking contexts. if 'b' has will-change:transform, then
> a will call UpdateRecursive next on 'b', which will then call UpdateRecursive
on
> 'c'.
>
> More generally, I think what is needed is a compositing trigger when an
element
> can move relative to its
> paint invalidation container (see
LayoutObject::ContainerForPaintInvalidation)
> due to a scrolling effect
> outside of that container. In the case of a position:sticky element, the
> movement would be the sticky
> counter-movement on scroll; for a position:absolute or position:fixed
> element it would be if it has a different scrolling ancestor than the paint
> invalidation container.
>
> Since CompositingRequirementsUpdater follows stacking context recursion order,
> it should always be possible
> to find out the paint invalidation container of a PaintLayer during that walk.
> This can then be used to decide
> whether the PaintLayer moves with respect to that paint invalidation
container.
> e.g. you can check
> whether the paint invalidation container is between a position:sticky element
> and the scroll it sticks to,
> or whether the paint invalidation container has a different scrolling ancestor
> than a position:absolute or
> position:fixed element.
Hi Chris. In the example from #20:
<div id='a' class='scroller'>
<div id='b' style='will-change:transform'>
<div id='sticky'></div>
</div>
</div>,
we call CompositingRequirementsUpdater::UpdateRecursive in the order a, b
sticky.
However, because we create a stacking context for b, the recursion ends in the
order a, sticky, b. My previous fix was near the end of UpdateRecursive so it
didn't propagate the correct value has_composited_non_root_ancestor of 'b' to
b's child 'sticky'. The new fix should trigger the compositing correctly.
I also tested the ContainerForPaintInvalidation as you suggested. If 'b' is
composited due to "will-change", 'sticky''s container would be 'b'. If 'b' is
composited due to "backface-visibility", 'sticky''s container would be
'document'.
When adding an absolute positioned element as a child of 'b', the new element's
container is the same as the 'sticky''s.
https://codereview.chromium.org/2845613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:382: reasons_to_composite |= kCompositingReasonScrollDependentPosition; You don't need current_recursion_data.has_composited_non_root_ancestor_. Instead: if (layer->SticksToScroller() && layer->AncestorScrollingLayer() && current_recursion_data.compositing_ancestor_ && // It's actually only needed if the ancestor scrolling layer of compositing_ancestor_ is below // the scroller |layer| sticks to, but that is more complicated to compute. current_recursion_data.compositing_ancestor_->AncestorScrollingLayer() && !current_recursion_data.compositing_ancestor_->AncestorScrollingLayer()->IsRoot()) { reasons_to_composite |= kCompositingReasonScrollDependentPosition; }
Thanks Chris for the feedback! Logic updated. https://codereview.chromium.org/2845613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:382: reasons_to_composite |= kCompositingReasonScrollDependentPosition; On 2017/05/10 17:48:44, chrishtr wrote: > You don't need current_recursion_data.has_composited_non_root_ancestor_. > > Instead: > > if (layer->SticksToScroller() && > layer->AncestorScrollingLayer() && > current_recursion_data.compositing_ancestor_ && > // It's actually only needed if the ancestor scrolling layer of > compositing_ancestor_ is below > // the scroller |layer| sticks to, but that is more complicated to compute. > current_recursion_data.compositing_ancestor_->AncestorScrollingLayer() && > > !current_recursion_data.compositing_ancestor_->AncestorScrollingLayer()->IsRoot()) > { > reasons_to_composite |= kCompositingReasonScrollDependentPosition; > } Done.
https://codereview.chromium.org/2845613002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:379: current_recursion_data.compositing_ancestor_->AncestorScrollingLayer() && Please add the comments explaining that this is not exact and over-promotes sometimes if compositing_ancestor_ is not under the same scroller as the sticky element.
Thanks Chris. https://codereview.chromium.org/2845613002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:379: current_recursion_data.compositing_ancestor_->AncestorScrollingLayer() && On 2017/05/10 19:29:36, chrishtr wrote: > Please add the comments explaining that this is not exact and over-promotes > sometimes > if compositing_ancestor_ is not under the same scroller as the sticky element. Done.
lgtm
The CQ bit was checked by yigu@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Hi Chris. I've updated the test because the previous one failed spv2. If failed because spv2 currently has a culling bug (crbug.com/718971): out of flow positioned elements may disappear after several amounts of scrolling. So I updated the ref test to make it scroll within a small range to avoid the absolute positioned element from disappearing.
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2845613002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:279: compositing_reason_finder_.DirectReasons(layer, ignore_lcd_text); I think we should set ignore_lcd_text appropriately here based on compositing ancestors, rather than add separate promotion trigger logic. https://codereview.chromium.org/2845613002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:376: layer->AncestorOverflowLayer()->ScrollsOverflow() && This seems to be duplicating the promotion logic in CompositingReasonFinder::NonStyleDeterminedDirectReasons, isn't the right change to make sure that we pass ignore_lcd_text = true to NonStyleDeterminedReasons when we have a composited ancestor? https://codereview.chromium.org/2845613002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:382: !current_recursion_data.compositing_ancestor_->AncestorScrollingLayer() If we have a composited ancestor scrolling layer then ignore_lcd_text should be true on line 279 leading to promotion of sticky elements.
Description was changed from ========== Fix the bug that non composited sticky element may not be correctly invalidated. We should have a paint invalidation for the non-composited element if it has ancestor that is composited but requires repaint on scroll. TEST=third_party/WebKit/LayoutTests/compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix the bug that sticky element may not be correctly invalidated due to non-promotion. When a non composited sticky element is nested in a compositied ancestor which requires repaint on scroll, the sticky element should also be composited to have correct paint invalidation. TEST=compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Fix the bug that sticky element may not be correctly invalidated due to non-promotion. When a non composited sticky element is nested in a compositied ancestor which requires repaint on scroll, the sticky element should also be composited to have correct paint invalidation. TEST=compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix the bug that sticky element may not be correctly invalidated due to non-promotion. When a non composited sticky element is nested in a compositied ancestor which requires repaint on scroll, the sticky element should also be composited to have correct paint invalidation. TEST=compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by yigu@chromium.org
I've updated the patch to set ignore_lcd_text. PTAL. Thanks! https://codereview.chromium.org/2845613002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:279: compositing_reason_finder_.DirectReasons(layer, ignore_lcd_text); On 2017/05/12 18:00:46, flackr wrote: > I think we should set ignore_lcd_text appropriately here based on compositing > ancestors, rather than add separate promotion trigger logic. Done. https://codereview.chromium.org/2845613002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:376: layer->AncestorOverflowLayer()->ScrollsOverflow() && On 2017/05/12 18:00:46, flackr wrote: > This seems to be duplicating the promotion logic in > CompositingReasonFinder::NonStyleDeterminedDirectReasons, isn't the right change > to make sure that we pass ignore_lcd_text = true to NonStyleDeterminedReasons > when we have a composited ancestor? Done. https://codereview.chromium.org/2845613002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:382: !current_recursion_data.compositing_ancestor_->AncestorScrollingLayer() On 2017/05/12 18:00:46, flackr wrote: > If we have a composited ancestor scrolling layer then ignore_lcd_text should be > true on line 279 leading to promotion of sticky elements. It's possible that a sticky element have a composited ancestor but doesn't have ancestor scrolling layer. To correct set ignore_lcd_text, I added another variable indicating that it has composited ancestor.
Working around the missing invalidation by promoting feels like we're making an inconsistent decision. I.e. if we want to promote scroll positioned elements to avoid repainting their compositing container even when it loses LCD text then we would promote even the ones contained by the root layer because they also cause invalidations and repaints. https://codereview.chromium.org/2845613002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:281: (layer->SticksToScroller() && For clarity, can you calculate a named local bool for this, i.e. bool moves_with_respect_to_compositing_ancestor = ... It seems like we should really be checking if we have already lost lcd text. This would require tracking if we think the current compositing ancestor layer meets the requirements (i.e. opaque, integer transform, etc). Add a TODO for this. https://codereview.chromium.org/2845613002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:397: child_recursion_data.has_composited_non_root_ancestor_ = true; Couldn't we determine this by checking !child_recursion_data.compositing_ancestor_->IsRootLayer()?
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by yigu@chromium.org
PTAL. Thanks. https://codereview.chromium.org/2845613002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:281: (layer->SticksToScroller() && On 2017/05/17 20:18:57, flackr wrote: > For clarity, can you calculate a named local bool for this, i.e. > bool moves_with_respect_to_compositing_ancestor = ... > > It seems like we should really be checking if we have already lost lcd text. > This would require tracking if we think the current compositing ancestor layer > meets the requirements (i.e. opaque, integer transform, etc). Add a TODO for > this. Done. https://codereview.chromium.org/2845613002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:397: child_recursion_data.has_composited_non_root_ancestor_ = true; On 2017/05/17 20:18:57, flackr wrote: > Couldn't we determine this by checking > !child_recursion_data.compositing_ancestor_->IsRootLayer()? Done.
LGTM with nits. https://codereview.chromium.org/2845613002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:276: layer->SticksToScroller(); Sorry, can you put both parts of the expression to this calculation here (and move the comment up)? This moves with respect to compositing ancestor if it has a non-root compositing ancestor and sticks to scroller. https://codereview.chromium.org/2845613002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:282: // layer meets the requirements (i.e. opaque, integer transform, etc). From your discussions with Chris, we also have to promote right now to work around a bug of not being able to invalidate the ancestor after updating the sticky layer position, right? (can you add this to the comment).
The CQ bit was checked by yigu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2845613002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2845613002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:276: layer->SticksToScroller(); On 2017/05/18 18:13:14, flackr wrote: > Sorry, can you put both parts of the expression to this calculation here (and > move the comment up)? This moves with respect to compositing ancestor if it has > a non-root compositing ancestor and sticks to scroller. Done. https://codereview.chromium.org/2845613002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:282: // layer meets the requirements (i.e. opaque, integer transform, etc). On 2017/05/18 18:13:14, flackr wrote: > From your discussions with Chris, we also have to promote right now to work > around a bug of not being able to invalidate the ancestor after updating the > sticky layer position, right? (can you add this to the comment). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yigu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2845613002/#ps280001 (title: "Address comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1495144669239880,
"parent_rev": "23cf82b8915a04b3c6c83717902b2bfe0c885dde", "commit_rev":
"e44021ea6e3e18bc59854b4c8d30999f1fc4379f"}
Message was sent while issue was closed.
Description was changed from ========== Fix the bug that sticky element may not be correctly invalidated due to non-promotion. When a non composited sticky element is nested in a compositied ancestor which requires repaint on scroll, the sticky element should also be composited to have correct paint invalidation. TEST=compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix the bug that sticky element may not be correctly invalidated due to non-promotion. When a non composited sticky element is nested in a compositied ancestor which requires repaint on scroll, the sticky element should also be composited to have correct paint invalidation. TEST=compositing/overflow/non-composited-sticky-element-in-main-thread-scrolled-composited-ancestor.html; BUG=698358 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2845613002 Cr-Commit-Position: refs/heads/master@{#472947} Committed: https://chromium.googlesource.com/chromium/src/+/e44021ea6e3e18bc59854b4c8d30... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/e44021ea6e3e18bc59854b4c8d30... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
