Bug 8132 – CSS parser sometimes accepts real number as integer

• Component: CSS
• Status: VERIFIED FIXED

[Gérard Talbot (no longer involved)]

rgb(int, int, float) is accepted by the CSS parser when it should trigger an error and subsequent error condition.

References:

"The format of an RGB value in the functional notation is 'rgb(' followed by a comma-separated list of three numerical values (either three *_integer_* values or three percentage values) followed by ')'." 
CSS 2.1, Section 4.3.6 Colors
http://www.w3.org/TR/CSS21/syndata.html#value-def-color

"An <integer> consists of one or more digits "0" to "9". A <number> can either be an <integer>, or it can be zero or more digits followed by a dot (.) followed by one or more digits." 
CSS 2.1, Section 4.3.1 Integers and real numbers
http://www.w3.org/TR/CSS21/syndata.html#q15

"illegal values, or values with illegal parts, are treated as if the declaration weren't there at all" 
CSS1 Forward compatibility
http://www.w3.org/TR/CSS1#forward-compatible-parsing


So, an integer, according to CSS 2.1, clearly excludes the presence of a dot (.), therefore 
selector {color: rgb(0, 0, 128.0);}
should trigger a CSS parsing error and the whole declaration should be dropped, ignored. This is what the testcase at provided URL tests.

Safari 2.02 (41613) fails the second line of the testcase at provided URL: the 2nd line is blue and not green.

Another good testpage is:
http://www.hixie.ch/tests/evil/css/css21/tests/t040306-syntax-01-f.htm

I searched for a duplicate and couldn't find one.

[Alexey Proskuryakov]

FWIW, WinIE 6 and Safari give identical results for the test from the bug URL - but Firefox passes.

[Rob Buis]

Created attachment 8128 [details]
Proposed patch

This patch solves the problem, but I do not know whether it has the right approach.
Also the new flag true_float should probably part of a bit string. Because of this
I do not request a review yet, but would like some feedback :)
Cheers,

Rob.

[Rob Buis]

Created attachment 8141 [details]
Updated patch

Updated version after feedback from othermaciej.
No extra shift/reduce conflicts are added because of this patch.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 8141 [details]
Updated patch

 num             [0-9]+|[0-9]*"."[0-9]+
+intnum          [0-9]+|[0-9]*

This looks wrong to me. [0-9]+ would be correct, the [0-9]* part is not relevant nor do we need a "|".

I'm also unclear on how flex handles the ambiguity since both {num} and {intnum} can match the same thing. We might need to rearrange the regular expressions so that is not the case.

I suggest using INTEGER instead of INTNUMBER and perhaps changing the name of NUMBER to something like FLOAT.

We need a test case. We don't check in bug fixes without test cases.

[Rob Buis]

Created attachment 8151 [details]
Updated patch after darins feedback

Indeed the regexp was badly copy-pasted, corrected now. I think it also
increases the number of conflicts, but the tests show no difference. Finally, I took
the original testcase and places it in css2.1/,  not sure if that is the best place, but the
testcase itself is pretty clear IMHO.
Cheers,

Rob.

[Darin Adler]

(In reply to comment #5)
> I think it also increases the number of conflicts, but the tests show no difference.

I believe a change to the lexer could change the number of conflicts. Conflicts are something in the Bison-generated parser, not the Flex-generated lexer.

> I took the original testcase and placed it in css2.1

The test case is not included in the patch.

[Darin Adler]

(In reply to comment #6)
> The test case is not included in the patch.

Oops, my bad. It is.

[Darin Adler]

Comment on attachment 8151 [details]
Updated patch after darins feedback

+  | IMPORT_SYM maybe_space string_or_uri maybe_space maybe_media_list {
+        $$ = static_cast<CSSParser *>(parser)->createImportRule($3, $5);
+    }

What does this change have to do with the bug you're fixing? Looks like something else you were working on?

I think it's quite ugly that isInt is there all the time, but only used for the CSS_NUMBER case. Might need at least a comment to make that clear.

review- just because of the IMPORT_SYM line; no need for more review, but we need a patch without that change

[Rob Buis]

Created attachment 8162 [details]
Updated patch, removing unrelated code

Indeed I had bits from another patch in there. Just shows that creating
patches while watching a movie is a bad idea :)
Cheers,

Rob.

[Rob Buis]

Comment on attachment 8162 [details]
Updated patch, removing unrelated code

Hmm, no idea why I didnt ask
for review before :}

[Alexey Proskuryakov]

Landed the test into fast/css, rather than css2.1 (the latter contains W3C tests).

[Timothy Hatcher]

Landed in r14352