Incorrect invalidation on Google Forum (issue 241113004)

0 views
Skip to first unread message

jchaf...@chromium.org

unread,
Apr 17, 2014, 4:03:22 PM4/17/14
to le...@chromium.org, blink-...@chromium.org, bemjb+r...@chromium.org, dsin...@chromium.org, zol...@webkit.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, p...@chromium.org, rune+...@opera.com, esp...@chromium.org
Reviewers: Levi,

Description:
Incorrect invalidation on Google Forum

The issue was that we would generate incremental invalidations for
a positioned element instead of a full invalidation. This is
because the repaint rect didn't move when intersected with the
overflow clip.

The fix is to force positioned children of a positioned element
that required a positioned movement layout to be relaid out. This
is similar to what was done in
https://codereview.chromium.org/192863004. To achieve the previous
fix, we make positioned movement layout do a simplified layout,
which simplifies the code by removing the possibility of doing
both independently.

BUG=360524

Please review this at https://codereview.chromium.org/241113004/

SVN Base: svn://svn.chromium.org/blink/trunk

Affected files (+61, -19 lines):
A LayoutTests/fast/repaint/positioned-list-offset-change-repaint.html
A
LayoutTests/fast/repaint/positioned-list-offset-change-repaint-expected.txt
M Source/core/rendering/RenderBlock.cpp
M Source/core/rendering/RenderObject.cpp
M Source/core/rendering/style/RenderStyleConstants.h


Index:
LayoutTests/fast/repaint/positioned-list-offset-change-repaint-expected.txt
diff --git
a/LayoutTests/fast/repaint/positioned-list-offset-change-repaint-expected.txt
b/LayoutTests/fast/repaint/positioned-list-offset-change-repaint-expected.txt
new file mode 100644
index
0000000000000000000000000000000000000000..267a82533be1dfc498a99f5331826058045e8c02
--- /dev/null
+++
b/LayoutTests/fast/repaint/positioned-list-offset-change-repaint-expected.txt
@@ -0,0 +1,8 @@
+This test checks that the shifted image is correctly invalidated.
+The image should be fully invalidated.
+
+(repaint rects
+ (rect 8 64 114 237)
+ (rect 8 64 214 237)
+)
+
Index: LayoutTests/fast/repaint/positioned-list-offset-change-repaint.html
diff --git
a/LayoutTests/fast/repaint/positioned-list-offset-change-repaint.html
b/LayoutTests/fast/repaint/positioned-list-offset-change-repaint.html
new file mode 100644
index
0000000000000000000000000000000000000000..c95c10caf7ffcad04c97a9cc068126a65f553d24
--- /dev/null
+++ b/LayoutTests/fast/repaint/positioned-list-offset-change-repaint.html
@@ -0,0 +1,49 @@
+<!DOCTYPE html>
+<html>
+<style>
+ul {
+ width: 978px;
+ position: absolute;
+ left: -2070px;
+}
+
+li {
+ position: absolute;
+ left: -690px;
+}
+
+#container {
+ height: 400px;;
+ position: relative;
+ overflow: hidden;
+}
+</style>
+<body>
+<div>This test checks that moving a positioned element with positioned
children invalidates correctly.</div>
+<div>The image below should be completely invalidated.</div>
+<div id="container">
+ <ul id="list">
+ <li>
+ <img src="resources/apple.jpg">
+ </li>
+ </ul>
+</div>
+
+<script src="resources/text-based-repaint.js"></script>
+<script>
+function setUp() {
+ var elSlider = document.getElementById("list");
+ elSlider.style.left = "590px";
+ elSlider.offsetLeft;
+
+ runRepaintTest();
+}
+
+function repaintTest() {
+ var elSlider = document.getElementById("list");
+ elSlider.style.left = "690px";
+};
+
+window.addEventListener("load", setUp, false);
+</script>
+</body></html>
Index: Source/core/rendering/RenderBlock.cpp
diff --git a/Source/core/rendering/RenderBlock.cpp
b/Source/core/rendering/RenderBlock.cpp
index
c75eeed13d3c4a2602cfca85c7476431156d6428..60e03ab2a87ca234916b4dda829c7781fe19b7c5
100644
--- a/Source/core/rendering/RenderBlock.cpp
+++ b/Source/core/rendering/RenderBlock.cpp
@@ -1523,7 +1523,7 @@ void RenderBlock::simplifiedNormalFlowLayout()

