Summary: | SVG error parsing empty path | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Rogers <pdr> | ||||||||||
Component: | SVG | Assignee: | Philip Rogers <pdr> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, d-r, fmalita, joethomas, krit, _pants, rniwa, schenney, zimmermann | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Philip Rogers
2012-02-18 16:53:04 PST
Created attachment 128506 [details]
ProposedPatch
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. (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. (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 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?
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. Created attachment 210045 [details]
Patch
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 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
Created attachment 210055 [details]
Patch
Comment on attachment 210055 [details] Patch Clearing flags on attachment: 210055 Committed r154896: <http://trac.webkit.org/changeset/154896> All reviewed patches have been landed. Closing bug. |