Clip visual rect with real overflow clip rect

Clip visual rect with real overflow clip rect

BUG=599935
Committed: https://crrev.com/ce52a40114fdbdcf5b6c0914f64fe08b6c867c57
Cr-Commit-Position: refs/heads/master@{#384977}

[Xianzhu, 2016-04-01 23:31:26]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Xianzhu, 2016-04-01 23:50:59]

Wait a while. The flipped clipping seems incorrect.

[Xianzhu, 2016-04-02 00:34:47]

On 2016/04/01 23:50:59, Xianzhu wrote:
> Wait a while. The flipped clipping seems incorrect.

Turned out the code is correct, but we have crbug.com/600039 in layout of
vertical-rl block with borders.

Updated test to expose the error.

[chrishtr, 2016-04-04 16:33:16]

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden-expected.txt
(right):

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden-expected.txt:19:
PASS gFocusedDocument.activeElement.getAttribute("id") is "f5"
Why the changes here? Does this test need to be adjusted?

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:250:
EXPECT_TRUE(container->mapToVisualRectInAncestorSpace(container, rect));
Why did you change two call sites in this test?

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:273: LayoutRect rect
= targetOverflowRect;
Add an assert about what targetOverflowRect is equal to before mapping.

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:275:
EXPECT_EQ(LayoutRect(2, 3, 140, 110), rect);
Why 2,3?

Add a quick note that this is 140x110 because it includes the box-shadow
overflow, and is
 not clipped by the clip rect of 'container'

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:278:
EXPECT_EQ(LayoutRect(232, 121, 50, 80), rect);
Add a quick note that this adjusts for the top/left of 'container' and applies
its clip of
50x80.

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:281:
EXPECT_EQ(LayoutRect(0, 0, 70, 100), containerOverflowRect);
Why 70x100? Doesn't it include the overflow from 'target'?

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:287:
EXPECT_EQ(LayoutRect(222, 111, 70, 100), rect);
No clip applied?

[Xianzhu, 2016-04-04 17:46:58]

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden-expected.txt
(right):

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-div-overflow-scrol-hidden-expected.txt:19:
PASS gFocusedDocument.activeElement.getAttribute("id") is "f5"
On 2016/04/04 16:33:15, chrishtr wrote:
> Why the changes here? Does this test need to be adjusted?

(Sorry, I should have add comment to this file when I requested for review.)

The test tests focus movement in spatial navigation mode when pressing Down key.

Focus controller uses absoluteClippedOverflowRect to see if a focus candidate is
visible now and will be visible after scrolling. When Down key is pressed when
f5 is focused the first time (line 18), without this CL f6 is treated as visible
as it has 1 pixel line intersecting the scrolling container's border, and focus
is moved to it. Actually f6 is not visible at the time. With this CL, the first
Down key (line 18) scrolls the container down making f6 visible, then the next
Down key (line 19) focuses f6. This is expected behavior. The new line 20 is of
similar situation.

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp (right):

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:250:
EXPECT_TRUE(container->mapToVisualRectInAncestorSpace(container, rect));
On 2016/04/04 16:33:16, chrishtr wrote:
> Why did you change two call sites in this test?

The previous code was incorrect. We should never map container's overflow rect
by calling target's mapToVisualRectInAncestorSpace(). The result happened to be
correct because container and target have the same origin.

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:273: LayoutRect rect
= targetOverflowRect;
On 2016/04/04 16:33:16, chrishtr wrote:
> Add an assert about what targetOverflowRect is equal to before mapping.

Done.

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:275:
EXPECT_EQ(LayoutRect(2, 3, 140, 110), rect);
On 2016/04/04 16:33:15, chrishtr wrote:
> Why 2,3?
> 
> Add a quick note that this is 140x110 because it includes the box-shadow
> overflow, and is
>  not clipped by the clip rect of 'container'

Done.

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:278:
EXPECT_EQ(LayoutRect(232, 121, 50, 80), rect);
On 2016/04/04 16:33:16, chrishtr wrote:
> Add a quick note that this adjusts for the top/left of 'container' and applies
> its clip of
> 50x80.

Done.

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:281:
EXPECT_EQ(LayoutRect(0, 0, 70, 100), containerOverflowRect);
On 2016/04/04 16:33:16, chrishtr wrote:
> Why 70x100? Doesn't it include the overflow from 'target'?

No. The container has overflow clip, so its visual overflow doesn't include
overflow from descendants.

https://codereview.chromium.org/1846953008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp:287:
EXPECT_EQ(LayoutRect(222, 111, 70, 100), rect);
On 2016/04/04 16:33:15, chrishtr wrote:
> No clip applied?

Right. We should not apply container's overflow clip on container's own overflow
rect, otherwise we would clip off its border.

[chrishtr, 2016-04-04 18:14:50]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-04-04 18:14:51]

lgtm

[commit-bot: I haz the power, 2016-04-04 18:15:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846953008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846953008/80001

[Xianzhu, 2016-04-04 19:24:49]

Description was changed from

==========
Clip visual rect with real overflow clip rect

BUG=599939
==========

to

==========
Clip visual rect with real overflow clip rect

BUG=599935
==========

[commit-bot: I haz the power, 2016-04-04 19:33:21]

Description was changed from

==========
Clip visual rect with real overflow clip rect

BUG=599935
==========

to

==========
Clip visual rect with real overflow clip rect

BUG=599935
==========

[commit-bot: I haz the power, 2016-04-04 19:33:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-04 19:34:42]

Description was changed from

==========
Clip visual rect with real overflow clip rect

BUG=599935
==========

to

==========
Clip visual rect with real overflow clip rect

BUG=599935

Committed: https://crrev.com/ce52a40114fdbdcf5b6c0914f64fe08b6c867c57
Cr-Commit-Position: refs/heads/master@{#384977}
==========

[commit-bot: I haz the power, 2016-04-04 19:34:43]

Patchset 5 (id:??) landed as
https://crrev.com/ce52a40114fdbdcf5b6c0914f64fe08b6c867c57
Cr-Commit-Position: refs/heads/master@{#384977}