bool RenderBlock::simplifiedLayout()
{
- if ((!posChildNeedsLayout() && !needsSimplifiedNormalFlowLayout()) ||
normalChildNeedsLayout() || selfNeedsLayout())
+ if ((!posChildNeedsLayout() && !(needsSimplifiedNormalFlowLayout() ||
needsPositionedMovementLayout())) || normalChildNeedsLayout() ||
selfNeedsLayout())
return false;


@@ -1551,8 +1551,8 @@ bool RenderBlock::simplifiedLayout()
// relative positioned container. So if we can have fixed pos
objects in our positioned objects list check if any of them
// are statically positioned and thus need to move with their
absolute ancestors.
bool canContainFixedPosObjects = canContainFixedPositionObjects();
- if (posChildNeedsLayout() || canContainFixedPosObjects)
- layoutPositionedObjects(false, !posChildNeedsLayout() &&
canContainFixedPosObjects ? LayoutOnlyFixedPositionedObjects :
DefaultLayout);
+ if (posChildNeedsLayout() || needsPositionedMovementLayout() ||
canContainFixedPosObjects)
+ layoutPositionedObjects(false,
needsPositionedMovementLayout() ? ForcedLayoutAfterContainingBlockMoved :
(!posChildNeedsLayout() && canContainFixedPosObjects ?
LayoutOnlyFixedPositionedObjects : DefaultLayout));

// Recompute our overflow information.
// FIXME: We could do better here by computing a temporary
overflow object from layoutPositionedObjects and only
@@ -1660,11 +1660,6 @@ void RenderBlock::layoutPositionedObjects(bool
relayoutChildren, PositionedLayou
if (!r->needsLayout())
r->markForPaginationRelayoutIfNeeded(layoutScope);

- // We don't have to do a full layout. We just have to update our
position. Try that first. If we have shrink-to-fit width
- // and we hit the available width constraint, the layoutIfNeeded()
will catch it and do a full layout.
- if (r->needsPositionedMovementLayoutOnly() &&
r->tryLayoutDoingPositionedMovementOnly())
- r->clearNeedsLayout();
-
// If we are paginated or in a line grid, go ahead and compute a
vertical position for our object now.
// If it's wrong we'll lay out again.
LayoutUnit oldLogicalTop = 0;
Index: Source/core/rendering/RenderObject.cpp
diff --git a/Source/core/rendering/RenderObject.cpp
b/Source/core/rendering/RenderObject.cpp
index
d79cd3f75902c5e8c1d3aae705e877adb5771435..ff6eec93a3eeda5bd5817ab7371deafe366cb7f5
100644
--- a/Source/core/rendering/RenderObject.cpp
+++ b/Source/core/rendering/RenderObject.cpp
@@ -1857,8 +1857,6 @@ StyleDifference
RenderObject::adjustStyleDifference(StyleDifference diff, unsign
diff = StyleDifferenceLayout; // FIXME: Do this for now
since SimplifiedLayout cannot handle updating floating objects lists.
else if (diff < StyleDifferenceLayoutPositionedMovementOnly)
diff = StyleDifferenceSimplifiedLayout;
- else if (diff < StyleDifferenceSimplifiedLayout)
- diff =
StyleDifferenceSimplifiedLayoutAndPositionedMovement;
} else if (diff < StyleDifferenceRecompositeLayer)
diff = StyleDifferenceRecompositeLayer;
}
@@ -1987,10 +1985,7 @@ void RenderObject::setStyle(PassRefPtr<RenderStyle>
style)
setNeedsLayoutAndPrefWidthsRecalc();
else if (updatedDiff ==
StyleDifferenceLayoutPositionedMovementOnly)
setNeedsPositionedMovementLayout();
- else if (updatedDiff ==
StyleDifferenceSimplifiedLayoutAndPositionedMovement) {
- setNeedsPositionedMovementLayout();
- setNeedsSimplifiedNormalFlowLayout();
- } else if (updatedDiff == StyleDifferenceSimplifiedLayout)
+ else if (updatedDiff == StyleDifferenceSimplifiedLayout)
setNeedsSimplifiedNormalFlowLayout();
}

@@ -2139,9 +2134,6 @@ void RenderObject::styleDidChange(StyleDifference
diff, const RenderStyle* oldSt
setNeedsLayoutAndPrefWidthsRecalc();
else
setNeedsSimplifiedNormalFlowLayout();
- } else if (diff ==
StyleDifferenceSimplifiedLayoutAndPositionedMovement) {
- setNeedsPositionedMovementLayout();
- setNeedsSimplifiedNormalFlowLayout();
} else if (diff == StyleDifferenceLayoutPositionedMovementOnly)
setNeedsPositionedMovementLayout();

