Don't apply overflow clips in paint invalidation. (issue 2280243002 by chrishtr@chromium.org)

chri...@chromium.org

unread,

Aug 26, 2016, 6:37:15 PM8/26/16

to wko...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, slimming-pa...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Reviewers: wkorman, Xianzhu
CL: https://codereview.chromium.org/2280243002/

Message:
This essentially reverts https://codereview.chromium.org/1856993004.

Description:
Don't apply overflow clips in paint invalidation.

Previously, such clips were applied if the paint invalidation container did
not scroll overflow, but this is problematic, because overflow:hidden elements
can be scrolled via script.

BUG=597902,597902
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Affected files (+83, -8 lines):
A third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled.html
A third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled-expected.txt
M third_party/WebKit/Source/core/layout/LayoutBox.cpp
M third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp
M third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp

Index: third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled-expected.txt
diff --git a/third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled-expected.txt b/third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled-expected.txt
new file mode 100644
index 0000000000000000000000000000000000000000..aa4b2a50f8e48df4c8d0d6176a2d11533e661f70
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled-expected.txt
@@ -0,0 +1,63 @@
+{
+ "name": "Content Root Layer",
+ "bounds": [800, 600],
+ "children": [
+ {
+ "name": "LayoutView #document",
+ "bounds": [800, 600],
+ "contentsOpaque": true,
+ "drawsContent": true,
+ "children": [
+ {
+ "name": "LayoutBlockFlow DIV id='scroller'",
+ "position": [8, 8],
+ "bounds": [302, 302],
+ "drawsContent": true,
+ "backfaceVisibility": "hidden",
+ "paintInvalidations": [
+ {
+ "object": "LayoutBlockFlow DIV id='scroller'",
+ "rect": [0, 0, 302, 302],
+ "reason": "subtree"
+ },
+ {
+ "object": "LayoutBlockFlow DIV",
+ "rect": [1, 1, 300, 600],
+ "reason": "subtree"
+ },
+ {
+ "object": "LayoutBlockFlow DIV",
+ "rect": [1, -399, 300, 600],
+ "reason": "subtree"
+ },
+ {
+ "object": "LayoutBlockFlow DIV",
+ "rect": [1, 601, 100, 100],
+ "reason": "subtree"
+ },
+ {
+ "object": "LayoutBlockFlow DIV",
+ "rect": [1, 201, 100, 100],
+ "reason": "subtree"
+ }
+ ]
+ }
+ ]
+ }
+ ],
+ "objectPaintInvalidations": [
+ {
+ "object": "LayoutBlockFlow DIV id='scroller'",
+ "reason": "subtree"
+ },
+ {
+ "object": "LayoutBlockFlow DIV",
+ "reason": "subtree"
+ },
+ {
+ "object": "LayoutBlockFlow DIV",
+ "reason": "subtree"
+ }
+ ]
+}
+
Index: third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled.html
diff --git a/third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled.html b/third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled.html
new file mode 100644
index 0000000000000000000000000000000000000000..3872bdb84f7d9a15429ba96948e5bc03bcf0943b
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/paint/invalidation/overflow-hidden-yet-scrolled.html
@@ -0,0 +1,13 @@
+<!doctype html>
+<script src="../../fast/repaint/resources/text-based-repaint.js"></script>
+<div id="scroller" style="overflow: hidden; width: 300px; height: 300px; backface-visibility: hidden; border: 1px solid black">
+ <div style="width: 300px; height: 600px"></div>
+ <div style="width: 100px; height: 100px; background: lightgray"></div>
+</div>
+<script>
+onload = runRepaintTest;
+
+function repaintTest() {
+ scroller.scrollTop = 400;
+}
+</script>
Index: third_party/WebKit/Source/core/layout/LayoutBox.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBox.cpp b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
index 3ac149226ea287ae482c741872f4534cd0e74266..5bdf6ae4ce387f2f9ce805be4c333b4b1b99f119 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBox.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
@@ -1025,7 +1025,7 @@ bool LayoutBox::mapScrollingContentsRectToBoxSpace(LayoutRect& rect, ApplyOverfl
if (!hasClipRelatedProperty())
return true;

- if (applyOverflowClip == ApplyNonScrollOverflowClip && scrollsOverflow())
+ if (applyOverflowClip == ApplyNonScrollOverflowClip)
return true;

if (hasOverflowClip()) {
Index: third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp
diff --git a/third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp b/third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp
index 9d380d2dcd495337a1986dfda0b18d4d71cab736..b45503a9952551ae29af17e7412f6123ac37fde3 100644
--- a/third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp
+++ b/third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp
@@ -315,9 +315,9 @@ void PaintInvalidationState::updateForNormalChildren()
if (!box.hasClipRelatedProperty())
return;

- // Do not clip or scroll for the paint invalidation container, if it scrolls overflow, because it will always use composited
- // scrolling in this case.
- if (box == m_paintInvalidationContainer && box.scrollsOverflow()) {
+ // Do not clip or scroll for the paint invalidation container, because the semantics of visual rects do not include clipping or
+ // scrolling on that object.
+ if (box == m_paintInvalidationContainer) {
DCHECK(!m_clipped); // The box establishes paint invalidation container, so no m_clipped inherited.
} else {
// This won't work fully correctly for fixed-position elements, who should receive CSS clip but for whom the current object
Index: third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp
diff --git a/third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp b/third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp
index a0ec91f8f141f9f0dd68799d42da8ed32e9224b5..dbda2256b4af0173fa4fe1a5b98eec2263c2aed9 100644
--- a/third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp
+++ b/third_party/WebKit/Source/core/layout/VisualRectMappingTest.cpp
@@ -423,8 +423,8 @@ TEST_F(VisualRectMappingTest, ContainerOverflowHidden)

rect = targetOverflowRect;
EXPECT_TRUE(target->mapToVisualRectInAncestorSpace(container, rect));
- // Rect is clipped by container's overflow clip.
- EXPECT_EQ(LayoutRect(10, 10, 50, 80), rect);
+ // Rect is not clipped by container's overflow clip.
+ EXPECT_EQ(LayoutRect(10, 10, 140, 110), rect);
}

TEST_F(VisualRectMappingTest, ContainerFlippedWritingModeAndOverflowHidden)
@@ -463,8 +463,7 @@ TEST_F(VisualRectMappingTest, ContainerFlippedWritingModeAndOverflowHidden)
target->flipForWritingMode(rect);
EXPECT_TRUE(target->mapToVisualRectInAncestorSpace(container, rect));
// 58 = target_physical_x(100) + container_border_left(40) - scroll_left(58)
- // The other sides of the rect are clipped by container's overflow clip.
- EXPECT_EQ(LayoutRect(58, 10, 32, 80), rect);
+ EXPECT_EQ(LayoutRect(-10, 10, 140, 110), rect);
}

TEST_F(VisualRectMappingTest, ContainerAndTargetDifferentFlippedWritingMode)

wko...@chromium.org

unread,

Aug 26, 2016, 6:58:03 PM8/26/16

to chri...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, slimming-pa...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

lgtm

The BUG= has the same bug in there 2x, maybe you meant to paste a different bug
for one of those?

https://codereview.chromium.org/2280243002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2280243002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1028
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1028: if
(applyOverflowClip == ApplyNonScrollOverflowClip)
In reviewing the docs for this method to make sure we didn't need to
update them, I saw this:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBox.h?q=layoutbox.h&sq=package:chromium&dr=CSs&l=906

// If edgeInclusive is true, then this method may return true even
// if the resulting rect has zero area.

There is no sign of 'edgeInclusive' except in VisualRectFlags which is
not a part of this method. If this just a totally stale comment maybe
remove as part of this change.

https://codereview.chromium.org/2280243002/

chri...@chromium.org

unread,

Aug 26, 2016, 7:05:21 PM8/26/16

to wangx...@chromium.org, wko...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, slimming-pa...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2280243002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2280243002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1028
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1028: if
(applyOverflowClip == ApplyNonScrollOverflowClip)

On 2016/08/26 at 22:58:03, wkorman wrote:
> In reviewing the docs for this method to make sure we didn't need to
update them, I saw this:
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBox.h?q=layoutbox.h&sq=package:chromium&dr=CSs&l=906
>
> // If edgeInclusive is true, then this method may return true even
> // if the resulting rect has zero area.
>
> There is no sign of 'edgeInclusive' except in VisualRectFlags which is
not a part of this method. If this just a totally stale comment maybe
remove as part of this change.

See line 1041 in the new file.

https://codereview.chromium.org/2280243002/

chri...@chromium.org

unread,

Aug 26, 2016, 7:05:35 PM8/26/16

to wangx...@chromium.org, wko...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, slimming-pa...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Also, added the correct bug.

https://codereview.chromium.org/2280243002/

wko...@chromium.org

unread,

Aug 26, 2016, 8:32:02 PM8/26/16

to chri...@chromium.org, wangx...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, slimming-pa...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2280243002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBox.cpp
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2280243002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBox.cpp#newcode1028
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1028: if
(applyOverflowClip == ApplyNonScrollOverflowClip)

On 2016/08/26 23:05:18, chrishtr wrote:
> On 2016/08/26 at 22:58:03, wkorman wrote:
> > In reviewing the docs for this method to make sure we didn't need to
update
> them, I saw this:
> >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutBox.h?q=layoutbox.h&sq=package:chromium&dr=CSs&l=906
> >
> > // If edgeInclusive is true, then this method may return true
even
> > // if the resulting rect has zero area.
> >
> > There is no sign of 'edgeInclusive' except in VisualRectFlags which
is not a
> part of this method. If this just a totally stale comment maybe remove
as part
> of this change.
>
> See line 1041 in the new file.

May have forgotten to add file or upload

https://codereview.chromium.org/2280243002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 27, 2016, 7:53:58 PM8/27/16

to chri...@chromium.org, wangx...@chromium.org, wko...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, slimming-pa...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2280243002/40001

https://codereview.chromium.org/2280243002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 27, 2016, 10:16:07 PM8/27/16

to chri...@chromium.org, wangx...@chromium.org, wko...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, slimming-pa...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Committed patchset #3 (id:40001)

https://codereview.chromium.org/2280243002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 27, 2016, 10:17:38 PM8/27/16

to chri...@chromium.org, wangx...@chromium.org, wko...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, chromium...@chromium.org, dongseo...@intel.com, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, slimming-pa...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Patchset 3 (id:??) landed as
https://crrev.com/537724421c0f5d046e56fb0eba001da3bcea0613
Cr-Commit-Position: refs/heads/master@{#414951}

https://codereview.chromium.org/2280243002/

Reply all

Reply to author

Forward