Bug 119164 – REGRESSION (r121551) Incorrect handling of invalid media query list.

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 207555 [details]
Testcase

[Radar WebKit Bug Importer]

<rdar://problem/14564510>

[zalan]

regressed at http://trac.webkit.org/changeset/121551

patch is coming up.

[zalan]

Created attachment 208290 [details]
Patch

[Simon Fraser (smfr)]

How does this affect our results on http://www.w3.org/Style/CSS/Test/MediaQueries/20100726/media-queries-test.html ?

[zalan]

(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.

[zalan]

Created attachment 208324 [details]
Patch

[Antti Koivisto]

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()

[Rune Lillesveen]

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.

[zalan]

(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.

[Rune Lillesveen]

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.

[Rune Lillesveen]

(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.

[zalan]

tracking the error case here: bug 119573

[zalan]

Created attachment 208339 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 208339 [details]
Patch for landing

Clearing flags on attachment: 208339

Committed r153822: <http://trac.webkit.org/changeset/153822>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.