Bug 37431

Summary: Make comma/whitespace around arc flags optional in SVG path syntax
Product: WebKit Reporter: Jeff Schiller <jeffschiller>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, krit, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Test case showing that comma/whitespace is optional between and after arc flags
none
Test case showing that comma/whitespace is optional between and after arc flags and that flags should only be 0 or 1
none
Fix to the parser to only accept 0,1 for arc flags and to make comma/whitespace after the flag be optional none

Description Jeff Schiller 2010-04-11 19:52:03 PDT
Created attachment 53136 [details]
Test case showing that comma/whitespace is optional between and after arc flags

As per recent W3C SVG WG discussion: http://lists.w3.org/Archives/Public/www-svg/2010Apr/0042.html

All other browsers (Firefox, Opera, Batik, IE9) treat the whitespace/comma between largeArcFlag and sweepFlag in the SVG arc path command syntax as optional.  They also treat the whitespace/comma between sweepFlag and the coordinate pair as optional.  

Finally only values of "0" and "1" are allowed for the largeArcFlag and sweepFlag as per the path grammar: http://dev.w3.org/SVG/profiles/1.1F2/publish/paths.html#PathDataBNF

I am attaching a test case.  This test case renders all paths as green in Firefox, Opera and IE9.  This test case renders all paths as red in WebKit-based browsers. 

I would like to suggest that WebKit align with what other browsers are doing, especially considering that the WG considers this as 'proper' behavior.
Comment 1 Jeff Schiller 2010-04-11 19:56:11 PDT
Created attachment 53139 [details]
Test case showing that comma/whitespace is optional between and after arc flags and that flags should only be 0 or 1
Comment 2 Jeff Schiller 2010-04-11 22:05:35 PDT
Created attachment 53146 [details]
Fix to the parser to only accept 0,1 for arc flags and to make comma/whitespace after the flag be optional

Test file shows all green paths after applying the patch and all LayoutTests pass.
Comment 3 Dirk Schulze 2010-04-11 23:11:13 PDT
Looks good to me. Since this is still in discussion and it is not sure if this moves into SVG1.1SE or SVG2.0, I think we shouldn't land this patch yet. I know, that this could be landed because of compatibility reasons, but let us wait some days for the responses of the SVG WG members.
I'll clear the review flag for the moment. Feel free to set it again after more clearness on this topic later.
Comment 4 Jeff Schiller 2010-04-12 05:33:41 PDT
The SVG 1.1 2nd Edition spec has been updated to support the grammar as outlined in this bug: http://dev.w3.org/SVG/profiles/1.1F2/publish/paths.html#PathDataBNF

SVG WG decision: http://lists.w3.org/Archives/Public/www-svg/2010Apr/0046.html
Comment 5 Jeff Schiller 2010-04-12 07:55:21 PDT
Incidentally, WebKit with this patch correctly displays the following test case from the official SVG 1.1 2nd Edition Test Suite: http://dev.w3.org/SVG/profiles/1.1F2/test/svg/paths-data-20-f.svg
Comment 6 Dirk Schulze 2010-04-12 08:21:09 PDT
Comment on attachment 53146 [details]
Fix to the parser to only accept 0,1 for arc flags and to make comma/whitespace after the flag be optional

LGTM. r=me.
Comment 7 WebKit Commit Bot 2010-04-12 08:50:00 PDT
Comment on attachment 53146 [details]
Fix to the parser to only accept 0,1 for arc flags and to make comma/whitespace after the flag be optional

Clearing flags on attachment: 53146

Committed r57473: <http://trac.webkit.org/changeset/57473>
Comment 8 WebKit Commit Bot 2010-04-12 08:50:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Dirk Schulze 2010-04-12 09:04:12 PDT
This patch causes a break of the SL bot http://build.webkit.org/builders/SnowLeopard Intel Leaks/builds/5883/steps/compile-webkit/logs/stdio
Comment 10 Dirk Schulze 2010-04-12 09:07:37 PDT
(In reply to comment #9)
> This patch causes a break of the SL bot
> http://build.webkit.org/builders/SnowLeopard Intel
> Leaks/builds/5883/steps/compile-webkit/logs/stdio

cc1plus: warnings being treated as errors
/Volumes/Data/WebKit-BuildSlave/snowleopard-intel-leaks/build/WebCore/svg/SVGParserUtilities.cpp:145: warning: no previous prototype for 'bool WebCore::parseArcFlag(const UChar*&, const UChar*, bool&)'
Comment 11 Jeff Schiller 2010-04-12 09:11:16 PDT
Ok, I'm not sure I can test this out as I don't have SL.  Is it just a matter of adding a function prototype to the header?  I declared the function before invoking it, but I guess that's not good enough?
Comment 12 Dirk Schulze 2010-04-12 09:51:43 PDT
Landed SL build bix in http://trac.webkit.org/changeset/57474