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.

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?

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

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).

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

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

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....

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)).

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?

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

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 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.

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 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.

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.

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").

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+".

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

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

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 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?

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

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 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.

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

Checked in 6276d0c9a49d.

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.