Bug 19965 - @mediaall recognized as @media all rather than as a generic (unrecognized ATKEYWORD)
: @mediaall recognized as @media all rather than as a generic (unrecognized ATK...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To: Nobody
http://lists.w3.org/Archives/Public/w...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-09 11:52 PDT by Jacob Refstrup
Modified: 2008-07-17 22:49 PDT (History)
2 users (show)

See Also:


Attachments
Test case that demonstrates issue with @mediaall (317 bytes, text/html)
2008-07-09 11:54 PDT, Jacob Refstrup
no flags Details
Patch that fixes @-rules from being recognized too early (571 bytes, patch)
2008-07-09 11:57 PDT, Jacob Refstrup
no flags Details | Formatted Diff | Diff
Updated patch to use "@"{ident}, updated ChangeLog and added test case (3.00 KB, patch)
2008-07-11 01:46 PDT, Jacob Refstrup
rwlbuis: review-
Details | Formatted Diff | Diff
Updated test case to use dumpAsText (3.64 KB, patch)
2008-07-11 14:23 PDT, Jacob Refstrup
no flags Details | Formatted Diff | Diff
Fixed ChangeLog (with proper email) (317 bytes, patch)
2008-07-11 15:43 PDT, Jacob Refstrup
no flags Details | Formatted Diff | Diff
Fixed tabbing and added expected.txt (3.95 KB, patch)
2008-07-11 16:02 PDT, Jacob Refstrup
rwlbuis: review-
Details | Formatted Diff | Diff
Final patch - fixed changelog (4.02 KB, patch)
2008-07-16 08:55 PDT, Jacob Refstrup
rwlbuis: review+
Details | Formatted Diff | Diff
Final patch (4.02 KB, patch)
2008-07-16 14:19 PDT, Jacob Refstrup
rwlbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jacob Refstrup 2008-07-09 11:52:49 PDT
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.
Comment 1 Jacob Refstrup 2008-07-09 11:54:41 PDT
Created attachment 22189 [details]
Test case that demonstrates issue with @mediaall
Comment 2 Jacob Refstrup 2008-07-09 11:57:35 PDT
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.
Comment 3 Jacob Refstrup 2008-07-10 10:02:03 PDT
(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 :-)
Comment 4 Jacob Refstrup 2008-07-11 01:46:06 PDT
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.
Comment 5 Rob Buis 2008-07-11 13:07:09 PDT
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?
Comment 6 Jacob Refstrup 2008-07-11 13:37:13 PDT
(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
Comment 7 Jacob Refstrup 2008-07-11 14:23:36 PDT
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.
Comment 8 Jacob Refstrup 2008-07-11 15:43:03 PDT
Created attachment 22252 [details]
Fixed ChangeLog (with proper email)
Comment 9 Jacob Refstrup 2008-07-11 16:02:14 PDT
Created attachment 22253 [details]
Fixed tabbing and added expected.txt
Comment 10 Rob Buis 2008-07-16 01:37:52 PDT
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.
Comment 11 Rob Buis 2008-07-16 01:44:33 PDT
(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.
Comment 12 Jacob Refstrup 2008-07-16 08:55:48 PDT
Created attachment 22307 [details]
Final patch - fixed changelog

No regression issues with Qt and Gtk (don't have access to MacOSX yet).
Comment 13 Jacob Refstrup 2008-07-16 08:56:55 PDT
(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
Comment 14 Rob Buis 2008-07-16 11:49:14 PDT
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.
Comment 15 Rob Buis 2008-07-16 11:54:33 PDT
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
Comment 16 Jacob Refstrup 2008-07-16 14:19:18 PDT
Created attachment 22316 [details]
Final patch

Added required newline in ChangeLog
Comment 17 Jacob Refstrup 2008-07-16 14:19:47 PDT
(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

Comment 18 Rob Buis 2008-07-17 12:47:23 PDT
Comment on attachment 22316 [details]
Final patch

once again, r=me :)
Comment 19 Rob Buis 2008-07-17 12:51:03 PDT
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.
Comment 20 Rob Buis 2008-07-17 22:49:52 PDT
Landed in r35225.