Bug 62203 – stroke-dasharray does not handle "none"

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

[Dirk Schulze]

stroke-dasharray should handle value 'none' but doesn't. This is a parsing bug in parseSVGStrokeDasharray().

See http://trac.webkit.org/browser/trunk/Source/WebCore/css/SVGCSSParser.cpp#L316

I do not plan to work on that. Rob, isn't that something for you? Shouldn't be hard to fix.

We just need a check like in CSSParser.cpp:

if (value->id == CSSValueNone || (value->unit == CSSPrimitiveValue::CSS_STRING && equalIgnoringCase(value->string, "none"))) {
            return primitiveValueCache()->createIdentifierValue(CSSValueNone);

But because we have a comma separated list we have to check for something like "none,3" as well. This would be an invalid string.

[Rob Buis]

Hi Dirk,

(In reply to comment #0)
> stroke-dasharray should handle value 'none' but doesn't. This is a parsing bug in parseSVGStrokeDasharray().
> See http://trac.webkit.org/browser/trunk/Source/WebCore/css/SVGCSSParser.cpp#L316
> I do not plan to work on that. Rob, isn't that something for you? Shouldn't be hard to fix.
> We just need a check like in CSSParser.cpp:
> if (value->id == CSSValueNone || (value->unit == CSSPrimitiveValue::CSS_STRING && equalIgnoringCase(value->string, "none"))) {
>             return primitiveValueCache()->createIdentifierValue(CSSValueNone);
> But because we have a comma separated list we have to check for something like "none,3" as well. This would be an invalid string.

Good find! Yes I'll take a look.
Cheers,

Rob.

[Rob Buis]

Created attachment 96264 [details]
Patch

[Rob Buis]

Created attachment 96272 [details]
Patch

[Darin Adler]

Comment on attachment 96264 [details]
Patch

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

> Source/WebCore/css/SVGCSSParser.cpp:245
> +            parsedValue = CSSValueList::createCommaSeparated();

This seems like a roundabout way to achieve tis. While it’s true that we use an empty vector to represent an empty dash array, creating an empty CSS value list for parsing purposes is not the normal way we handle such things. Shouldn’t the fix be in SVGCSSStyleSelector.cpp instead?

> Source/WebCore/css/SVGCSSParser.cpp:248
> +        }
>          else

In WebKit coding style we put the brace on the same line as the else.

[Rob Buis]

Hi Darin,

(In reply to comment #4)
> (From update of attachment 96264 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96264&action=review
> 
> > Source/WebCore/css/SVGCSSParser.cpp:245
> > +            parsedValue = CSSValueList::createCommaSeparated();
> 
> This seems like a roundabout way to achieve tis. While it’s true that we use an empty vector to represent an empty dash array, creating an empty CSS value list for parsing purposes is not the normal way we handle such things. Shouldn’t the fix be in SVGCSSStyleSelector.cpp instead?

You are right, I started with that approach, not sure why I changed it later.

> > Source/WebCore/css/SVGCSSParser.cpp:248
> > +        }
> >          else
> 
> In WebKit coding style we put the brace on the same line as the else.

Strange that the style checker didn't get it.
Thanks for the review, new patch coming up.
Cheers,

Rob.

[Rob Buis]

Created attachment 96278 [details]
Patch

[Rob Buis]

Created attachment 96288 [details]
Patch

[Dirk Schulze]

Comment on attachment 96288 [details]
Patch

LGTM. r=me.

[Rob Buis]

Committed r88266: <http://trac.webkit.org/changeset/88266>