In SVG the type <length> can have exponents: integer ([e|E] interger)? see http://www.w3.org/TR/SVG/types.html#DataTypeNumber We support this for SVG attributes, but not for SVG CSS properties like 'stroke-width', because we use the CSS parser and that parser doesn't support this. See attached link for an example.
Rob do you want to take a look at this as well? ;-)
Hi Dirk, (In reply to comment #1) > Rob do you want to take a look at this as well? ;-) Good find! This is very interesting to work on, you can assign to me if you are busy with the animation stuff, I can look into it this week. Cheers, Rob.
(In reply to comment #2) > Hi Dirk, > > (In reply to comment #1) > > Rob do you want to take a look at this as well? ;-) > > Good find! This is very interesting to work on, you can assign to me if you are busy with the animation stuff, I can look into it this week. Sure, I'm happy if you're working on it :-)
Have to extend this bug. It's not only <length> that can take exponents, all CSS properties with <number> can take exponents: <angle>, <length>, <frequency>, <icccolor>, <percentage>, <time>. Some of them are defined by CSS3 as well, but without of exponents. Means we have to differ <number> for CSS/HTML and for CSS/SVG during parsing numbers. CSS3: http://www.w3.org/TR/css3-values/#ltnumbergt SVG: http://www.w3.org/TR/SVG/types.html#DataTypeNumber
(In reply to comment #4) > CSS3: http://www.w3.org/TR/css3-values/#ltnumbergt > SVG: http://www.w3.org/TR/SVG/types.html#DataTypeNumber Ouch! Only if the value gets applied to the SVG _attribute_ it must allow exponent numbers, but it should _not_ on setting the property with CSS! <rect width="100" height="100" stroke-width="1e1px"> is allowed <rect width="100" height="100" style="stroke-width:1e1px;"> is not allowed.
Will upload a fix for that soon. Have it running in a local build. Just needs more testing.
Created attachment 134958 [details] Patch
Created attachment 134960 [details] Patch Forgot to add original author of the patch to the changelog :P.
Comment on attachment 134960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134960&action=review Nice job, still some caveats, r- for test coverage: > Source/WebCore/css/CSSParser.cpp:8603 > + int offset = 1; why int? I'd use unsigned. > Source/WebCore/css/CSSParser.cpp:8605 > + if (m_currentCharacter[offset] == '+') { Is this safe to access? Is it guaranteed that m_currentCharacter isn't accessed out of boundaries, aka. that 'e' is always followed by another character. > Source/WebCore/css/CSSParser.cpp:8606 > + offset++; ++offset. > Source/WebCore/css/CSSParser.cpp:8609 > + offset++; Ditto. > Source/WebCore/css/CSSParser.cpp:8612 > + double exponent; No need for this local here, just declare it in the if (offset > digits) branch directly, > Source/WebCore/css/CSSParser.cpp:8616 > + exponent = charactersToDouble(m_currentCharacter + digits, offset - digits); Can this yield "invalid" doubles, is this clamped somehow? Shall we sanitize the parsed exponent here? (thinking of NaN handling, or Inf) > LayoutTests/svg/css/script-tests/scientific-numbers.js:13 > +shouldBeEqualToString("document.defaultView.getComputedStyle(text, null).fontSize", "16px"); You can just use "getComputedStyle(text).fontSize". > LayoutTests/svg/css/script-tests/scientific-numbers.js:232 > + This needs to be extended; leading whitespace, trailing whitespace, mid white-space (" 5e10", " 5e1 0", etc. (also including negative values, for both number and exponent). Arbitary characters inside of exponent, exponents leading to double overflow, etc.
Created attachment 134986 [details] Patch New approach: Use SVG parser to parse numbers. The logic there is well tested and has a lot of more checks to make sure that we don't miss a case. Added more tests for some edge cases as well as overflow tests. isASCIIDigit() and equal checks prevent us from reading data outside the buffer.
(In reply to comment #10) > Created an attachment (id=134986) [details] > Patch > > New approach: Use SVG parser to parse numbers. The logic there is well tested and has a lot of more checks to make sure that we don't miss a case. Added more tests for some edge cases as well as overflow tests. Great, I'll look at thew new patch soon. > isASCIIDigit() and equal checks prevent us from reading data outside the buffer. This still worries me. I'll check if the new patch is safer.
Comment on attachment 134986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134986&action=review I have some new comments: > Source/WebCore/css/CSSParser.cpp:79 > +#if ENABLE(SVG) > +#include "SVGParserUtilities.h" > +#endif I don't think you need to guard this. > Source/WebCore/css/CSSParser.cpp:8606 > + if (isASCIIAlphaCaselessEqual(*m_currentCharacter, 'e')) { At this point it's guaranteed that m_currentCharacter exists okay. So dereferencing the UChar* pointer is fine. > Source/WebCore/css/CSSParser.cpp:8607 > + if (m_currentCharacter[1] == '-' || m_currentCharacter[1] == '+' || isASCIIDigit(m_currentCharacter[1])) { But who guarantees that the UChar* m_currentCharacter + 1 that you're dereferencing here exists? The old code passes m_tokenStart to charactersToDouble as const UChar* parameter, and m_currentCharacter - m_tokenStart as size. I'd strongly suggest to reuse that way of accessing the string. const UChar* currentCharacter = m_tokenStart; unsigned length = m_currentCharacter - m_tokenStart; if (length > 1 && isASCIIAlphaCaselessEqual(*currentCharacter, 'e')) { // Now its guaranteed that the passed in string is at least 'eX' (e + another item) } If I'm missing something, and this is already guaranteed by the CSSParser code, you should at least assert that the length is > 1. > Source/WebCore/css/CSSParser.cpp:8612 > + while (isASCIIDigit(m_currentCharacter[offset])) If the branch above isn't taken that means the first character following the 'e' is not '+', not '-' nor an ascii digit - why do you continue parsing at this point? You could have exited already. > Source/WebCore/css/CSSParser.cpp:8615 > + if (!parseSVGNumber(m_tokenStart, m_currentCharacter + offset - m_tokenStart, yylval->number)) This would then read as if (!parseSVGNumber(m_tokenStart, length + offset, yylval->number). > Source/WebCore/css/CSSParser.cpp:8616 > + break; If parseSVGNumber() fails, is yylval->number reset to a specific value? (Not sure about that, that's why I'm asking). > Source/WebCore/svg/SVGParserUtilities.cpp:152 > + One newline too much. > LayoutTests/svg/css/script-tests/scientific-numbers.js:13 > +shouldBeEqualToString("document.defaultView.getComputedStyle(text).fontSize", "16px"); You still use the long form here, getComputedStyle(text).fontSize should work, without the document.defaultView prefix :-)
(In reply to comment #12) > (From update of attachment 134986 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134986&action=review > > I have some new comments: > > > Source/WebCore/css/CSSParser.cpp:79 > > +#if ENABLE(SVG) > > +#include "SVGParserUtilities.h" > > +#endif > > I don't think you need to guard this. It is not necessary but prevent including the file on compiling. That was the intention. > > > Source/WebCore/css/CSSParser.cpp:8606 > > + if (isASCIIAlphaCaselessEqual(*m_currentCharacter, 'e')) { > > At this point it's guaranteed that m_currentCharacter exists okay. So dereferencing the UChar* pointer is fine. > > > Source/WebCore/css/CSSParser.cpp:8607 > > + if (m_currentCharacter[1] == '-' || m_currentCharacter[1] == '+' || isASCIIDigit(m_currentCharacter[1])) { > > But who guarantees that the UChar* m_currentCharacter + 1 that you're dereferencing here exists? That is the code before these lines: while (true) { if (!isASCIIDigit(m_currentCharacter[0])) { // Only one dot is allowed for a number, // and it must be followed by a digit. if (m_currentCharacter[0] != '.' || dotSeen || !isASCIIDigit(m_currentCharacter[1])) break; dotSeen = true; } ++m_currentCharacter; } So no extra check. The string gets terminated by a \n. Just checked it. Therefore is should be save to check the value for 1, since 0 was no /n. > > The old code passes m_tokenStart to charactersToDouble as const UChar* parameter, and m_currentCharacter - m_tokenStart as size. > I'd strongly suggest to reuse that way of accessing the string. > const UChar* currentCharacter = m_tokenStart; > unsigned length = m_currentCharacter - m_tokenStart; > if (length > 1 && isASCIIAlphaCaselessEqual(*currentCharacter, 'e')) { > // Now its guaranteed that the passed in string is at least 'eX' (e + another item) > } But just for the first char after the 'e'. Like I said, this is not necessary, since the last char is a \n on the string. > > If I'm missing something, and this is already guaranteed by the CSSParser code, you should at least assert that the length is > 1. Not necessary. > > > Source/WebCore/css/CSSParser.cpp:8612 > > + while (isASCIIDigit(m_currentCharacter[offset])) > > If the branch above isn't taken that means the first character following the 'e' is not '+', not '-' nor an ascii digit - why do you continue parsing at this point? You could have exited already. Well, not exiting. If I break I would break stuff. The unit is included in the token (10em wouldn't work, which gets tested by the new test as well). I could put the while loop in the else tree. > > > Source/WebCore/css/CSSParser.cpp:8615 > > + if (!parseSVGNumber(m_tokenStart, m_currentCharacter + offset - m_tokenStart, yylval->number)) > > This would then read as if (!parseSVGNumber(m_tokenStart, length + offset, yylval->number). > > > Source/WebCore/css/CSSParser.cpp:8616 > > + break; > > If parseSVGNumber() fails, is yylval->number reset to a specific value? (Not sure about that, that's why I'm asking). m_token get's not set and therefore the value is invalid. > > > Source/WebCore/svg/SVGParserUtilities.cpp:152 > > + > > One newline too much. Thanks. Will fix that. > > > LayoutTests/svg/css/script-tests/scientific-numbers.js:13 > > +shouldBeEqualToString("document.defaultView.getComputedStyle(text).fontSize", "16px"); > > You still use the long form here, getComputedStyle(text).fontSize should work, without the document.defaultView prefix :-) That was a misunderstanding. I thought you meant the ", null)" on computed style which I fixed. Will fix that.
Created attachment 135000 [details] Patch Patch. See previous comment.
Comment on attachment 135000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135000&action=review Next round of comments, poor Dirk :-) > Source/WebCore/css/CSSParser.cpp:8604 > + if (isASCIIAlphaCaselessEqual(*m_currentCharacter, 'e')) { Are we sure that at least one element follows the 'e' ? Can we ASSERT(m_currentCharacter - m_tokenStart > 1) here? Before accessing: if (m_currentCharacter[1].. below > Source/WebCore/css/CSSParser.cpp:8612 > + if (m_currentCharacter[1] == '-' || m_currentCharacter[1] == '+' || isASCIIDigit(m_currentCharacter[1])) { > + offset += 2; > + while (isASCIIDigit(m_currentCharacter[offset])) > + ++offset; > + // Use FLOATTOKEN if we have exponents. > + dotSeen = true; > + } > + } ++m_currentCharacter; if (*m_currentCharacter == '-' || *m_currentCharacter == '+' || isASCIIDigit(*m_currentCharacter)) { ++m_currentCharacter; while (isASCIIDigit(*m_currentCharacter)) ++m_currentCharacter; // Use FLOATTOKEN if the string contains exponents. dotSeen = true; } if (!parseNumber(m_tokenStart, m_currentCharacter, yylval->number, false)) break; You'll need a new parseNumber() variant taking a double&, but you can remove the new parseSVGNumber() method that you've added. > Source/WebCore/svg/SVGParserUtilities.h:36 > +bool parseSVGNumber(UChar*& ptr, size_t length, double& number); Can be avoided. > LayoutTests/svg/css/script-tests/scientific-numbers.js:274 > +text.setAttribute("font-size", "50e0.0"); > +shouldBeEqualToString("getComputedStyle(text).fontSize", "16px"); Hard to understand the test, without seeing what you're trying to assign. I propose following: function setFontSize(valueString) { text.setAttribute("font-size", valueString); } function test(valueString, expectedString) { debug("text.setAttribute('font-size', '" + valueString + "');"); setFontSize(valueString); shouldBeEqualToString("getComputedStyle(text).fontSize", expectedString); } and use: debug("Valid values"); test("50px", "50px"); test("100px", "100px"); setFontSize("16px"); debug(""); debug("Invalid values"); test("50e0.0", "16px"); test("50e 0", "16px"); .... setFontSize("16px"); debug(""); debug("Next category...");
(In reply to comment #15) Sorry. I am looking at this after a long time. > if (!parseNumber(m_tokenStart, m_currentCharacter, yylval->number, false)) > break; > IIRC the problem was that m_tokenStart and m_currentCharacter are of type UChar*, while parseNumber takes const UChar* (and const UChar*&). So I would need a type conversion which does not look pretty. > You'll need a new parseNumber() variant taking a double&, but you can remove the new parseSVGNumber() method that you've added. How could I avoid it? Even for parseNumber with a double I would need a new method. I choose parseSVGNumber to indicate from the name that it will be a number that is parsed according to the SVG rules. I still think that this is the proper solution. Especially here where it is not obvious that we call the method in SVGParserUtilities. > Hard to understand the test, without seeing what you're trying to assign. Thanks, I'll change the test and the other code suggestions.
Created attachment 151721 [details] Patch
Comment on attachment 151721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151721&action=review Looks reasonable, r=me. > Source/WebCore/ChangeLog:10 > + The SVG parser is already well tested and has some extra checks for edge cases that prevent > + us e.g for overflows. prevent from overflow, or rather protect from overflow
Committed r122978: <http://trac.webkit.org/changeset/122978>
Did you mean for this to be a pixel test? It looks like including SVGTestCase.js sets "window.enablePixelTesting = true" which causes pixel results to be dumped.
(In reply to comment #20) > Did you mean for this to be a pixel test? It looks like including SVGTestCase.js sets "window.enablePixelTesting = true" which causes pixel results to be dumped. No, should be dumpAsText. thanks