[CSS Shapes] Can't select content within the area of the floating box when clip-path is applied

[CSS Shapes] Can't select content within the area of the floating box when clip-path is applied

According to http://www.w3.org/TR/css-masking-1/, the clipped part of the element should not have pointer event dispatching. So add this logic to RenderBlock::nodeAtPoint. This fixes the bug as it disallows selection on any clipped parts.

BUG=364153
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172619

[krit, 2014-04-16 18:51:07]

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

https://codereview.chromium.org/240423005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:2769: // FIXME: handle REFERENCE
I would do a switch here to make sure you cover all the things.

https://codereview.chromium.org/240423005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:2773: if
(!clipPath->path(borderBoxRect()).contains(locationInContainer.point() -
localOffset, clipPath->windRule()))
Not sure if that is a win here. What about checking if the point is in the
bounding box first? It might make it slower though since we do not need the
bounding box anywhere else.

Also, checking path is one way, for things like circle or ellipse and maybe even
rect you don't want that and it is faster to do it yourself. Not sure how Skia
handles these cases though.

[krit, 2014-04-16 20:08:19]

https://codereview.chromium.org/240423005/diff/20001/LayoutTests/fast/shapes/...
File
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-clip-path-selection.html
(right):

https://codereview.chromium.org/240423005/diff/20001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-clip-path-selection.html:40:
shouldBe('window.getSelection().toString()', '"other"');
Can you do a positive check as well? Also, since the patch is clip-path only,
you don't need to have shape inside the demo.

[rwlbuis, 2014-04-18 18:18:39]

On 2014/04/16 20:08:19, krit wrote:
>
https://codereview.chromium.org/240423005/diff/20001/LayoutTests/fast/shapes/...
> File
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-clip-path-selection.html
> (right):
> 
>
https://codereview.chromium.org/240423005/diff/20001/LayoutTests/fast/shapes/...
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-clip-path-selection.html:40:
> shouldBe('window.getSelection().toString()', '"other"');
> Can you do a positive check as well? Also, since the patch is clip-path only,
> you don't need to have shape inside the demo.

Done!

[krit, 2014-04-22 13:04:06]

On 2014/04/18 18:18:39, rwlbuis wrote:
> On 2014/04/16 20:08:19, krit wrote:
> >
>
https://codereview.chromium.org/240423005/diff/20001/LayoutTests/fast/shapes/...
> > File
> >
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-clip-path-selection.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/240423005/diff/20001/LayoutTests/fast/shapes/...
> >
>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-clip-path-selection.html:40:
> > shouldBe('window.getSelection().toString()', '"other"');
> > Can you do a positive check as well? Also, since the patch is clip-path
only,
> > you don't need to have shape inside the demo.
> 
> Done!

I still don't see any changes on how you test for point in path.

One mouse hover can cause dozens of calls of this function. You don't want to
check against the path directly if the bounding box of the clip path is making
the element significant smaller.

We had this issue with SVG in the past. This is the reason why SVG has certain
heuristics before it actually does hit testing on the Path directly. As said,
for circles, ellipses and insets you can even do it manually wihtou ever asking
Path. But at lest the bounding box check should be in this patch.

[rwlbuis, 2014-04-22 20:30:48]

On 2014/04/22 13:04:06, krit wrote:
> 
> I still don't see any changes on how you test for point in path.
> 
> One mouse hover can cause dozens of calls of this function. You don't want to
> check against the path directly if the bounding box of the clip path is making
> the element significant smaller.
> 
> We had this issue with SVG in the past. This is the reason why SVG has certain
> heuristics before it actually does hit testing on the Path directly. As said,
> for circles, ellipses and insets you can even do it manually wihtou ever
asking
> Path. But at lest the bounding box check should be in this patch.

I was able to track this down :) Actually the Path::contains() calls
SkPathContainsPoint(), which
as its first step checks against the Path bounding box. So it seems we are safe!

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[krit, 2014-04-25 05:25:16]

On 2014/04/22 20:30:48, rwlbuis wrote:
> On 2014/04/22 13:04:06, krit wrote:
> > 
> > I still don't see any changes on how you test for point in path.
> > 
> > One mouse hover can cause dozens of calls of this function. You don't want
to
> > check against the path directly if the bounding box of the clip path is
making
> > the element significant smaller.
> > 
> > We had this issue with SVG in the past. This is the reason why SVG has
certain
> > heuristics before it actually does hit testing on the Path directly. As
said,
> > for circles, ellipses and insets you can even do it manually wihtou ever
> asking
> > Path. But at lest the bounding box check should be in this patch.
> 
> I was able to track this down :) Actually the Path::contains() calls
> SkPathContainsPoint(), which
> as its first step checks against the Path bounding box. So it seems we are
safe!
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

LGTM then.

[rwlbuis, 2014-04-25 10:44:32]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2014-04-25 10:44:33]

The CQ bit was unchecked by rob.buis@samsung.com

[rwlbuis, 2014-04-25 10:44:38]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-04-25 10:44:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/240423005/60001

[commit-bot: I haz the power, 2014-04-25 12:39:54]

Change committed as 172619