Bug 131375

Summary: [CSS Shapes] Can't select content within the area of the floating box when clip-path is applied
Product: WebKit Reporter: Zoltan Horvath <zoltan>
Component: CSSAssignee: Zoltan Horvath <zoltan>
Status: RESOLVED FIXED    
Severity: Normal CC: bjonesbe, commit-queue, esprehn+autocc, glenn, kondapallykalyan, krit, rwlbuis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 98664    
Attachments:
Description Flags
Test-case
none
Patch
none
Patch - Added support and testing for the CSS box models
none
Same as previous, just renamed selectors in the test case none

Description Zoltan Horvath 2014-04-08 11:06:56 PDT
Can't select content within the floating area (lightgreen).

- Selection works properly for image shapes.
Comment 1 Zoltan Horvath 2014-04-08 11:07:21 PDT
Created attachment 228858 [details]
Test-case
Comment 2 Bem Jones-Bey 2014-04-08 11:17:47 PDT
It looks like this only is a problem when -webkit-clip-path is used on the floating box. If the clip path is removed from the example, then the issue doesn't occur.
Comment 3 Rob Buis 2014-04-25 08:36:39 PDT
I landed a fix for this in Blink:

http://src.chromium.org/viewvc/blink?view=rev&rev=172619
Comment 4 Zoltan Horvath 2014-04-25 14:33:07 PDT
Created attachment 230204 [details]
Patch
Comment 5 Dirk Schulze 2014-04-26 12:31:41 PDT
Comment on attachment 230204 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230204&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2971
> +            if (!clipPath->pathForReferenceRect(borderBoxRect()).contains(locationInContainer.point() - localOffset, clipPath->windRule()))

Same question as in Blink. Hit-testing occurs dozen of times just by hovering over the original element size. Can you try to a create (manual) performance test? I am thinking about a script that creates hundred of div boxes with fixed size. Each box with a circle clip shape and a radius of 50%. Add :hover pseudo selectors that change the background color. Then hover with the mouse really fast and compare it with the same document where the div boxes are not clipped.

It would be interesting if it makes sense to investigate in more heuristics before asking the graphic lib if the point is in the path. Especially for circle, inset, ellipse it would be very easy to add heuristics. For polygon we could at least check the bounding box.
Comment 6 Zoltan Horvath 2014-05-05 23:36:42 PDT
Hi Dirk,

I made the performance tests.

The test displays 30 circles (radius 100px) clipped by clip path. It exposes mouse hover/unhover from the 0,0 point until the center of the circle.

DESCRIPTION: Testing clip-path using 30 shapes.
RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Time= 1417.15 ms
median= 1439.0 ms, stdev= 171.044246353 ms, min= 1038.0 ms, max= 1700.0 ms
RESULT Layout: Shapes: ShapeOutsideClipPathSelection: JSHeap= 598262.2 bytes
median= 604022.0 bytes, stdev= 11827.0697603 bytes, min= 575207.0 bytes, max= 604038.0 bytes
RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Malloc= 1014103.6 bytes
median= 1010400.0 bytes, stdev= 8320.6471307 bytes, min= 1009616.0 bytes, max= 1036768.0 bytes
Finished: 35.947498 s

DESCRIPTION: Testing clip-path + shapes with circle() using 30 shapes.
RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Time= 1510.9 ms
median= 1546.5 ms, stdev= 130.736335705 ms, min= 1128.0 ms, max= 1594.0 ms
RESULT Layout: Shapes: ShapeOutsideClipPathSelection: JSHeap= 596199.2 bytes
median= 601959.0 bytes, stdev= 11827.0697603 bytes, min= 573144.0 bytes, max= 601975.0 bytes
RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Malloc= 1017173.6 bytes
median= 1015184.0 bytes, stdev= 7281.3986395 bytes, min= 1010456.0 bytes, max= 1033488.0 bytes
Finished: 37.944173 s

No-Shapes: 1439.0ms stdev~8%
Shapes: 1546.5ms  stdev ~11.8%

Shapes case is ~7.4% worse.

Although, the standard deviations are pretty high, I've got the similar results after multiple runs. If you want to see these tests in the trunk, I can add them in a separate bug. Also, if you think it's worth to introduce optimizations, we can open the bug for it, but I see those task as possible follow up work after landing this. What do you think?
Comment 7 Dirk Schulze 2014-05-06 01:28:57 PDT
(In reply to comment #6)
> Hi Dirk,
> 
> I made the performance tests.
> 
> The test displays 30 circles (radius 100px) clipped by clip path. It exposes mouse hover/unhover from the 0,0 point until the center of the circle.
> 
> DESCRIPTION: Testing clip-path using 30 shapes.
> RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Time= 1417.15 ms
> median= 1439.0 ms, stdev= 171.044246353 ms, min= 1038.0 ms, max= 1700.0 ms
> RESULT Layout: Shapes: ShapeOutsideClipPathSelection: JSHeap= 598262.2 bytes
> median= 604022.0 bytes, stdev= 11827.0697603 bytes, min= 575207.0 bytes, max= 604038.0 bytes
> RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Malloc= 1014103.6 bytes
> median= 1010400.0 bytes, stdev= 8320.6471307 bytes, min= 1009616.0 bytes, max= 1036768.0 bytes
> Finished: 35.947498 s
> 
> DESCRIPTION: Testing clip-path + shapes with circle() using 30 shapes.
> RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Time= 1510.9 ms
> median= 1546.5 ms, stdev= 130.736335705 ms, min= 1128.0 ms, max= 1594.0 ms
> RESULT Layout: Shapes: ShapeOutsideClipPathSelection: JSHeap= 596199.2 bytes
> median= 601959.0 bytes, stdev= 11827.0697603 bytes, min= 573144.0 bytes, max= 601975.0 bytes
> RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Malloc= 1017173.6 bytes
> median= 1015184.0 bytes, stdev= 7281.3986395 bytes, min= 1010456.0 bytes, max= 1033488.0 bytes
> Finished: 37.944173 s
> 
> No-Shapes: 1439.0ms stdev~8%
> Shapes: 1546.5ms  stdev ~11.8%
> 
> Shapes case is ~7.4% worse.
> 
> Although, the standard deviations are pretty high, I've got the similar results after multiple runs. If you want to see these tests in the trunk, I can add them in a separate bug. Also, if you think it's worth to introduce optimizations, we can open the bug for it, but I see those task as possible follow up work after landing this. What do you think?

Thanks for the tests. I am not sure if you understood my request though.

Just want to have performance tests for hit testing:
1) with clip-path
2) without clip-path.

