Bug 82628

Summary: Hit testing on a stroke with vector-effects:non-scaling-stroke is broken
Product: WebKit Reporter: Philip Rogers <pdr>
Component: SVGAssignee: Philip Rogers <pdr>
Status: RESOLVED FIXED    
Severity: Normal CC: d-r, eric, mnaganov, pnormand, rakuco, rwlbuis, schenney, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Update svg-ellipse-non-scale-stroke expectations
none
Fix hit testing on non-scaling strokes
none
Update per reviewer comments
zimmermann: review+
Patch for landing none

Description Philip Rogers 2012-03-29 10:58:30 PDT
As the description says, non-scaling-strokes are not correctly handing hit testing on the stroke.
Comment 2 Philip Rogers 2012-04-24 08:21:23 PDT
(In reply to comment #1)
> Passes for a long time already:
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2Fhittest%2Fsvg-ellipse-non-scale-stroke.xhtml

I believe this was a simple mistake by philn@webkit.org. http://trac.webkit.org/changeset/113986 incorrectly rebaselined svg/hittest/svg-ellipse-non-scale-stroke-expected.txt to contain "FAIL" values, so this test should still be failing. Re-re-baseline patch forthcoming.
Comment 3 Mikhail Naganov 2012-04-24 08:23:17 PDT
Oh, right -- I've missed the last two "FAIL" lines. Thanks for noticing this!
Comment 4 Philippe Normand 2012-04-24 08:29:40 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Passes for a long time already:
> > 
> > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2Fhittest%2Fsvg-ellipse-non-scale-stroke.xhtml
> 
> I believe this was a simple mistake by philn@webkit.org. http://trac.webkit.org/changeset/113986 incorrectly rebaselined svg/hittest/svg-ellipse-non-scale-stroke-expected.txt to contain "FAIL" values, so this test should still be failing. Re-re-baseline patch forthcoming.

Oops, sorry!
Comment 5 Philip Rogers 2012-04-24 09:00:00 PDT
Created attachment 138574 [details]
Update svg-ellipse-non-scale-stroke expectations
Comment 6 WebKit Review Bot 2012-04-24 10:46:36 PDT
Comment on attachment 138574 [details]
Update svg-ellipse-non-scale-stroke expectations

Clearing flags on attachment: 138574

Committed r115081: <http://trac.webkit.org/changeset/115081>
Comment 7 WebKit Review Bot 2012-04-24 10:46:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Philip Rogers 2012-04-24 10:54:19 PDT
(In reply to comment #7)
> All reviewed patches have been landed.  Closing bug.

Reopening because the bug is not fixed.
Comment 9 Philip Rogers 2012-05-07 10:35:15 PDT
Created attachment 140550 [details]
Fix hit testing on non-scaling strokes
Comment 10 Philip Rogers 2012-05-07 10:52:18 PDT
Reviewers: I'm not in love with the two similar blocks of code getting the ctm and modifying the path, but I'm not sure of a better way. Suggestions very welcome in those areas :)
Comment 11 Stephen Chenney 2012-05-16 12:00:15 PDT
Bump. Niko or Rob, do you have time for this? Anyone else?
Comment 12 Nikolas Zimmermann 2012-05-16 12:43:38 PDT
Comment on attachment 140550 [details]
Fix hit testing on non-scaling strokes

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

Patch looks good in general, some issues I'd like to resolve before giving r+:

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:105
> +    if (style()->svgStyle()->vectorEffect() == VE_NON_SCALING_STROKE) {

Shouldn't we only do this if m_path is non-null? Is this guaranteed?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:108
> +        SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
> +        AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate);
> +        Path* nonScaledStrokePath = nonScalingStrokePath(m_path.get(), nonScalingStrokeTransform);

You could share this code with the other code in inflateWithStrokeAndMarkerBounds.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:465
> +        if (style()->svgStyle()->vectorEffect() == VE_NON_SCALING_STROKE) {

I really wish we had a central place to handle VE_NON_SCALING_STROKE. Code for it is sprinkled around rendering/.
Can we do better? Do you have ideas on how to share even more of this?
Comment 13 Philip Rogers 2012-05-19 18:50:08 PDT
Created attachment 142887 [details]
Update per reviewer comments
Comment 14 Philip Rogers 2012-05-19 19:27:28 PDT
(In reply to comment #12)
> (From update of attachment 140550 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140550&action=review
> 
> Patch looks good in general, some issues I'd like to resolve before giving r+:
> 
> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:105
> > +    if (style()->svgStyle()->vectorEffect() == VE_NON_SCALING_STROKE) {
> 
> Shouldn't we only do this if m_path is non-null? Is this guaranteed?

In this case it is guaranteed but only because RenderSVGRect and RenderSVGShape have special logic to call createShape() if they have non-scaling strokes. I added two asserts to guard against mistakes in the future.

Just as a note: I think RenderSVGShape is overdue for a big refactor (by me). I have some ideas about how we could clean up the sharing and fallback mechanisms between RenderSVGShape and subclasses, since it's not easy to see when things have a path, who triggers falling back to RenderSVGShape, who calls createShape(), etc.

> 
> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:108
> > +        SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
> > +        AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate);
> > +        Path* nonScaledStrokePath = nonScalingStrokePath(m_path.get(), nonScalingStrokeTransform);
> 
> You could share this code with the other code in inflateWithStrokeAndMarkerBounds.

Done. I refactored most of the non-scaling stroke code to generally clean it up as well.

> 
> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:465
> > +        if (style()->svgStyle()->vectorEffect() == VE_NON_SCALING_STROKE) {
> 
> I really wish we had a central place to handle VE_NON_SCALING_STROKE. Code for it is sprinkled around rendering/.
> Can we do better? Do you have ideas on how to share even more of this?

We're not doing too bad--just code in the ResourceGradient, ResourcePattern, and RenderSVGShape/friends. Because non-scaling stroke affects both hit testing and rendering, and because we do special-cased non-raster/analytic paths like RenderSVGEllipse for performance reasons, I'm not sure this sprinkling can be cleaned up without hurting performance.

For path code in particular though, it would be nice to support non-scaling at the GraphicsContext level. Depending on how it is spec'ed, we might be able to unify our code for SVG2's variable-width stroke with non-scaling stroke at the same time.
Comment 15 Nikolas Zimmermann 2012-05-20 01:54:07 PDT
Comment on attachment 142887 [details]
Update per reviewer comments

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

r=me.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:470
> +        // NonScalingStroke only applies to stroke and not markers, see http://www.w3.org/TR/SVGTiny12/painting.html#NonScalingStroke

Missing trailing period.
Comment 16 Philip Rogers 2012-05-20 12:06:09 PDT
Created attachment 142910 [details]
Patch for landing
Comment 17 Philip Rogers 2012-05-20 12:08:13 PDT
(In reply to comment #15)
> (From update of attachment 142887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142887&action=review
> 
> r=me.
> 
> > Source/WebCore/rendering/svg/RenderSVGShape.cpp:470
> > +        // NonScalingStroke only applies to stroke and not markers, see http://www.w3.org/TR/SVGTiny12/painting.html#NonScalingStroke
> 
> Missing trailing period.

Thanks for the review Niko!
Comment 18 WebKit Review Bot 2012-05-20 12:49:02 PDT
Comment on attachment 142910 [details]
Patch for landing

Clearing flags on attachment: 142910

Committed r117709: <http://trac.webkit.org/changeset/117709>
Comment 19 WebKit Review Bot 2012-05-20 12:49:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Raphael Kubo da Costa (:rakuco) 2012-05-20 14:36:00 PDT
This commit appears to have caused a test regression the EFL 64-bit Debug and GTK+ 64-bit Release bots.

The EFL bot reports the following:

--- /home/buildslave-1/webkit-buildslave/efl-linux-debug/build/layout-test-results/svg/hittest/svg-shapes-non-scale-stroke-expected.txt
+++ /home/buildslave-1/webkit-buildslave/efl-linux-debug/build/layout-test-results/svg/hittest/svg-shapes-non-scale-stroke-actual.txt
@@ -5,7 +5,7 @@
 PASS ellipse1 stroke contains point at (29, 37)
 PASS ellipse2 stroke contains point at (279, 37)
 PASS ellipse1 stroke contains point at (34, 48)
-PASS ellipse2 stroke contains point at (284, 48)
+FAIL ellipse2 stroke contains point at (284, 48)
 PASS ellipse1 stroke contains point at (94, 70)
 PASS ellipse2 stroke contains point at (344, 70)
 PASS ellipse1 stroke contains point at (100, 78)
Comment 21 Philip Rogers 2012-05-20 14:39:41 PDT
(In reply to comment #20)
> This commit appears to have caused a test regression the EFL 64-bit Debug and GTK+ 64-bit Release bots.
> 
> The EFL bot reports the following:
> 
> --- /home/buildslave-1/webkit-buildslave/efl-linux-debug/build/layout-test-results/svg/hittest/svg-shapes-non-scale-stroke-expected.txt
> +++ /home/buildslave-1/webkit-buildslave/efl-linux-debug/build/layout-test-results/svg/hittest/svg-shapes-non-scale-stroke-actual.txt
> @@ -5,7 +5,7 @@
>  PASS ellipse1 stroke contains point at (29, 37)
>  PASS ellipse2 stroke contains point at (279, 37)
>  PASS ellipse1 stroke contains point at (34, 48)
> -PASS ellipse2 stroke contains point at (284, 48)
> +FAIL ellipse2 stroke contains point at (284, 48)
>  PASS ellipse1 stroke contains point at (94, 70)
>  PASS ellipse2 stroke contains point at (344, 70)
>  PASS ellipse1 stroke contains point at (100, 78)

Ack, probably due to a hit testing bug in platform-specific code. Lets skip it (preparing patch now).