Index: Source/core/rendering/style/RenderStyleConstants.h
diff --git a/Source/core/rendering/style/RenderStyleConstants.h
b/Source/core/rendering/style/RenderStyleConstants.h
index
5efade95d908501ebf1c0578740ca485c7159bc8..021c9a3f34ced88a31af425a3bc9368f648ffe70
100644
--- a/Source/core/rendering/style/RenderStyleConstants.h
+++ b/Source/core/rendering/style/RenderStyleConstants.h
@@ -53,7 +53,6 @@ enum PrintColorAdjust {
// - StyleDifferenceRepaintLayer - The layer and its descendant layers
need to be repainted.
// - StyleDifferenceLayoutPositionedMovementOnly - Only the position of
this positioned object has been updated.
// - StyleDifferenceSimplifiedLayout - Only overflow needs to be
recomputed.
-// - StyleDifferenceSimplifiedLayoutAndPositionedMovement - Both
positioned movement and simplified layout updates are required.
// - StyleDifferenceLayout - A full layout is required.
enum StyleDifference {
StyleDifferenceEqual,
@@ -62,7 +61,6 @@ enum StyleDifference {
StyleDifferenceRepaintLayer,
StyleDifferenceLayoutPositionedMovementOnly,
StyleDifferenceSimplifiedLayout,
- StyleDifferenceSimplifiedLayoutAndPositionedMovement,
StyleDifferenceLayout
};



le...@chromium.org

unread,
Apr 17, 2014, 4:35:55 PM4/17/14
to jchaf...@chromium.org, blink-...@chromium.org, bemjb+r...@chromium.org, dsin...@chromium.org, zol...@webkit.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, p...@chromium.org, rune+...@opera.com, esp...@chromium.org
LGTM with a nit.


https://codereview.chromium.org/241113004/diff/1/Source/core/rendering/RenderBlock.cpp
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/241113004/diff/1/Source/core/rendering/RenderBlock.cpp#newcode1526
Source/core/rendering/RenderBlock.cpp:1526: if ((!posChildNeedsLayout()
&& !(needsSimplifiedNormalFlowLayout() ||
needsPositionedMovementLayout())) || normalChildNeedsLayout() ||
selfNeedsLayout())
This conditional is really hard to work out. Could we give the set of
conditionals a name or comment explaining what it's supposed to do?

https://codereview.chromium.org/241113004/

jchaf...@chromium.org

unread,
Apr 18, 2014, 2:20:31 PM4/18/14
to le...@chromium.org, blink-...@chromium.org, bemjb+r...@chromium.org, dsin...@chromium.org, zol...@webkit.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, p...@chromium.org, rune+...@opera.com, esp...@chromium.org

https://codereview.chromium.org/241113004/diff/1/Source/core/rendering/RenderBlock.cpp
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/241113004/diff/1/Source/core/rendering/RenderBlock.cpp#newcode1526
Source/core/rendering/RenderBlock.cpp:1526: if ((!posChildNeedsLayout()
&& !(needsSimplifiedNormalFlowLayout() ||
needsPositionedMovementLayout())) || normalChildNeedsLayout() ||
selfNeedsLayout())
On 2014/04/17 20:35:55, Levi wrote:
> This conditional is really hard to work out. Could we give the set of
> conditionals a name or comment explaining what it's supposed to do?

Split the logic and added some comment to make more obvious what's going
on.

https://codereview.chromium.org/241113004/

le...@chromium.org

unread,
Apr 18, 2014, 2:52:42 PM4/18/14
to jchaf...@chromium.org, blink-...@chromium.org, bemjb+r...@chromium.org, dsin...@chromium.org, zol...@webkit.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, p...@chromium.org, rune+...@opera.com, esp...@chromium.org
Thank you! This is clearer to me :)

https://codereview.chromium.org/241113004/

commi...@chromium.org

unread,
Apr 21, 2014, 12:25:21 PM4/21/14
to jchaf...@chromium.org, le...@chromium.org, blink-...@chromium.org, bemjb+r...@chromium.org, dsin...@chromium.org, zol...@webkit.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, p...@chromium.org, rune+...@opera.com, esp...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jchaf...@chromium.org/241113004/40001


https://codereview.chromium.org/241113004/

commi...@chromium.org

unread,
Apr 21, 2014, 12:36:33 PM4/21/14
to jchaf...@chromium.org, le...@chromium.org, blink-...@chromium.org, bemjb+r...@chromium.org, dsin...@chromium.org, zol...@webkit.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, p...@chromium.org, rune+...@opera.com, esp...@chromium.org
Change committed as 172042

https://codereview.chromium.org/241113004/
Reply all
Reply to author
Forward
0 new messages