As the description says, non-scaling-strokes are not correctly handing hit testing on the stroke.
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
(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.
Oh, right -- I've missed the last two "FAIL" lines. Thanks for noticing this!
(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!
Created attachment 138574 [details] Update svg-ellipse-non-scale-stroke expectations
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>
All reviewed patches have been landed. Closing bug.
(In reply to comment #7) > All reviewed patches have been landed. Closing bug. Reopening because the bug is not fixed.
Created attachment 140550 [details] Fix hit testing on non-scaling strokes
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 :)
Bump. Niko or Rob, do you have time for this? Anyone else?
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?
Created attachment 142887 [details] Update per reviewer comments
(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 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.
Created attachment 142910 [details] Patch for landing
(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 on attachment 142910 [details] Patch for landing Clearing flags on attachment: 142910 Committed r117709: <http://trac.webkit.org/changeset/117709>
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)
(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).