Include clip-path and border radius when hit testing boxes

Include clip-path and border radius when hit testing boxes

This patch fixes a bug where clip-path and border radius were ignored
on boxes such as images. This code was present in LayoutBlock but
needed to be moved up to LayoutBox.

The patch itself primarily extracts the clip-path and border radius
code into functions called by both LayoutBlock and LayoutBox. The clip
path code has been cleaned up a little not use a switch. A TODO has
been added for sharing even more code between the block and box hit
testing code. Another TODO has been added for compositor support for
hit test clipping.

BUG=468613
Committed: https://crrev.com/ba4a97410b76057c11d300c4bd61402863f50d45
Cr-Commit-Position: refs/heads/master@{#365050}

[pdr., 2015-12-08 04:31:31]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, rbyers@chromium.org

[pdr., 2015-12-08 04:31:40]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-08 04:33:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506053003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506053003/1

[commit-bot: I haz the power, 2015-12-08 06:33:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 06:33:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2015-12-08 15:24:23]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-08 15:24:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506053003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506053003/1

[commit-bot: I haz the power, 2015-12-08 16:15:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 16:15:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2015-12-08 21:58:08]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-12-08 22:06:17]

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

[commit-bot: I haz the power, 2015-12-08 22:06:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 22:06:18]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Rick Byers, 2015-12-08 22:13:47]

I don't have any experience in the SVG-specific code here, but the hit-testing
logic LGTM.

[chrishtr, 2015-12-08 22:14:32]

https://codereview.chromium.org/1506053003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1506053003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:1172: if
(hitTestClippedOutByClipPath(FloatPoint(locationInContainer.point() -
adjustedLocation)))
And css clip?

What kind of element is a box but not a block, and can have clip-path, but can't
have overflow clip?

[pdr., 2015-12-08 22:20:08]

On 2015/12/08 at 22:13:47, rbyers wrote:
> I don't have any experience in the SVG-specific code here, but the hit-testing
logic LGTM.

Thanks!

Even though this test has svg in it, this code applies to all the different
kinds of clip-path we support. Firefox only supports clip-path: [svg] so I used
svg to ensure the test passes in both browsers.

[pdr., 2015-12-08 22:32:39]

On 2015/12/08 at 22:14:32, chrishtr wrote:
>
https://codereview.chromium.org/1506053003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):
> 
>
https://codereview.chromium.org/1506053003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBox.cpp:1172: if
(hitTestClippedOutByClipPath(FloatPoint(locationInContainer.point() -
adjustedLocation)))
> And css clip?

