Bug 37413 – Render SVG Paths up to first error

• Component: SVG
• Status: RESOLVED FIXED

[Jeff Schiller]

As part of their efforts for IE9, Microsoft has written several SVG tests.  Some of these do not pass in WebKit.

This bug addresses the following test:

http://samples.msdn.microsoft.com/ietestcenter/svg112/svg_harness.htm?url=./svg112/svg/chapter_08.2.svg

which tests the requirement in the SVG 1.1 specification:

http://dev.w3.org/SVG/profiles/1.1F2/publish/implnote.html#PathElementImplementationNotes

"The general rule for error handling in path data is that the SVG user agent shall render a ‘path’ element up to (but not including) the path command containing the first error in the path data specification."

WebKit's SVG parser tosses out the entire path if any errors are encountered and thus we see a red path on the fourth subtest.

I will attach a simple patch, with some LayoutTest stuff updated.  However, it will require some reworking of the dom/svg/fuzz-path-parser.html test so I wanted to see if folks agree with the fix first.

[Jeff Schiller]

Created attachment 53091 [details]
Patch to the path parser so that paths are rendered up to the first error.  

This is just the initial patch.  The fuzz path layout test not yet updated.

[Jeff Schiller]

Created attachment 53092 [details]
Diff of expected-actual results for dom/svg/fuzz-path-parser layout test

Is it ok to rewrite the expected results for this test to match the diff "Parsed as N command(s)..." results?  The difference is because now the parser creates segments/commands up until the first error and does not toss out the entire path.

I'm not clear on whether the expected results are manually created or via script.  I'm also not clear on just how random things are (scripted-random.js suggests that it's not really random, just pseudo-random).

[Jeff Schiller]

Created attachment 53119 [details]
Patch for parser and layout tests.

Patch which fixes the SVG path parser, updates a layout test and changes the expected output of the svg/dom/fuzz-path-parser layout test.  The reason the fuzz-path-parser layout test results change so drastically is that the paths are processed up to the first encountered error.

[Dirk Schulze]

(In reply to comment #3)
> Created an attachment (id=53119) [details]
> Patch for parser and layout tests.
> 
> Patch which fixes the SVG path parser, updates a layout test and changes the
> expected output of the svg/dom/fuzz-path-parser layout test.  The reason the
> fuzz-path-parser layout test results change so drastically is that the paths
> are processed up to the first encountered error.

I think this is a big mistake in the Spec and I'm not sure if this behavior helps the developer... 
Nevertheless, it's better that the Spec has an error handling now.

There is something that I don't like. I think the script should give a response, that an error occured during the parsing, but the path got build up to this point. Is it possible to modify the script to do that?

[Dirk Schulze]

(In reply to comment #4)
> (In reply to comment #3)
> > Created an attachment (id=53119) [details] [details]
> > Patch for parser and layout tests.
> > 
> > Patch which fixes the SVG path parser, updates a layout test and changes the
> > expected output of the svg/dom/fuzz-path-parser layout test.  The reason the
> > fuzz-path-parser layout test results change so drastically is that the paths
> > are processed up to the first encountered error.
> 
> I think this is a big mistake in the Spec and I'm not sure if this behavior
> helps the developer... 
> Nevertheless, it's better that the Spec has an error handling now.
> 
> There is something that I don't like. I think the script should give a
> response, that an error occured during the parsing, but the path got build up
> to this point. Is it possible to modify the script to do that?

Hm. Maybe the 'Parsed as' command is enough. Since it tells up to which point the path got parsed.

[Jeff Schiller]

Hi Dirk,

The SVG spec has always been clear on the error handling for paths, even going back to SVG 1.0: http://www.w3.org/TR/SVG10/implnote.html#PathElementImplementationNotes from 2001

WebKit has just not been compliant in this regard

Personally, I think displaying partial path is better than displaying nothing.  Plus it's what other browsers do.

If you like, I can investigating updating the test script to indicate that some error was encountered in the updated patch that I'll have to do after Bug 37431 gets checked in.

[Dirk Schulze]

(In reply to comment #6)
> Hi Dirk,
> 
> The SVG spec has always been clear on the error handling for paths, even going
> back to SVG 1.0:
> http://www.w3.org/TR/SVG10/implnote.html#PathElementImplementationNotes from
> 2001
> 
> WebKit has just not been compliant in this regard
> 
> Personally, I think displaying partial path is better than displaying nothing. 
> Plus it's what other browsers do.
> 
> If you like, I can investigating updating the test script to indicate that some
> error was encountered in the updated patch that I'll have to do after Bug 37431
> gets checked in.

Would be great if you can update the results now, that 37431 got landed. You don't need to change the script, if it is to complicated to catch an early break on parsing. But it could be helpful, if the test mention this case.

[Jeff Schiller]

Created attachment 53204 [details]
Updated patch to modify layout test results after Bug 37431 landed

[Jeff Schiller]

Comment on attachment 53204 [details]
Updated patch to modify layout test results after Bug 37431 landed

FYI, I don't know if I should update chromium Layout Test changes.

[Dirk Schulze]

(In reply to comment #9)
> (From update of attachment 53204 [details])
> FYI, I don't know if I should update chromium Layout Test changes.

Jeff, you told me via mail, that one or more pixel tests would fail after your patch. If you're sure, that the new results are more correct, you have to replace them. If you're working on Mac, you can use the webkit tools for that:

WebKitTools/Scripts/run-webkit-tests -p --reset-results --tolerance=0 LayoutTests/svg/

This will reset all results, that are different to the expected one by the actual results in the svg folder.

[Jeff Schiller]

Created attachment 53331 [details]
Patch to the path parser so that paths are rendered up to the first error plus updates to layout tests

I updated the test so that the pixel results would not change.  Latest patch should be good to go, can someone review/commit?

[Dirk Schulze]

Comment on attachment 53331 [details]
Patch to the path parser so that paths are rendered up to the first error plus updates to layout tests

LGTM. r=me.

[WebKit Commit Bot]

Comment on attachment 53331 [details]
Patch to the path parser so that paths are rendered up to the first error plus updates to layout tests

Clearing flags on attachment: 53331

Committed r57591: <http://trac.webkit.org/changeset/57591>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.