Bug 82628 – Hit testing on a stroke with vector-effects:non-scaling-stroke is broken

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

As the description says, non-scaling-strokes are not correctly handing hit testing on the stroke.

[Mikhail Naganov]

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

[Philip Rogers]

(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 [e-mail]. 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.

[Mikhail Naganov]

Oh, right -- I've missed the last two "FAIL" lines. Thanks for noticing this!

[Philippe Normand]

(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 [e-mail]. 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!

[Philip Rogers]

Created attachment 138574 [details]
Update svg-ellipse-non-scale-stroke expectations

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philip Rogers]

(In reply to comment #7)
> All reviewed patches have been landed.  Closing bug.

Reopening because the bug is not fixed.

[Philip Rogers]

Created attachment 140550 [details]
Fix hit testing on non-scaling strokes

[Philip Rogers]

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 :)

[Stephen Chenney]

Bump. Niko or Rob, do you have time for this? Anyone else?

[Nikolas Zimmermann]

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?

[Philip Rogers]

Created attachment 142887 [details]
Update per reviewer comments

[Philip Rogers]

(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.

[Nikolas Zimmermann]

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.

[Philip Rogers]

Created attachment 142910 [details]
Patch for landing

[Philip Rogers]

(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!

[WebKit Review Bot]

Comment on attachment 142910 [details]
Patch for landing

Clearing flags on attachment: 142910

Committed r117709: <http://trac.webkit.org/changeset/117709>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

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)

[Philip Rogers]

(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).