Last Comment Bug 454226 - Media Queries: "all," should work
: Media Queries: "all," should work
Status: VERIFIED FIXED
: verified1.9.1
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9.1b2
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
http://tc.labs.opera.com/mediaqueries...
Depends on: 461266
Blocks: mediaqueries
  Show dependency treegraph
 
Reported: 2008-09-08 08:25 PDT by Anne (:annevk)
Modified: 2010-02-07 09:41 PST (History)
15 users (show)
roc: blocking1.9.1+
mozaakash: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (11.09 KB, patch)
2008-09-19 09:54 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Anne (:annevk) 2008-09-08 08:25:59 PDT
Even though the media query list "all," is invalid, it should still apply. The second (empty) media query should be simply ignored.

Tested with: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080908054322 Minefield/3.1b1pre
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-09 15:37:56 PDT
It should work for the same reason that "all, [foobar]" should work.  It's probably worth having tests for things like that too.  In particular, all of these should work:

,all
all,
all,foobar
foobar,all
(foobar),all
all,(foobar)
[foobar],all
all,[foobar]

Now I need to see which of these actually work...
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-10 13:41:28 PDT
I think the spec could be a little clearer here, though.  It currently says:

  If one media query in a comma-separated list is rejected, the other
  media queries in the list are evaluated as if the rejected media query
  had not been there.

But it doesn't really say what "rejected" means in this context, and it seems reasonable to assume that it's referring only to the possibilities mentioned just before (unknown media types, unknown media features).  If it's actually referring to all syntax errors (which I think is what you're suggesting in this bug), then it probably ought to say so.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-10 13:46:32 PDT
And I seem to remember explicit discussion at some point saying that errors within one query in a list invalidate the whole list, which is what I implemented.

I'm going to wait for feedback before spending the time to fix this; it's not trivial.
Comment 4 Anne (:annevk) 2008-09-11 23:23:51 PDT
I thought we reversed that decision and decided that any kind of error would make the selector ignored. (Though still taking the braces and quotes into account. E.g., '(,(width:0),all,)' is false.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-19 08:34:12 PDT
Ah, I see the spec now says:

  User agents are to handle unexpected tokens encountered while parsing a media
  query by reading until the end of the media query, while observing the rules
  for matching pairs of (), [], {}, "", and '', and correctly handling escapes.
  Media queries with unexpected tokens are ignored. [CSS21]
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-09-19 09:54:52 PDT
Created attachment 339475 [details] [diff] [review]
patch

Here's a patch that does what you describe.

It breaks Acid3, test 46, case t.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-10-08 09:14:48 PDT
Comment on attachment 339475 [details] [diff] [review]
patch

The WG discussed this today; we'll ask Hixie to change Acid3.
Comment 8 Boris Zbarsky [:bz] 2008-10-08 09:46:15 PDT
Comment on attachment 339475 [details] [diff] [review]
patch

>-  // Callers must clear or throw out aMedia if GatherMedia returns false.

Why is that no longer the case?

Other than that, looks good.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-10-08 10:17:50 PDT
GatherMedia no longer propagates a "there was a parse error" state, but instead handles it internally; it returns false only for out-of-memory or other low-level scanner error.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-10-11 17:52:35 PDT
Fixed: http://hg.mozilla.org/mozilla-central/rev/53740a23fe93
Comment 11 Tony Chung [:tchung] 2008-10-31 17:21:12 PDT
cc'ing adam, as he is writing testcases for media query work
Comment 12 Aakash Desai [:aakashd] 2009-04-17 11:15:19 PDT
verified FIXED on builds:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090416 Minefield/3.6a1pre ID:20090416030845

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090417 Shiretoko/3.5b4pre ID:20090417030722

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