Bug 146997 – [Cairo] SVG path not rendered with all-zero dasharray

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Jinyoung Hur]

SVG path with "stroke-dasharray:0" style is not rendered at all.
This is because cairo_set_dash() doesn't expect all-zero dashes.

W3C SVG v1.1 spec says, "If the sum of the values is zero, then the stroke is rendered as if a value of none were specified."
So, correct rendering would be achieved if cairo_set_dash() is skipped. 

Also, from the FireFox patch[1], I recognized that CanvasRenderingContext2D.setLineDash() has the same problem because it also uses the same cairo API.
Though, I couldn't find any reference about all-zero dash array in Canvas 2D Context spec, it would be better to be rendered as a solid line rather than nothing.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1169609

[Jinyoung Hur]

Created attachment 256901 [details]
Patch

[Jinyoung Hur]

Created attachment 256903 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 256903 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [WinCairo] SVG path not rendered with all-zero dasharray

This patch influences on EFL and GTK ports as well. So, Please remove *Win* prefix.

[Gyuyoung Kim]

CC'ring Martin and Alex.

[Martin Robinson]

Comment on attachment 256903 [details]
Patch

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

Nice. Fix. I have a few small nits.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:907
> +    // Avoid all-zero patterns that would trigger the CAIRO_STATUS_INVALID_DASH context error state.
> +    bool allZero = true;
> +    for (auto& dash : dashes) {
> +        if (dash) {
> +            allZero = false;
> +            break;
> +        }

I think you could pull this bit out into a helper function that returns a boolean. Something like isDashArrayAllZero.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:911
> +    if (allZero)
> +        return;
> +

What should the behavior of an all-zero dash array be? Should it disable dashes? If so, perhaps in that case you should call cairo_set_dash with num_dashes=0. Either way, I think you need to adjust the state appropriately.

> LayoutTests/fast/canvas/script-tests/canvas-lineDash.js:107
> +// Verify all-zero dash rendering

Nit: missing period.

> LayoutTests/fast/canvas/script-tests/canvas-lineDash.js:110
> +ctx.lineWidth = 4; // To make the test immune to plaform anti-aliasing discrepancies

Ditto.

[Jinyoung Hur]

Created attachment 257263 [details]
Patch

[Jinyoung Hur]

Hi, Martin.
I've uploaded new patch. Thanks!

[Alex Christensen]

Comment on attachment 257263 [details]
Patch

Can the DashArray be empty?

[WebKit Commit Bot]

Comment on attachment 257263 [details]
Patch

Clearing flags on attachment: 257263

Committed r187171: <http://trac.webkit.org/changeset/187171>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jinyoung Hur]

(In reply to comment #8)
> Comment on attachment 257263 [details]
> Patch
> 
> Can the DashArray be empty?

Yes. According to the comment in cairo.c, num_dashes=0 disables dashing.

/**
 * cairo_set_dash:
 * @cr: a cairo context
 * @dashes: an array specifying alternate lengths of on and off stroke portions
 * @num_dashes: the length of the dashes array
 * @offset: an offset into the dash pattern at which the stroke should start
 *
 * Sets the dash pattern to be used by cairo_stroke(). A dash pattern
 * is specified by @dashes, an array of positive values. Each value
 * provides the length of alternate "on" and "off" portions of the
 * stroke. The @offset specifies an offset into the pattern at which
 * the stroke begins.
 *
 * Each "on" segment will have caps applied as if the segment were a
 * separate sub-path. In particular, it is valid to use an "on" length
 * of 0.0 with %CAIRO_LINE_CAP_ROUND or %CAIRO_LINE_CAP_SQUARE in order
 * to distributed dots or squares along a path.
 *
 * Note: The length values are in user-space units as evaluated at the
 * time of stroking. This is not necessarily the same as the user
 * space at the time of cairo_set_dash().
 *
 * If @num_dashes is 0 dashing is disabled.
 *
 * If @num_dashes is 1 a symmetric pattern is assumed with alternating
 * on and off portions of the size specified by the single value in
 * @dashes.
 *
 * If any value in @dashes is negative, or if all values are 0, then
 * @cr will be put into an error state with a status of
 * %CAIRO_STATUS_INVALID_DASH.
 *
 * Since: 1.0
 **/