938569 - Path fails to render, with comma between move command and implied line command

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Zéfling]

Attachment: Rendu.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20131113095254

Steps to reproduce:

The render in Nighlty is not good for this SVG : 
http://ikilote.net/Template/4.0/XHTML5/Defaut/Image/Logo.svg

In Fx25, this SVG file works perfectly.

Comment 1

[Robert Longson [:longsonr]]

Would you be willing to find a regression range?

Comment 2

[Alice0775 White]

Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=936a5d4b340d&tochange=fcf5c8adaf14
Regressed by:
fcf5c8adaf14	Robert Longson — Bug 929011 - Simplify path and transform parsing. r=dholbert

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: reporter's testcase

Comment 4

[Robert Longson [:longsonr]]

Attachment: only a comma short of perfect

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 832474 [details] [diff] [review]
only a comma short of perfect

>diff --git a/content/svg/content/src/nsSVGPathDataParser.cpp b/content/svg/content/src/nsSVGPathDataParser.cpp
>--- a/content/svg/content/src/nsSVGPathDataParser.cpp
>+++ b/content/svg/content/src/nsSVGPathDataParser.cpp
>@@ -148,16 +148,18 @@ nsSVGPathDataParser::ParseMoveto()
>     return false;
>   }
> 
>   if (!SkipWsp() || IsAlpha(*mIter)) {
>     // End of data, or start of a new command
>     return true;
>   }
> 
>+  SkipCommaWsp();
>+
>   // Per SVG 1.1 Section 8.3.2
>   // If a moveto is followed by multiple pairs of coordinates,
>   // the subsequent pairs are treated as implicit lineto commands
>   return ParseLineto(absCoords);


I think we actually want to change the existing SkipWsp to SkipCommaWsp, right?

Otherwise, I think a variant of your reftest with an explicit "L" (or another explicit command) after the comma would still fail.
e.g.
  <path d="M0,0, L100,0,100,100,0,100z" fill="lime" shape-rendering="crispEdges"/>

Comment 6

[Robert Longson [:longsonr]]

Your example is invalid and *should* fail.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 832474 [details] [diff] [review]
only a comma short of perfect

You're right. (I was misled by the fact that both Presto and Blink incorrectly think my example is valid. :) )

Thanks for the sanity-check.

r=me

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> You're right. (I was misled by the fact that both Presto and Blink
> incorrectly think my example is valid. :) )

For the benefit of anyone else who might be confused about this: "M0,0, 100,100" is valid (with a comma after the 0,0) because moveto-argument-sequence is defined with an optional comma:
 # moveto-argument-sequence:
 #     coordinate-pair
 #     | coordinate-pair comma-wsp? lineto-argument-sequence

BUT a move followed by an explicit command like does *not* get an optional comma:
 # moveto-drawto-command-group:
 #     moveto wsp* drawto-commands?

(There, "drawto-commands" represents any drawing command that starts with an explicit letter)

So "M0,0, L100,100" is invalid (as is my example in comment 5), though it becomes valid if you drop the "L".

http://www.w3.org/TR/SVG/paths.html#PathDataBNF

Comment 9

[Daniel Holbert [:dholbert]]

s/explicit command like/explicit command letter/

Comment 10

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/717711fc6d5d

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/717711fc6d5d