1187770 - Zero-length SVG lines with stroke-linecap="square" don't render

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Tom Klein]

Attachment: testcase.svg

This is a cairo bug (so mostly seen on linux).  A zero-length svg line with stroke-linecap="square" should display a square centered at the start point = end point.

Expected results: Two lime squares.

Actual results: Nothing displayed.

Comment 1

[Tom Klein]

Attachment: cairo patch.diff

The testcase (without this patch) works under cairo if stroke-linecap="round" instead of "square", and it works for both square and round linecaps for firefox using D2D on Windows and with chrome and opera on linux.

The attached patch seems to fix this one, and it passes the svg reftests and mochitests, but that's as much testing as I've done, sorry.

Feel free to take this one.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I believe the current behaviour of Cairo matches what PDF does. I'm not in a huge rush to change it. I'm also surprised that this works in Chrome as I didn't think they supported degenerate dashes at all. Tom, can you look at how Chrome implements this?

Comment 3

[Tom Klein]

Attachment: tests.svg

I added some more tests to cover: zero-length lines, path with zero-length subpath, and line with zero-length dashes, all for square and round linecaps.

Cairo renders in all cases except for the zero-length lines with square linecaps.

Chrome doesn't render dashing with either linecap, but does render the zero-length line and zero-length subpath.

I didn't spend much time looking, but chrome does appear to have explicit support for zero-length subpaths:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/layout/svg/LayoutSVGPath.cpp&sq=package:chromium&rcl=1438037135&l=149
Apparently that doesn't include dashing though.

Also saw this bug discussing moving zero-length subpath support to skia:
https://code.google.com/p/chromium/issues/detail?id=422974&q=zero-length%20subpath&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Cr%20Status%20Owner%20Summary%20OS%20Modified

Comment 4

[Jonathan Kew [:jfkthame]]

As far as cairo is concerned, I believe this is by design, since it's unclear what the result should be. See case (2) in the "Note" at http://cairographics.org/manual/cairo-cairo-t.html#cairo-stroke.

Comment 5

[Tom Klein]

Right, okay, sorry, I was confused by being unaware of the work done in bug 589648: we're artificially lengthening the zero-length subpath in my test above so that cairo will render it.

I guess we need to do something similar for lines.

Moving to SVG component.

Comment 6

[Tom Klein]

Attachment: patch.diff

Comment 7

[Robert Longson [:longsonr]]

Comment on attachment 8641926 [details] [diff] [review]
patch.diff

content (i.e. dom/svg) should not depend on layout/svg (it's the other way round). So can you move the nsSVGUtils stuff to SVGContentUtils please (but see below as the method you're adding is unnecessary)?

> 
>+void
>+SVGLineElement::MaybeAdjustForZeroLength(float aX1, float aY1,
>+                                         float* aX2, float aY2)

I think x2 would be better passed as a reference than a pointer

>+void
>+nsSVGUtils::GetPathStyleData(nsSVGPathGeometryElement* aContent,
>+                             uint8_t* aStrokeLineCap,
>+                             Float* aStrokeWidth)

Don't do this use the existing SVGContentUtils::GetStrokeOptions instead.

On the right lines, but would like to see another patch here hence the r-

Comment 8

[Tom Klein]

Attachment: Patch v2

Comment 9

[Tom Klein]

Thanks Robert.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83ba87149b69

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bea2f46be0f1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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