Last Comment Bug 757554 - media query parsing should reject all uses of "not", "only", "and", and "or" as media_type
: media query parsing should reject all uses of "not", "only", "and", and "or" ...
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
http://dev.w3.org/csswg/css4-mediaque...
Depends on:
Blocks: mediaqueries
  Show dependency treegraph
 
Reported: 2012-05-22 11:50 PDT by Kang-Hao (Kenny) Lu [:kennyluck]
Modified: 2014-09-12 05:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (1.11 KB, text/html)
2012-05-22 11:50 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
no flags Details
Option A - this patch adds some tests for the current behavior (1.02 KB, patch)
2012-05-22 13:04 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
dbaron: review-
Details | Diff | Review
Option B - to implement the current spec (my reading) (3.36 KB, patch)
2012-05-22 13:08 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
no flags Details | Diff | Review
Reject uses of reserved 'not', 'only', 'and', and 'or' as a media type. (5.72 KB, patch)
2013-06-15 11:44 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
cam: review+
Details | Diff | Review

Description Kang-Hao (Kenny) Lu [:kennyluck] 2012-05-22 11:50:27 PDT
Created attachment 626116 [details]
test case

The rule-based syntax in the spec

  # media_query
  #  : [ONLY | NOT]? S* media_type S* [ AND S* expression ]*

has the interesting (boring?) consequence that media_type allows "only" and "not", which means that, according to the formal grammar, a small amount of backtracking (not sure if I am using the right word here) is required. I see three options to address this spec-implementation mismatch.

A. Ask spec to change to match what Firefox implements.

I'm pretty sure that other browsers do what Firefox does here (but I'll attach testing results in a minute). Also, if this section is rewritten in a state-machine-based approach, it is likely to be what's implemented.

B. Change our implementation to accept "only" and "not" as media type.

C. Change the spec and the implementation to disallow "and" as media type.

I am opposed to this as it requires redundant check to make sure the media type isn't "and".


The effect of this is only observable via CSSOM.
Comment 1 Kang-Hao (Kenny) Lu [:kennyluck] 2012-05-22 12:31:07 PDT
(In reply to Kang-Hao (Kenny) Lu [:kennyluck] from comment #0)
> A. Ask spec to change to match what Firefox implements.

I forgot to mention that what Firefox does currently is that it disallows "and" and "only" as media type when it is *not* preceded by "and" or "not". 
 
> I'm pretty sure that other browsers do what Firefox does here (but I'll
> attach testing results in a minute). 

So IE9 implements what Firefox does. Opera12alpha and Chromium nightly implement C. (The test case doesn't work for Chromium because it doesn't seem to parse <style> with @media that doesn't match :( and also it doesn't implement the "Malformed media query" => "drop until ','" rule :(. A dangling "and" is dropped along with the whole at-rule.)
Comment 2 Kang-Hao (Kenny) Lu [:kennyluck] 2012-05-22 13:04:20 PDT
Created attachment 626144 [details] [diff] [review]
Option A - this patch adds some tests for the current behavior
Comment 3 Kang-Hao (Kenny) Lu [:kennyluck] 2012-05-22 13:08:48 PDT
Created attachment 626147 [details] [diff] [review]
Option B - to implement the current spec (my reading)

This is admittedly quite crappy.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-03 14:52:33 PDT
Did you raise this on www-style?  I think that's worth doing.
Comment 5 Kang-Hao (Kenny) Lu [:kennyluck] 2012-08-03 21:14:41 PDT
Yes. Florian's opinion[1] seems to be B. (for which I have a patch too), but using author's point of view for these kinds of corner situations doesn't make sense to me...

I am not eager to pursue any way so I'll let you decide what to do here. We might as well just postpone this until someone bothers to submit a testcase. 

[1] http://lists.w3.org/Archives/Public/www-style/2012May/0955
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-05-27 02:48:24 PDT
I think my preference is actually option D:

http://lists.w3.org/Archives/Public/www-style/2013May/0658.html


But I think this is relatively low priority, and we should wait for the spec to settle before moving.

(Sorry to take so long to get to this, again.)
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-05-30 00:51:43 PDT
The CSS WG resolved in yesterday's teleconference:
http://krijnhoetmer.nl/irc-logs/css/20130529#l-326 (minutes soon, hopefully)
to accept my proposal (D) in http://lists.w3.org/Archives/Public/www-style/2013May/0658.html
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-05-30 00:53:54 PDT
Comment on attachment 626144 [details] [diff] [review]
Option A - this patch adds some tests for the current behavior

That means these two tests:

>+  query_should_be_parseable("and");
>+  query_should_be_parseable("not not");

and this test:

>+  query_should_be_parseable("and and (orientation)");

are no longer correct, and code changes are required.  (Should be 'not'.)

Are you interested in fixing the code here, or should I?
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-03 00:13:02 PDT
Minutes of decision at http://lists.w3.org/Archives/Public/www-style/2013May/0783.html
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-15 11:44:36 PDT
Created attachment 763128 [details] [diff] [review]
Reject uses of reserved 'not', 'only', 'and', and 'or' as a media type.
Comment 11 Cameron McCormack (:heycam) 2013-06-16 21:28:00 PDT
Comment on attachment 763128 [details] [diff] [review]
Reject uses of reserved 'not', 'only', 'and', and 'or' as a media type.

Review of attachment 763128 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-17 14:26:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/68eed79362f7
Comment 13 Ed Morley [:emorley] 2013-06-18 04:05:59 PDT
https://hg.mozilla.org/mozilla-central/rev/68eed79362f7
Comment 14 Jean-Yves Perrier [:teoli] 2014-09-12 05:26:35 PDT
Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/24

I don't think it is worth a mention in the main text.

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