Under-invalidation of paint properties when changing containing block |
|||||
Issue description
Currently, we force update of paint property subtrees if an object changes
a property tree node that it itself owns, or if paintOffset changes. But
we also need to do so if containing block hierarchies change, in such
a way that paintOffset does not change.
I think this can be fixed by forcing subtree update if any of the border
box property tree state properties change.
Failing example:
<!DOCTYPE html>
<script>
if (window.testRunner) {
window.enablePixelTesting = true;
window.testRunner.waitUntilDone();
}
function repaintTest() {
console.log("hi");
scroller.scrollTop = 200;
scrollpanel.style.position = "absolute";
}
</script>
<script type="text/javascript" src="resources/text-based-repaint.js"></script>
<script type="text/javascript" src="../../resources/run-after-layout-and-paint.js"></script>
<style>
* {
margin: 0px;
}
#scroller {
width: 100px;
overflow: scroll;
height: 200px;
}
#ul {
position: relative;
}
#scrollpanel {
background: green;
position: inherit;
top: 0px;
left: 0px;
width: 100px; height: 100px;
}
</style>
<body onload="runAfterLayoutAndPaint(runRepaintTest);">
<div id="scroller">
<div id="scrollpanel">
<div class="container">
<div id="ul">
<div style="background-color: red; width: 100px; height:100px;"></div>
</div>
</div>
</div>
<div style="width: 10px; height: 1000px; background: green"></div>
</div>
</body>
,
Dec 15 2016
,
Dec 15 2016
,
Dec 15 2016
I'll check. The fix is pretty simple.
,
Dec 15 2016
I think that is a different bug. This is specific to local border box changes. To exhibit this bug, we have to be very careful to make sure paintOffset changes don't trigger a subtree update.
,
Dec 15 2016
I think virtual/spinvalidation/fast/layers/remove-layer-with-nested-stacking.html is somehow related, though it's not about local border box changes. It's about a subtree moved in the layout tree when a float object is moved float property causing anonymous blocks created and subtrees in continuations moved to new parents. I wonder if that case and this bug can be treated in the same way: add LayoutObject::needsPaintPropertyUpdateSubtree flag, set the flag in LayoutObjectChildList::insertChildNode(), and force subtree paint property tree update during PrePaintTreeWalk. Wdyt?
,
Dec 15 2016
It seems that in the #6 way we can avoid subtree update on local border box changes without layout tree restructure.
,
Dec 15 2016
It would be nice if we could avoid Layout/CSS changes needing to know the difference between local and subtree paint property updates, because the distinction is pretty subtle. I don't have another suggestion on how to handle that situation though.
,
Dec 15 2016
How about ObjectPaintProperties::m_previousParentObject? During paint property update, if the parent changes, force subtree update.
,
Dec 15 2016
I think that is a good idea. @Chris, can you think of any cases that would miss?
,
Dec 15 2016
Just think of it a bit more, and found a problem that we may need to save m_previousParentObject when m_paintProperties is null. I think if we use subtreeNeedsPaintPropertyUpdate() we can limit its usage to direct reasons of subtree update only. With it, we can remove FrameView::shouldInvalidateAllPaintAndPaintProperties() which was just added in https://codereview.chromium.org/2575653002/ for printing, and set/use the new flag on LayoutView.
,
Dec 16 2016
Just found that what I wanted to fix (moved subtree) is different from this bug (containing block change because of 'position' change). They may need different solutions.
,
Dec 16 2016
WIP patch, going to wait a little to see if other positioning bugs come up: https://codereview.chromium.org/2584653002
,
Dec 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/81c78e614b8a82160e2e0403011eba4701d09784 commit 81c78e614b8a82160e2e0403011eba4701d09784 Author: wangxianzhu <wangxianzhu@chromium.org> Date: Tue Dec 27 17:57:17 2016 [SPInvalidation] Force subtree paint property update on css position change CSS position change may not affect paint properties of the object itself, but may affect paint properties of descendants. BUG= 674623 TEST=All/ScrollAnchorTest.ExcludeAbsolutePositionThatSticksToViewport/0 TEST=All/PaintPropertyTreeBuilderTest.PositionChangeUpdateDescendantProperties/* CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2604743002 Cr-Commit-Position: refs/heads/master@{#440761} [modify] https://crrev.com/81c78e614b8a82160e2e0403011eba4701d09784/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp [modify] https://crrev.com/81c78e614b8a82160e2e0403011eba4701d09784/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
,
Dec 28 2016
,
Dec 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/346b1c326cb8c8c2978de61294338d3575708a86 commit 346b1c326cb8c8c2978de61294338d3575708a86 Author: chrishtr <chrishtr@chromium.org> Date: Wed Dec 28 23:47:32 2016 Remove paintOffsetTranslation for fixed-position elements. Thus only transformed elements now paint with transform. The concept of painting fixed-position elements with transform was introduced in https://codereview.chromium.org/1110653003, to help with paint invalidation of SPv1. However we ended up invalidating whole subtrees on scroll in v1, so this is not necessary. Also added invalidation of paint properties when changing compositing mode, since this can affect whether paintsWithTransform changes its behavior. BUG= 674623 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2570423003 Cr-Commit-Position: refs/heads/master@{#440906} [modify] https://crrev.com/346b1c326cb8c8c2978de61294338d3575708a86/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2 [modify] https://crrev.com/346b1c326cb8c8c2978de61294338d3575708a86/third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp [modify] https://crrev.com/346b1c326cb8c8c2978de61294338d3575708a86/third_party/WebKit/Source/core/paint/PaintLayer.cpp [modify] https://crrev.com/346b1c326cb8c8c2978de61294338d3575708a86/third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp [modify] https://crrev.com/346b1c326cb8c8c2978de61294338d3575708a86/third_party/WebKit/Source/core/paint/PaintLayerPainter.h [modify] https://crrev.com/346b1c326cb8c8c2978de61294338d3575708a86/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp [modify] https://crrev.com/346b1c326cb8c8c2978de61294338d3575708a86/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by wangxianzhu@chromium.org
, Dec 15 2016