It makes sense that shapes will make it slower. My concern was just about clip-path hit testing performance. The reason, if it is bad, we might want to compute the bounding box of the element after clipping and do hit testing on the bounding box first before asking isPointInPath().
Comment 8 Dirk Schulze 2014-05-06 01:31:19 PDT
Comment on attachment 230204 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230204&action=review

> Source/WebCore/rendering/RenderBlock.cpp:2970
> +            // FIXME: handle marginBox etc.

Even if borderBox is probably the most use reference box, could you add the other boxes as well? the logic to compute these boxes should be in RenderLayer probably.
Comment 9 Zoltan Horvath 2014-05-06 12:25:37 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Hi Dirk,
> > 
> > I made the performance tests.
> > 
> > The test displays 30 circles (radius 100px) clipped by clip path. It exposes mouse hover/unhover from the 0,0 point until the center of the circle.
> > 
> > DESCRIPTION: Testing clip-path using 30 shapes.
> > RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Time= 1417.15 ms
> > median= 1439.0 ms, stdev= 171.044246353 ms, min= 1038.0 ms, max= 1700.0 ms
> > RESULT Layout: Shapes: ShapeOutsideClipPathSelection: JSHeap= 598262.2 bytes
> > median= 604022.0 bytes, stdev= 11827.0697603 bytes, min= 575207.0 bytes, max= 604038.0 bytes
> > RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Malloc= 1014103.6 bytes
> > median= 1010400.0 bytes, stdev= 8320.6471307 bytes, min= 1009616.0 bytes, max= 1036768.0 bytes
> > Finished: 35.947498 s

DESCRIPTION: Testing no-clip-path 30 boxes.
RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Time= 1343.85 ms
median= 1356.5 ms, stdev= 83.9801198279 ms, min= 997.0 ms, max= 1429.0 ms
RESULT Layout: Shapes: ShapeOutsideClipPathSelection: JSHeap= 598390.2 bytes
median= 604150.0 bytes, stdev= 11827.0697603 bytes, min= 575335.0 bytes, max= 604166.0 bytes
RESULT Layout: Shapes: ShapeOutsideClipPathSelection: Malloc= 1017564.8 bytes
median= 1015432.0 bytes, stdev= 7189.77538336 bytes, min= 1010640.0 bytes, max= 1037768.0 bytes
Finished: 33.877189 s

> Thanks for the tests. I am not sure if you understood my request though.
> 
> Just want to have performance tests for hit testing:
> 1) with clip-path
> 2) without clip-path.

clip-path: 1439ms ~6% worse
no-clip-path: 1356ms 

I've run the the test for that case as well, results are above.
Comment 10 Zoltan Horvath 2014-05-06 12:25:58 PDT
(In reply to comment #8)
> (From update of attachment 230204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230204&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:2970
> > +            // FIXME: handle marginBox etc.
> 
> Even if borderBox is probably the most use reference box, could you add the other boxes as well? the logic to compute these boxes should be in RenderLayer probably.

I'll start to look into this.
Comment 11 Dirk Schulze 2014-05-06 12:27:10 PDT
(In reply to comment #9)

> clip-path: 1439ms ~6% worse
> no-clip-path: 1356ms 
> 
> I've run the the test for that case as well, results are above.

Ok, that sounds reasonable. Then just the other references boxes and the patch is good to go :).
Comment 12 Zoltan Horvath 2014-05-07 21:35:06 PDT
Created attachment 231042 [details]
Patch - Added support and testing for the CSS box models
Comment 13 Zoltan Horvath 2014-05-07 21:37:50 PDT
Created attachment 231043 [details]
Same as previous, just renamed selectors in the test case
Comment 14 WebKit Commit Bot 2014-05-08 00:57:39 PDT
Comment on attachment 231043 [details]
Same as previous, just renamed selectors in the test case

Clearing flags on attachment: 231043

Committed r168463: <http://trac.webkit.org/changeset/168463>
Comment 15 WebKit Commit Bot 2014-05-08 00:57:43 PDT
All reviewed patches have been landed.  Closing bug.