1242147 - SVG stroke-linecap doesn't work for circles in Firefox

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[ketner.j]

Attachment: svg.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160106234723

Steps to reproduce:

I've got a problem with svg stroke-linecap attribute. I've got circular progress bar in AngularJS and I would like to set the upper circle (blue one) to have a rounded "ends". I'm submitting my svg declaration in file.


Actual results:

It doesn't render rounded end of lines.


Expected results:

It should render rounded end of lines as it's in Chrome browser.

Comment 1

[Tom Klein]

Attachment: circle/ellipse undashed/dashed stroke testcase.html

I think we want to keep the bug 1178159 behavior if the stroke isn't dashed, but use whatever stroke-linecap the user requests when the stroke of a circle or ellipse *is* dashed.  (No idea why I didn't do that in the first place... :/)  That's what chrome does.

Comment 2

[Tom Klein]

Attachment: Patch v1

The issue with the desired behavior described in bug 1 is that that means that the stroke-linecap used to render a circle or ellipse depends on the dash stroke, which is at odds with SVGContentUtils::GetStrokeOptions's  eIgnoreStrokeDashing aFlags option: if aFlags == eIgnoreStrokeDashing then we currently don't check whether we have dashing or not, and so we can't know which linecap style to set for the stroke.

The only case where that's currently an issue (i.e. where we call GetStrokeOptions for a circle/ellipse with aFlags == eIgnoreStrokeDashing) is in nsSVGPathGeometryFrame::GetBBoxContribution.  But given our current behavior in, for example, SVGCircleElement::GetGeometryBounds, where we ignore dashing and linecaps, it seems like we shouldn't need the linecap value for a circle/ellipse in GetBBoxContribution.  So this patch is for the case where that's true: where you don't actually need to know the linecap value when aFlags == eIgnoreStrokeDashing and aElement is a circle or ellipse.

What's giving me pause on whether or not that's really true is the example in my next comment...

Robert, I'm asking for feedback on how you feel about having a case where nsSVGPathGeometryFrame::GetBBoxContribution might not be giving the right linecap value.  The other option I see would be to pull out just the parts of GetStrokeDashData necessary to create a minimal StrokeIsDashed() function that would allow us to set the correct linecap for circles/ellipses even if aFlags == eIgnoreStrokeDashing.  But then the logic gets messier, and we might be doing work that's never actually needed.

Comment 3

[Tom Klein]

Attachment: a circle with (dash) corners.html

(You should view this testcase in chrome or with the patch applied to get the intended effect.)

So when the stroke is dashed, circles and ellipses actually *can* have corners if stroke-linecap="square" (or rounded corners that stick out if stroke-linecap="round"), as in the attachment, where the stroke extends outside the bounds of a non-dashed stroke.

That seems to be at odds with SVG[Circle|Ellipse]Element::GetGeometryBounds where we assume the bounds don't go outside the edge of a non-dashed stroke.  It also seems to be at odds with https://dxr.mozilla.org/mozilla-central/rev/d6d81655dd9e146c300a64c0fcaeb04ca3300a19/layout/svg/nsSVGUtils.cpp#1231
in nsSVGUtils::PathExtentsToMaxStrokeExtents.  We do indeed return the wrong bounds in getBoundingClientRect for this case.

I'm also wondering if having a too small mRect in this example will bite us in invalidation in some cases?  I need to look into that some more.

Those might all be issues for another bug or bugs - I think they all existed before this issue arose.

Comment 4

[Robert Longson [:longsonr]]

(In reply to Tom Klein from comment #3)
> Created attachment 8711560 [details]
> a circle with (dash) corners.html
> 
can you raise a follow up please. we'll get painting artifacts if we don't fix this. If the empty dash sections are big enough then the bounding box may be too large so perhaps we can't ignore the stroke dash at all for BBox calculations. Perhaps we could drop the flag there and use the nullptr for the stroke dash data to indicate that we want to know whether it's dashed, but not what the dashing is so we can use Moz2D's bounds calculation in that case.

Comment 5

[Tom Klein]

Comment on attachment 8711555 [details] [diff] [review]
Patch v1

We never actually use the StrokeOptions linecap value in GetBBoxContribution for a circle/ellipse, so I think this patch is safe in possibly returning the wrong linecap value from GetStrokeOptions.

I filed bug 1242829 as a followup so that we do consider linecap in bounds calculations for dashed stroke on circles and ellipses.

Comment 6

[Robert Longson [:longsonr]]

Should nominate this for aurora/beta after it lands as it's a regression from 42.

Comment 7

[Tom Klein]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=571ab9474a7c

Comment 8

[Tom Klein]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc44ca7bde11647ea2150da56d00f6dec6848139
Bug 1242147 - Honor stroke-linecap on SVG circle and ellipse when stroke is dashed. r=longsonr

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc44ca7bde11

Comment 10

[Tom Klein]

Attachment: Patch v1 with reviewer added to the patch description line

I don't know if it was necessary or not, but thought maybe I better attach the uplift request to the version of the patch on m-c (which just adds the reviewer in the description line to the original v1 patch).

Approval Request Comment
[Feature/regressing bug #]: bug 1178159
[User impact if declined]: dashed lines around svg circles and ellipses will render with the wrong linecaps for certain stroke-linecap values.
[Describe test coverage new/current, TreeHerder]: The patch includes a reftest.
[Risks and why]: Low risk; the patch only affects the style of the ends of dashes in dashed outlines for svg circles and ellipses.
[String/UUID change made/needed]: None.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a fairly recent regression.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8712833 [details] [diff] [review]
Patch v1 with reviewer added to the patch description line

Adds a test, fixes a recent regression. Please uplift to aurora and beta.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cfba3ed2daca

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/24026e3ea7c5

Comment 15

[Vuyisile Ndlovu [:terrameijar]]

Tested this on Nightly (Firefox 47).Circles and ellipses don't have corners in them anymore.

Comment 16

[Tummala Dhanvi [:c0mrad3][UTC+5.30]]

Verified as Fixed in Firefox Stable 45 Debian 8 GNU/Linux

Verified as Fixed in Firefox Beta 46.0b1 Debian 8 GNU/Linux

I have verified the fix using the first attachment and in both cases the blue circle has rounded ends