Bug 19965 – @mediaall recognized as @media all rather than as a generic (unrecognized ATKEYWORD)

• Component: CSS
• Status: RESOLVED FIXED

[Jacob Refstrup]

A discussion on www-style clarified that webkits behaviour in recognizing @mediaall (without space) as equivalent to @media all was incorrect. In general this is true of all @-rules in webkit -- it needs to be a long greedy match for an identifier.

[Jacob Refstrup]

Created attachment 22189 [details]
Test case that demonstrates issue with @mediaall

[Jacob Refstrup]

Created attachment 22190 [details]
Patch that fixes @-rules from being recognized too early

Fix the bug by adding a rule to tokenizer.flex:

"@"{ident}

And have this rule return the '@' token -- this means that CSSGrammar.y does not have to change.

[Jacob Refstrup]

(In reply to comment #0)
> A discussion on www-style clarified that webkits behaviour in recognizing
> @mediaall (without space) as equivalent to @media all was incorrect. In general
> this is true of all @-rules in webkit -- it needs to be a long greedy match for
> an identifier.
> 

Working on a better patch (that also follows the patch guidelines :-)

[Jacob Refstrup]

Created attachment 22245 [details]
Updated patch to use "@"{ident}, updated ChangeLog and added test case 

Changed the fix to use "@"{ident} to return ATKEYWORD to be closer to the CSS2.1 spec. The '@' by itself (without a following identifier) should per CSS2.1 not be used in error recovery rules the same place that the ATKEYWORD is used. 

In practice this doesn't change error recovery as the invalid_block and invalid_block_list are used in all productions that need error recovery.

[Rob Buis]

Comment on attachment 22245 [details]
Updated patch to use "@"{ident}, updated ChangeLog and added test case 

You need to include layout test ChangeLog and results too. Maybe you can use dumpAsText to avoid the pixel test.

Are there no regressions?

Why the yellow background? Can't you just use white or not set the background?

[Jacob Refstrup]

(In reply to comment #5)
> (From update of attachment 22245 [details] [edit])
> You need to include layout test ChangeLog and results too. Maybe you can use
> dumpAsText to avoid the pixel test.
> 
> Are there no regressions?
> 

Not sure I follow -- atrule_longest_match.html render correctly before; now it will. I see that I am missing the expected text -- is that what you were referring to?

> Why the yellow background? Can't you just use white or not set the background?
> 

I suppose I can -- I just needed a check to ensure that the parser recovered after the incorrect @-rule.

I'll work on updating the patch.

Thanks for the input,
- Jacob

[Jacob Refstrup]

Created attachment 22251 [details]
Updated test case to use dumpAsText

Changed test style rules to use display: none/block/inline as a way of testing for correct behavior.

[Jacob Refstrup]

Created attachment 22252 [details]
Fixed ChangeLog (with proper email)

[Jacob Refstrup]

Created attachment 22253 [details]
Fixed tabbing and added expected.txt

[Rob Buis]

Comment on attachment 22253 [details]
Fixed tabbing and added expected.txt 

'ealier' is mentioned in the ChangeLog. Email address needs to be set.

You need to rerun the layout ChangeLog so it lists the .txt file too.

Does the patch cause no regressions on the tests?

If you can fix the minor niggles, put up the new patch and have assured there are no regressions with the patch, then I'll okay it.

[Rob Buis]

(In reply to comment #6)
> > Are there no regressions?
> > 
> Not sure I follow -- atrule_longest_match.html render correctly before; now it
> will. I see that I am missing the expected text -- is that what you were
> referring to?

Just to be clear, all patches need to go through the run-webkit-tests run
to check for regressions. See 'Regression tests' here:

https://www.webkit.org/coding/contributing.html

What basically is required is that you run the whole suite without your patch,
note the numer of pass/fails, then apply and build with your patch, rerun the suite and note whether anything changed with the pass/fail with the patch. Only when the fail count is the same can the patch be accepted.
Cheers,

Rob.

[Jacob Refstrup]

Created attachment 22307 [details]
Final patch - fixed changelog

No regression issues with Qt and Gtk (don't have access to MacOSX yet).

[Jacob Refstrup]

(In reply to comment #11)
> (In reply to comment #6)
> > > Are there no regressions?
> > > 
> > Not sure I follow -- atrule_longest_match.html render correctly before; now it
> > will. I see that I am missing the expected text -- is that what you were
> > referring to?
> 
> Just to be clear, all patches need to go through the run-webkit-tests run
> to check for regressions. See 'Regression tests' here:
> 
> https://www.webkit.org/coding/contributing.html
> 
> What basically is required is that you run the whole suite without your patch,
> note the numer of pass/fails, then apply and build with your patch, rerun the
> suite and note whether anything changed with the pass/fail with the patch. Only
> when the fail count is the same can the patch be accepted.
> Cheers,
> 
> Rob.
> 

Thanks for the followup Rob -- it does pass in the sense no new or different errors were reported in regression against Qt and Gtk.

- Jacob

[Rob Buis]

Hi,

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #6)
> > > > Are there no regressions?
> > > > 
> > > Not sure I follow -- atrule_longest_match.html render correctly before; now it
> > > will. I see that I am missing the expected text -- is that what you were
> > > referring to?
> > 
> > Just to be clear, all patches need to go through the run-webkit-tests run
> > to check for regressions. See 'Regression tests' here:
> > 
> > https://www.webkit.org/coding/contributing.html
> > 
> > What basically is required is that you run the whole suite without your patch,
> > note the numer of pass/fails, then apply and build with your patch, rerun the
> > suite and note whether anything changed with the pass/fail with the patch. Only
> > when the fail count is the same can the patch be accepted.
> > Cheers,
> > 
> > Rob.
> > 
> 
> Thanks for the followup Rob -- it does pass in the sense no new or different
> errors were reported in regression against Qt and Gtk.
> 
> - Jacob

Nice! I tested on OS X tiger and could see no regressions either.
Cheers,

Rob.

[Rob Buis]

Comment on attachment 22307 [details]
Final patch - fixed changelog

Minor niggles, there should be a newline after the Reviewed line in the WebCore ChangeLog and also a newline between LayoutTests ChangeLog entries.

In general check the existing entries for the right formatting.

r=me

[Jacob Refstrup]

Created attachment 22316 [details]
Final patch

Added required newline in ChangeLog

[Jacob Refstrup]

(In reply to comment #15)
> (From update of attachment 22307 [details] [edit])
> Minor niggles, there should be a newline after the Reviewed line in the WebCore
> ChangeLog and also a newline between LayoutTests ChangeLog entries.
> 
> In general check the existing entries for the right formatting.
> 
> r=me
> 

Okay - thx. Fixed the newline.
- Jacob

[Rob Buis]

Comment on attachment 22316 [details]
Final patch

once again, r=me :)

[Rob Buis]

Hi,

(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 22307 [details] [edit] [edit])
> > Minor niggles, there should be a newline after the Reviewed line in the WebCore
> > ChangeLog and also a newline between LayoutTests ChangeLog entries.
> > 
> > In general check the existing entries for the right formatting.
> > 
> > r=me
> > 
> 
> Okay - thx. Fixed the newline.

Again, patch seems fine. I landed it for you, it landed
in r35225.
Cheers,

Rob.

[Rob Buis]

Landed in r35225.