Bug 78980 – SVG error parsing empty path

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

We currently throw an error when parsing an empty path:
<svg xmlns="http://www.w3.org/2000/svg" height="100" width="100">
<path d=""/>
</svg>

Error: Problem parsing d=""


According to the spec (http://www.w3.org/TR/SVG/paths.html#PathData):
"Note that the BNF allows the path 'd' attribute to be empty. This is not an error, instead it disables rendering of the path."

[Joe Thomas]

Created attachment 128506 [details]
ProposedPatch

[Philip Rogers]

Overall this change looks good to me, though I think there may be a corner case dealing with animated paths that you may want to consider:
<path d="M700,200 h-150 a150,150 0 1,0 150,-150 z" fill="red" stroke="blue" stroke-width="5">
<animate attributeName="d" from="" to="" dur="1s" repeatCount="indefinite" />
</path>

In this example I think the path should become disabled, but I think an argument could be made the other way. FWIW, Firefox does disable the path in this case but we don't.

[Joe Thomas]

(In reply to comment #2)
> Overall this change looks good to me, though I think there may be a corner case dealing with animated paths that you may want to consider:
> <path d="M700,200 h-150 a150,150 0 1,0 150,-150 z" fill="red" stroke="blue" stroke-width="5">
> <animate attributeName="d" from="" to="" dur="1s" repeatCount="indefinite" />
> </path>
> 
> In this example I think the path should become disabled, but I think an argument could be made the other way. FWIW, Firefox does disable the path in this case but we don't.

I feel that webkit behavior is correct. If 'from' and 'to' values are empty, we should respect the 'path data' value in "path" element for better UX.

FYI, If I apply animation to 'opacity' with 'from' and 'to' empty, then firefox respects the opacity attribute value mentioned in the path element.

<path d="M500,200 h-150 a150,150 0 1,0 150,-150  z" opacity=".5" fill="red" stroke="blue" stroke-width="5" >
<animate attributeName="opacity" from="" to="" dur="1s" repeatCount="indefinite" /> </path>

I am not an expert in this area, please correct me if I am wrong. Also, it might be good to handle this corner case in a different bug.

[Philip Rogers]

(In reply to comment #3)
> (In reply to comment #2)
> > Overall this change looks good to me, though I think there may be a corner case dealing with animated paths that you may want to consider:
> > <path d="M700,200 h-150 a150,150 0 1,0 150,-150 z" fill="red" stroke="blue" stroke-width="5">
> > <animate attributeName="d" from="" to="" dur="1s" repeatCount="indefinite" />
> > </path>
> > 
> > In this example I think the path should become disabled, but I think an argument could be made the other way. FWIW, Firefox does disable the path in this case but we don't.
> 
> I feel that webkit behavior is correct. If 'from' and 'to' values are empty, we should respect the 'path data' value in "path" element for better UX.
> 
> FYI, If I apply animation to 'opacity' with 'from' and 'to' empty, then firefox respects the opacity attribute value mentioned in the path element.
> 
> <path d="M500,200 h-150 a150,150 0 1,0 150,-150  z" opacity=".5" fill="red" stroke="blue" stroke-width="5" >
> <animate attributeName="opacity" from="" to="" dur="1s" repeatCount="indefinite" /> </path>

I don't think this is valid. The reason opacity is different is that values of "" are not valid--opacity should be a number--but "" is a valid path. In your opacity example you are passing an invalid opacity value and I suspect the animation is considered invalid, and is simply ignored. Whereas for path animations I think "" should not be ignored because it is a valid path.

> 
> I am not an expert in this area, please correct me if I am wrong. Also, it might be good to handle this corner case in a different bug.

I don't know a whole lot about this code either so please just consider my point a friendly suggestion :) My feeling is that your fix for this bug is too local--you've skipped the error printing but the real bug is that empty paths should not be treated as errors. That said, I am not a reviewer so please do wait for someone with a little more knowledge in this area to respond before doing much work.

[Nikolas Zimmermann]

Comment on attachment 128506 [details]
ProposedPatch

Hi Joe, thanks for the patch! Phillip is right, that this is rather a work-around than a fix.
If we want to treat an empty path as valid, SVGPathParserFactor::buildXXXFromString() needs to be fixed.
There are various ways to convert a SVGPathByteStream to a String, and/or a SVGPathSegList - I guess you'd need to inspect all these code paths, to make sure we deal with empty paths correctly.

Also your change lacks test coverage, where this actually affects a test, despite the error printing which is now gone.
Can you investigate more?

[Joe Thomas]

Thanks Philip and Nikolas for the clarification. I will investigate more on it.

*** Bug 110691 has been marked as a duplicate of this bug. ***

Just hit this bug myself and noticed there have been no updates for a year. Anything I can do to help get the fix accepted?

I'm setting d to "M0 0" in the meantime, in case this helps anyone else looking for a workaround.

[Rob Buis]

Created attachment 210045 [details]
Patch

[Build Bot]

Comment on attachment 210045 [details]
Patch

Attachment 210045 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1627740

New failing tests:
svg/custom/dynamic-empty-path.svg
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 210053 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Rob Buis]

Created attachment 210055 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 210055 [details]
Patch

Clearing flags on attachment: 210055

Committed r154896: <http://trac.webkit.org/changeset/154896>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.