The attached file shows red in WebKit, and green in Firefox. It has an invalid media query: @media screen and (max-device-width: 480px) screen and (device-width: 768px) { … } The regression range is around http://trac.webkit.org/log/trunk/?rev=121566&stop_rev=121513 but I can't figure out which change caused this.
Created attachment 207555 [details] Testcase
<rdar://problem/14564510>
regressed at http://trac.webkit.org/changeset/121551 patch is coming up.
Created attachment 208290 [details] Patch
How does this affect our results on http://www.w3.org/Style/CSS/Test/MediaQueries/20100726/media-queries-test.html ?
(In reply to comment #5) > How does this affect our results on http://www.w3.org/Style/CSS/Test/MediaQueries/20100726/media-queries-test.html ? Thanks for bringing this test suite to my attention. Apparently both this and the more up-to-date version (http://www.w3.org/Style/CSS/Test/MediaQueries/20120229/test_media_queries.html) rely on the presence of one media query, even when the query is syntactically incorrect. function query_is_parseable(q) { parse_test_style_text.data = "@media screen, " + q + " {}"; var sheet = parse_test_style_element.sheet; // XXX yikes, not live! if (sheet.cssRules.length == 1 && sheet.cssRules[0].type == CSSRule.MEDIA_RULE) return sheet.cssRules[0].media.mediaText != "screen, not all"; assert_unreached( "unexpected result testing whether query " + q + " is parseable"); } function query_should_not_be_parseable(q) { test(function() { assert_false(query_is_parseable(q)) }, "query " + q + " should not be parseable"); } Other than that, this patch has no impact on the test results (on both test suites) (failure numbers: 33 and 16) Changing the patch to support this assumption.
Created attachment 208324 [details] Patch
Comment on attachment 208324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208324&action=review r=me > Source/WebCore/css/CSSParser.cpp:11502 > + RuleList emptyRules; I think it would be bit nicer to just have RuleList() in call sites. > Source/WebCore/css/CSSParser.cpp:11510 > + } else if (rules) > + rule = StyleRuleMedia::create(media, *rules); > + else > + rule = StyleRuleMedia::create(media, emptyRules); Could combine these by using ternary operator rules ? *rules : RuleList()
I was made aware of this patch on #blink. The last case in the added layout test is not invalid and goes red in Presto, Gecko, and current Blink master.
(In reply to comment #9) > I was made aware of this patch on #blink. The last case in the added layout test is not invalid and goes red in Presto, Gecko, and current Blink master. Thanks, I'll check it out.
Also note that several patches landed in Blink to make the w3c media queries test suite fully pass. Those included changes to use the CSS parser to parse the media attribute as a media query list (as specified in HTML5), since media attributes are used in the test suite. I don't know which of them (if any) have made it into WebKit.
(In reply to comment #10) > (In reply to comment #9) > > I was made aware of this patch on #blink. The last case in the added layout test is not invalid and goes red in Presto, Gecko, and current Blink master. > Thanks, I'll check it out. Not invalid in the sense that: "@media screen and (min-width: 480px), and (device-width: 768px)" will end up as: "@media screen and (min-width: 480px), not all" And match when the window is wider than 480px.
tracking the error case here: bug 119573
Created attachment 208339 [details] Patch for landing
Comment on attachment 208339 [details] Patch for landing Clearing flags on attachment: 208339 Committed r153822: <http://trac.webkit.org/changeset/153822>
All reviewed patches have been landed. Closing bug.