Bug 45017 - CSS MediaQuery tests fail
Summary: CSS MediaQuery tests fail
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://www.w3.org/Style/CSS/Test/Medi...
Keywords:
: 77958 (view as bug list)
Depends on: 96752 97006
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-31 21:28 PDT by Simon Fraser (smfr)
Modified: 2013-03-21 04:01 PDT (History)
9 users (show)

See Also:


Attachments
Reduced testcase (688 bytes, text/html)
2010-09-03 08:57 PDT, Simon Fraser (smfr)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-08-31 21:28:05 PDT
180 of the 190 tests in the url fail. Firefox passes them all.
Comment 1 Simon Fraser (smfr) 2010-09-03 08:57:02 PDT
Created attachment 66503 [details]
Reduced testcase

It seems that sheet.cssRules does not include the @media at-rules in WebKit.
Comment 2 Simon Fraser (smfr) 2010-09-03 11:15:57 PDT
There are several issues here.

First, we don't keep around empty CSSRules, so an empty @media rule doesn't show in sheet.cssRules. This patch fixes that:


diff --git a/WebCore/css/CSSMediaRule.cpp b/WebCore/css/CSSMediaRule.cpp
index d1c220b..9f8bb4a 100644
--- a/WebCore/css/CSSMediaRule.cpp
+++ b/WebCore/css/CSSMediaRule.cpp
@@ -40,9 +40,11 @@ CSSMediaRule::~CSSMediaRule()
     if (m_lstMedia)
         m_lstMedia->setParent(0);
 
-    int length = m_lstCSSRules->length();
-    for (int i = 0; i < length; i++)
-        m_lstCSSRules->item(i)->setParent(0);
+    if (m_lstCSSRules) {
+        int length = m_lstCSSRules->length();
+        for (int i = 0; i < length; i++)
+            m_lstCSSRules->item(i)->setParent(0);
+    }
 }
 
 unsigned CSSMediaRule::append(CSSRule* rule)
diff --git a/WebCore/css/CSSParser.cpp b/WebCore/css/CSSParser.cpp
index e20537a..4b5f620 100644
--- a/WebCore/css/CSSParser.cpp
+++ b/WebCore/css/CSSParser.cpp
@@ -5344,7 +5344,7 @@ CSSRule* CSSParser::createImportRule(const CSSParserString& url, MediaList* medi
 
 CSSRule* CSSParser::createMediaRule(MediaList* media, CSSRuleList* rules)
 {
-    if (!media || !rules || !m_styleSheet)
+    if (!media || !m_styleSheet)
         return 0;
     m_allowImportRules = m_allowNamespaceDeclarations = m_allowVariablesRules = false;
     RefPtr<CSSMediaRule> rule = CSSMediaRule::create(m_styleSheet, media, rules);


Secondly, the error handling described here:
http://dev.w3.org/csswg/css3-mediaqueries/#error-handling
is not implemented.

Even with that, I don't understand the test. It seems to test that:

@media screen, not(orientation)

(which does not parse)
should result in @media screen, but I don't see how that is correct behavior, from the spec.
Comment 3 Alexey Proskuryakov 2010-09-03 11:21:36 PDT
> First, we don't keep around empty CSSRules, so an empty @media rule doesn't show in sheet.cssRules.

I think that's bug 15649.
Comment 4 Simon Fraser (smfr) 2010-09-03 11:22:58 PDT
Bug 15649 is about empty sheets, not empty @ rules.
Comment 5 Daniel Glazman 2010-09-03 11:29:06 PDT
> @media screen, not(orientation)
>
> (which does not parse)
> should result in @media screen, but I don't see how that is correct behavior, > from the spec.

The prose says the error handling cares about media queries. Here you
have a group of two media queries... So not(orientation) is invalid and
thrown away and screen is valid and preserved.
This was also discussed recently inside the CSS WG.
Comment 6 Anne van Kesteren 2010-09-04 01:58:19 PDT
It is turned into "not all" actually. See the bit about "Malformed media query" under error handling.
Comment 7 Simon Fraser (smfr) 2012-02-07 03:32:58 PST
*** Bug 77958 has been marked as a duplicate of this bug. ***
Comment 8 Rune Lillesveen 2013-03-21 03:42:14 PDT
I'm working on a patch that's for Media Queries error handling that's nearly finished. The Media Queries tests suite is using the media attribute which is currently split at commas, supporting HTML4 error handling, before fed into the CSS parser, so that needs to be fixed too.