Bug 78980 - SVG error parsing empty path
: SVG error parsing empty path
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-02-18 16:53 PST by
Modified: 2013-08-30 10:50 PST (History)


Attachments
ProposedPatch (7.29 KB, patch)
2012-02-23 10:59 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.01 KB, patch)
2013-08-29 16:32 PST, Rob Buis
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (457.26 KB, application/zip)
2013-08-29 17:21 PST, Build Bot
no flags Details
Patch (13.56 KB, patch)
2013-08-29 17:33 PST, Rob Buis
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-18 16:53:04 PST
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."
------- Comment #1 From 2012-02-23 10:59:55 PST -------
Created an attachment (id=128506) [details]
ProposedPatch
------- Comment #2 From 2012-02-23 11:40:20 PST -------
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.
------- Comment #3 From 2012-02-23 13:11:30 PST -------
(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.
------- Comment #4 From 2012-02-23 13:48:15 PST -------
(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.
------- Comment #5 From 2012-02-24 00:47:31 PST -------
(From update of attachment 128506 [details])
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?
------- Comment #6 From 2012-02-24 10:17:56 PST -------
Thanks Philip and Nikolas for the clarification. I will investigate more on it.
------- Comment #7 From 2013-02-23 14:09:15 PST -------
*** Bug 110691 has been marked as a duplicate of this bug. ***
------- Comment #8 From 2013-02-23 14:12:49 PST -------
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?
------- Comment #9 From 2013-02-23 14:16:38 PST -------
I'm setting d to "M0 0" in the meantime, in case this helps anyone else looking for a workaround.
------- Comment #10 From 2013-08-29 16:32:02 PST -------
Created an attachment (id=210045) [details]
Patch
------- Comment #11 From 2013-08-29 17:21:51 PST -------
(From update of attachment 210045 [details])
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
------- Comment #12 From 2013-08-29 17:21:54 PST -------
Created an attachment (id=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
------- Comment #13 From 2013-08-29 17:33:37 PST -------
Created an attachment (id=210055) [details]
Patch
------- Comment #14 From 2013-08-30 10:50:52 PST -------
(From update of attachment 210055 [details])
Clearing flags on attachment: 210055

Committed r154896: <http://trac.webkit.org/changeset/154896>
------- Comment #15 From 2013-08-30 10:50:55 PST -------
All reviewed patches have been landed.  Closing bug.