Closed Bug 689319 Opened 8 years ago Closed 8 years ago

update to current rule for handling malformed media queries

Categories

(Core :: CSS Parsing and Computation, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: dbaron, Assigned: dbaron)

Details

(Keywords: css3)

Attachments

(2 files)

Attached patch patchSplinter Review
There was a spec update to media queries that I managed to miss.  The rule for handling "Malformed media query" in section 3.1 (Error Handling) now describes an error handling behavior that invalidates the query rather than the query list.

I caught this because Arron pointed out the test was wrong in:
http://lists.w3.org/Archives/Public/public-css-testsuite/2011Sep/0042.html
Attachment #562549 - Flags: review?(bzbarsky)
Comment on attachment 562549 [details] [diff] [review]
patch

r=me
Attachment #562549 - Flags: review?(bzbarsky) → review+
So the reason for the failures is that the patch is changing the behavior of items 15, 16, 17, and 20 in:
http://hg.mozilla.org/mozilla-central/file/d2241309d83c/browser/base/content/test/page_style_sample.html
with respect to the code here:
http://hg.mozilla.org/mozilla-central/file/d2241309d83c/browser/base/content/browser.js#l5835

Both before and after the patch, currentStyleSheet.media.mediaText is "not all".  The problem is that the patch changes currentStyleSheet.media.length from 0 to 1, which means we're now entering this nice broken code that assumes media queries don't exist:
  5843       let media = currentStyleSheet.media.mediaText.split(", ");
  5844       if (media.indexOf("screen") == -1 &&
  5845           media.indexOf("all") == -1)
  5846         continue;
and determining that the style sheet doesn't apply.


This can be tested by loading page_style_sample.html in scratchpad and running:

var links = document.getElementsByTagName("link");
var mylink = links[17];
alert(mylink.getAttribute("title") + ", " + mylink.sheet.media.length + ", " + mylink.sheet.media.mediaText);
This improves on bug 498312 by using window.matchMedia (which didn't exist then).  (It changes the test to make the failures that the other patch on this bug caused be the new correct behavior -- and then adds some additional tests.)
Attachment #567455 - Flags: review?(dao)
Comment on attachment 567455 [details] [diff] [review]
make page style menu use window.matchMedia

>+    // [...]  Note
>+    // that this assumes that the media queries are static, which isn't
>+    // quite true.  (For example, resizing the window can change which
>+    // style sheets apply.)

Does this actually matter? Manually resizing the window would close the page style menu, reopening it would call stylesheetFillPopup again.

>     if (currentStyleSheet.media.length > 0) {
>-      let media = currentStyleSheet.media.mediaText.split(", ");
>-      if (media.indexOf("screen") == -1 &&
>-          media.indexOf("all") == -1)
>+      var mediaQueryList = currentStyleSheet.media.mediaText;

nit: let mediaQueryList ...
Attachment #567455 - Flags: review?(dao) → review+
(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 567455 [details] [diff] [review] [diff] [details] [review]
> make page style menu use window.matchMedia
> 
> >+    // [...]  Note
> >+    // that this assumes that the media queries are static, which isn't
> >+    // quite true.  (For example, resizing the window can change which
> >+    // style sheets apply.)
> 
> Does this actually matter? Manually resizing the window would close the page
> style menu, reopening it would call stylesheetFillPopup again.

Ah, in that case, it doesn't matter.
You need to log in before you can comment on or make changes to this bug.