The default bug view has changed. See this FAQ.

update to current rule for handling malformed media queries

RESOLVED FIXED in mozilla10

Status

()

Core
CSS Parsing and Computation
P4
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({css3})

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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
Attachment #562549 - Flags: review?(bzbarsky)
Comment on attachment 562549 [details] [diff] [review]
patch

r=me
Attachment #562549 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 2

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1c0e678b61
Priority: -- → P4
Target Milestone: --- → mozilla10
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
(Assignee)

Comment 4

6 years ago
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);
(Assignee)

Comment 5

6 years ago
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.)
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+
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed6db7aab4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c9a00ca9d5c
https://hg.mozilla.org/mozilla-central/rev/7ed6db7aab4f
https://hg.mozilla.org/mozilla-central/rev/6c9a00ca9d5c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.