302971 - Scientific notation in stroke-width doesnt work

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[dave blasby]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

Using stroke-width="0.0005" works, but using stroke-width="5.0E-4" doesnt.

There might be other scientific notation problems elsewhere.


Reproducible: Always

Steps to Reproduce:
1. load the "works.svg" -- you'll get a few things on the screen
2. load the "not_works.svg" -- you'll get a pink screen
3. the only difference between the 2 files is the uses of scientific notation.



Expected Results:  
Both should work.

Comment 1

[dave blasby]

Attachment: svg file that works

Comment 2

[dave blasby]

Attachment: this is the svg file that doesnt work

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

http://www.w3.org/TR/SVG11/coords.html#Units doesn't say anything about this

http://www.w3.org/TR/CSS21/syndata.html#length-units disallows this

What says that SVG allows that notation?

Comment 4

[Jonathan Watt [:jwatt]]

Follow the links from http://www.w3.org/TR/SVG/painting.html#StrokeWidthProperty
biesi

Comment 5

[dave blasby]

Okay - This was my mistake, thanks for the feedback.  I'm marking this as invalid.

NOTE: scientific notation does work in the adobe plugin, so its likely others
will  stumble across this as well (especially for auto-generated GIS, CAD, or
scientific datasets).

Comment 6

[Jaime Mitchell (use [e-mail] for email)]

I think Comment 4 suggests this is valid. Jonathan, is that the case? should
this be reopened?

Comment 7

[Jonathan Watt [:jwatt]]

Yes, you were right the first time Dave, this is valid. :-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

So wait.  Is the idea that this is not a property, but is an attribute, so the "For SVG's XML attributes" part applies?  What am I missing?

Whatever we do with this bug, SVG needs some major clarification here if I can't tell what the right behavior is....

Comment 9

[Jonathan Watt [:jwatt]]

My reading is that exponents are allowed in the number/length values of properties when the properties are specified using presentational attributes (for flexibility), but not when specified using a styling language such as CSS (for compatibility (with CSS)).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Except it's clearly stated that scientific notation is not allowed in properties...

Note that SVG 1.2 tiny is not consistent with this.  Perhaps we should send some mail to the WG on this issue?

Comment 11

[Robert Longson [:longsonr]]

Sent question to WG:

http://lists.w3.org/Archives/Public/www-svg/2007Jan/0053.html

Comment 12

[Robert Longson [:longsonr]]

Attachment: parse scientific notation in SVG XML properties

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm not going to be able to get to this review in the foreseeable future (weeks, at least).  Please try David?

Note that there's a patch in the works somewhere to pretty completely rewrite this code, to make it faster, btw.  You might want to coordinate with that patch author..

Comment 14

[Robert Longson [:longsonr]]

Comment on attachment 253181 [details] [diff] [review]
parse scientific notation in SVG XML properties