In looking into the clip issue (http://jsbin.com/nuzixicaze), Chris and I found
that we should move all of this up into the paint layer instead of adding this
logic to LayoutBox.

Investigating that approach now..

[pdr., 2015-12-11 06:16:29]

Description was changed from

==========
Do not hit test areas of a box that are clipped out by clip-path

This patch fixes a bug where clip-path was ignored on boxes such as
images. The clip path hit test code in LayoutBlock has been cleaned
up and moved to LayoutBox as a helper. A TODO has also been added to
also check for border radius in LayoutBox::nodeAtPoint.

BUG=468613
==========

to

==========
Include clip-path and border radius when hit testing boxes

This patch fixes a bug where clip-path and border radius were ignored
on boxes such as images. This code was present in LayoutBlock but
needed to be moved up to LayoutBox.

The patch itself primarily extracts the clip-path and border radius
code into functions called by both LayoutBlock and LayoutBox. A TODO
has been added for sharing even more code between the block and box
hit testing code. Another TODO has been added for compositor support
for hit test clipping.

BUG=468613
==========

[pdr., 2015-12-11 06:34:14]

Description was changed from

==========
Include clip-path and border radius when hit testing boxes

This patch fixes a bug where clip-path and border radius were ignored
on boxes such as images. This code was present in LayoutBlock but
needed to be moved up to LayoutBox.

The patch itself primarily extracts the clip-path and border radius
code into functions called by both LayoutBlock and LayoutBox. A TODO
has been added for sharing even more code between the block and box
hit testing code. Another TODO has been added for compositor support
for hit test clipping.

BUG=468613
==========

to

==========
Include clip-path and border radius when hit testing boxes

This patch fixes a bug where clip-path and border radius were ignored
on boxes such as images. This code was present in LayoutBlock but
needed to be moved up to LayoutBox.

The patch itself primarily extracts the clip-path and border radius
code into functions called by both LayoutBlock and LayoutBox. The clip
path code has been cleaned up a little not use a switch. A TODO has
been added for sharing even more code between the block and box hit
testing code. Another TODO has been added for compositor support for
hit test clipping.

BUG=468613
==========

[pdr., 2015-12-11 06:36:20]

Pretty large update which reflects our findings in this area over the past week.

This patch now fixes both clip path and border radius, and the description has
been updated.

PTAL

[pdr., 2015-12-11 06:36:23]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-11 06:37:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506053003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506053003/40001

[commit-bot: I haz the power, 2015-12-11 08:27:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-11 08:27:08]

Dry run: This issue passed the CQ dry run.

[Rick Byers, 2015-12-11 16:02:51]

Ah, this seems better - thank you!
One suggestion for the test.

https://codereview.chromium.org/1506053003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/hittesting/image-with-clip-path.html
(right):

https://codereview.chromium.org/1506053003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/hittesting/image-with-clip-path.html:15:
clip-path: url(#clipPathTopLeft);
So this test covers the 'ClipPathOperation:REFERENCE' case in the code, but the
'ClipPathOperation:SHAPE' case is still untested, right?  Perhaps it would be
worth adding another case here for that too?

[pdr., 2015-12-11 18:09:23]

https://codereview.chromium.org/1506053003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/hittesting/image-with-clip-path.html
(right):

https://codereview.chromium.org/1506053003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/hittesting/image-with-clip-path.html:15:
clip-path: url(#clipPathTopLeft);
On 2015/12/11 at 16:02:51, Rick Byers wrote:
> So this test covers the 'ClipPathOperation:REFERENCE' case in the code, but
the 'ClipPathOperation:SHAPE' case is still untested, right?  Perhaps it would
be worth adding another case here for that too?

Good idea, done in the latest patch.

[Rick Byers, 2015-12-11 18:12:21]

LGTM

[pdr., 2015-12-11 18:17:54]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-11 18:19:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506053003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506053003/60001

[commit-bot: I haz the power, 2015-12-11 19:35:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-11 19:35:44]

Dry run: This issue passed the CQ dry run.

[pdr., 2015-12-13 21:03:39]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-13 21:03:52]

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

[pdr., 2015-12-14 01:01:12]

On 2015/12/13 at 21:03:52, commit-bot wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1506053003/80001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1506053003/80001

One more small update per offline discussion with chrishtr.

@chrishtr, LGTYNao?

[commit-bot: I haz the power, 2015-12-14 03:23:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-14 03:23:27]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2015-12-14 17:51:05]

lgtm

[pdr., 2015-12-14 18:20:36]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-12-14 18:20:37]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1506053003/#ps80001
(title: "Hit test in PaintLayer, add transformed clip path tests")

[commit-bot: I haz the power, 2015-12-14 18:21:38]

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

[commit-bot: I haz the power, 2015-12-14 18:29:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-14 18:30:43]

Description was changed from

==========
Include clip-path and border radius when hit testing boxes

This patch fixes a bug where clip-path and border radius were ignored
on boxes such as images. This code was present in LayoutBlock but
needed to be moved up to LayoutBox.

The patch itself primarily extracts the clip-path and border radius
code into functions called by both LayoutBlock and LayoutBox. The clip
path code has been cleaned up a little not use a switch. A TODO has
been added for sharing even more code between the block and box hit
testing code. Another TODO has been added for compositor support for
hit test clipping.

BUG=468613
==========

to

==========
Include clip-path and border radius when hit testing boxes

This patch fixes a bug where clip-path and border radius were ignored
on boxes such as images. This code was present in LayoutBlock but
needed to be moved up to LayoutBox.

The patch itself primarily extracts the clip-path and border radius
code into functions called by both LayoutBlock and LayoutBox. The clip
path code has been cleaned up a little not use a switch. A TODO has
been added for sharing even more code between the block and box hit
testing code. Another TODO has been added for compositor support for
hit test clipping.

BUG=468613

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

[commit-bot: I haz the power, 2015-12-14 18:30:44]

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