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
: WebKit
CSS
: 528+ (Nightly build)
: PC Windows XP
: P2 Normal
Assigned To:
: http://lists.w3.org/Archives/Public/w...
:
:
:
  Show dependency treegraph
 
Reported: 2008-07-09 11:52 PST by
Modified: 2008-07-17 22:49 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-07-09 11:52:49 PST
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 From 2008-07-09 11:54:41 PST -------
Created an attachment (id=22189) [details]
Test case that demonstrates issue with @mediaall 
------- Comment #2 From 2008-07-09 11:57:35 PST -------
Created an attachment (id=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 From 2008-07-10 10:02:03 PST -------
(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 From 2008-07-11 01:46:06 PST -------
Created an attachment (id=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 From 2008-07-11 13:07:09 PST -------
(From update of attachment 22245 [details])
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 From 2008-07-11 13:37:13 PST -------
(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 From 2008-07-11 14:23:36 PST -------
Created an attachment (id=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 From 2008-07-11 15:43:03 PST -------
Created an attachment (id=22252) [details]
Fixed ChangeLog (with proper email)
------- Comment #9 From 2008-07-11 16:02:14 PST -------
Created an attachment (id=22253) [details]
Fixed tabbing and added expected.txt 
------- Comment #10 From 2008-07-16 01:37:52 PST -------
(From update of attachment 22253 [details])
'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 From 2008-07-16 01:44:33 PST -------
(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 From 2008-07-16 08:55:48 PST -------
Created an attachment (id=22307) [details]
Final patch - fixed changelog

No regression issues with Qt and Gtk (don't have access to MacOSX yet).
------- Comment #13 From 2008-07-16 08:56:55 PST -------
(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 From 2008-07-16 11:49:14 PST -------
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 From 2008-07-16 11:54:33 PST -------
(From update of attachment 22307 [details])
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 From 2008-07-16 14:19:18 PST -------
Created an attachment (id=22316) [details]
Final patch

Added required newline in ChangeLog
------- Comment #17 From 2008-07-16 14:19:47 PST -------
(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 From 2008-07-17 12:47:23 PST -------
(From update of attachment 22316 [details])
once again, r=me :)
------- Comment #19 From 2008-07-17 12:51:03 PST -------
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 From 2008-07-17 22:49:52 PST -------
Landed in r35225.