(In reply to comment #13)

Changed to dbaron as suggested.

I don't know which bug is the rewrite. A search on CSS Parse was unrevealing.

Comment 15

[Boris Zbarsky [:bzbarsky]]

A search on bugs in "Style System" with the "perf" keyword doesn't give that many results.  Bug 311566 is the one you want.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 253181 [details] [diff] [review]
parse scientific notation in SVG XML properties

Could you remove the mSVGMode member from nsCSSParser, add GetSVGMode to both nsCSSParser and nsCSSScanner (both inline), and make SetSVGMode on nsCSSScanner be inline as well?

Where does the SVG spec say that exponential notation is allowed?  In particular, on what values is it allowed?  I'm surprised to see that you're monkeying with the code to set mIntegerValid, but whether you should do that depends on what the spec says.  I also can't really review the parsing code without knowing what it's trying to parse.

Is there a grammatical production for what the spec says should be accepted for exponential notation?  If not, what do other implementations accept and not accept?  You should add tests for both acceptance and rejection cases.

Sorry I took so long to get to this.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, I found the part of the spec in http://www.w3.org/TR/SVG/types.html#BasicDataTypes .  Since <integer> is unchanged from CSS, it's pretty clear you shouldn't be changing the mIntegerValid code.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch also seems like it is not be pushing back sufficient characters in the failure cases (such as "3.0em" -- the e needs to be the next character to read after reading the number "3.0").

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Although the more interesting failure-to-Pushback cases are cases where you'd incorrectly accept something that should be an error, such as "font-size:3e+".

Comment 20

[Robert Longson [:longsonr]]

Attachment: address review comments

Note: I'm still checking for gotE in the integerValid code to avoid treating 1e-3 as an integer.

Comment 21

[Robert Longson [:longsonr]]

Attachment: updated patch

previous patch would have accepted -e3 which is not much of a number. Should insist on getting a digit before the e.

Comment 22

[Robert Longson [:longsonr]]

Attachment: hopefully final update

digit check not required after all as caller ensures that a digit is present before and e.

Apologies for all the spam due to the multiple attempts.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 266349 [details] [diff] [review]
hopefully final update

>   // Gather up characters that make up the number
>   PRUint8* lexTable = gLexTable;
>+  PRBool gotE = PR_FALSE;
>   for (;;) {
>     c = Read(aErrorCode);
>     if (c < 0) break;
>-    if (!gotDot && (c == '.') &&
>+    if (!gotDot && !gotE && (c == '.') &&
>         CheckLexTable(Peek(aErrorCode), IS_DIGIT, lexTable)) {
>       gotDot = PR_TRUE;
>+#ifdef MOZ_SVG
>+    } else if (!gotE && (c == 'e' || c == 'E')) {
>+      if (!IsSVGMode()) break;

Please put the break on its own line, with braces around it.

>+      PRInt32 nextChar = Peek(aErrorCode);
>+      PRInt32 sign = 0;
>+      if (nextChar == '-' || nextChar == '+') {
>+        sign = Read(aErrorCode);
>+        nextChar = Peek(aErrorCode);
>+      }
>+      if (CheckLexTable(nextChar, IS_DIGIT, lexTable)) {
>+        gotE = PR_TRUE;
>+        if (sign) {
>+          ident.Append(PRUnichar(c));
>+          c = sign;
>+        }
>+      } else {
>+        if (sign) {
>+          Unread();
>+        }
>+        break;
>+      }
>+#endif
>     } else if ((c > 255) || ((lexTable[c] & IS_DIGIT) == 0)) {
>       break;
>     }
>     ident.Append(PRUnichar(c));
>   }


A bit later, there's code:
      // Put back character that stopped numeric scan
      Unread();
and Unread() can't be called twice.  You'll need to convert things to Pushback().

> 
>+#ifdef MOZ_SVG
>+      if (!gotDot && !gotE) {
>+#else
>       if (!gotDot) {
>+#endif

No need for the ifdefs -- gotE is defined (and always false) when MOZ_SVG is not defined.  Just make the change.

(This pattern occurs twice.)

>Index: nsCSSScanner.h
>+    NS_ASSERTION(aSVGMode == PR_TRUE || aSVGMode == PR_FALSE, "bad PRBool value");

Please wrap at <80 characters.


This is pretty complicated and hard-to-review code.  Do you have testcases (for both the new feature and other code you're touching)?  Could you add them as a mochitest, as part of the patch?

Comment 24

[Robert Longson [:longsonr]]

Attachment: address review comments

Addressed review comments.

I couldn't figure out how to get mochitests to work so I did some reftests instead.

Comment 25

[Boris Zbarsky [:bzbarsky]]

What part of the test documentation was unclear?  We should fix it, if it's bad enough that people can't figure it out.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 290222 [details] [diff] [review]
address review comments

r+sr=dbaron.

Sorry about taking so long to get to this, again.  In the future, feel free to bug me if I miss something for this long.

Comment 27

[Robert Longson [:longsonr]]

Attachment: update to tip

Updated to tip and reftests converted to mochitests with extra ex and em tests

Comment 28

[Robert Longson [:longsonr]]

Checked in 6276d0c9a49d.

Comment 29

[Robert Longson [:longsonr]]

I had to remove the ex tests as they didn't work on Mac. The em test is still in though so there is still a test for a unit beginning with an e.