Last Comment Bug 689319 - update to current rule for handling malformed media queries
: update to current rule for handling malformed media queries
Status: RESOLVED FIXED
: css3
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P4 normal (vote)
: mozilla10
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-26 14:58 PDT by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2011-10-18 05:36 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (9.55 KB, patch)
2011-09-26 14:58 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
Details | Diff | Splinter Review
make page style menu use window.matchMedia (3.90 KB, patch)
2011-10-17 08:28 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
dao+bmo: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-09-26 14:58:06 PDT
Created attachment 562549 [details] [diff] [review]
patch

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
Comment 1 Boris Zbarsky [:bz] (TPAC) 2011-09-26 22:11:21 PDT
Comment on attachment 562549 [details] [diff] [review]
patch

r=me
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-09-30 12:28:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1c0e678b61
Comment 3 Matt Brubeck (:mbrubeck) 2011-09-30 14:14:40 PDT
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/c59e1d1d8615

Because of failures in browser_page_style_menu.js:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6629138&tree=Mozilla-Inbound
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-10-17 08:05:30 PDT
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);
Comment 5 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-10-17 08:28:38 PDT
Created attachment 567455 [details] [diff] [review]
make page style menu use window.matchMedia

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.)
Comment 6 Dão Gottwald [:dao] 2011-10-17 08:48:56 PDT
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 ...
Comment 7 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-10-17 09:08:40 PDT
(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.

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