@media rules ignored in user agent style sheet html.css

[James Craig]

Media query rule blocks are ignored in the WebKit user agent style sheet. Or more accurately, they are treated as if they always match. For example, the @media (inverted-colors) block in bug 168447 is always applied, regardless of whether colors are inverted.

Blocking bug 168447 (<rdar://30559874>)

[Radar WebKit Bug Importer]

<rdar://problem/30885951>

[Dean Jackson]

Interestingly, @media (foo) is evaluated to false, but a valid falsy media query like (inverted-colors) evaluates to true.

[Dean Jackson]

It's because MediaQueryEvaluator::evaluate doesn't have a document or frame in the context of the default style sheet, so uses its fallback value.

[Dean Jackson]

But this doesn't explain why the rules are applied, since the fallback value is false.

[Dean Jackson]

Oops. The fallback is true.

But amazingly, because it is created with:

static NeverDestroyed<const MediaQueryEvaluator> staticScreenEval("screen");

which is cast to the MediaQueryEvaluator(bool fallback), and the "s" evaluates to true.  The fun of template programming.

[Dean Jackson]

I have a fix for the invalid creation of the MediaQueryEvaluator, but that doesn't address this bug.

I'm going to have to ask Antti what he thinks is the best solution for associating a defaultStyleSheet or its rules with the current frame.

[Dean Jackson]

There isn't a StyleResolver to use when we call
RuleSet::addRulesFromSheet inside CSSDefaultStyleSheets.

(In reply to Dean Jackson from comment #6)
> I have a fix for the invalid creation of the MediaQueryEvaluator, but that
> doesn't address this bug.
> 
> I'm going to have to ask Antti what he thinks is the best solution for
> associating a defaultStyleSheet or its rules with the current frame.

Did you hear back from Antti?

[Antti Koivisto]

RuleSets for user agent stylesheets are shared between all documents. This is a major optimization that we don't want to lose. To allow media queries on UA sheets we would need to add a new type of UA sheet that is resolved per-document. Simplest approach would probably be have a different file (that is, not html4.css) for rules like this. Implementation for this should not be difficult at all.

Alternatively we could move media queries later in the style pipeline. For example we could generate RuleSets for all media queries and then pick between them at style resolution time. This is a bigger refactoring.

[Antti Koivisto]

Created attachment 326094 [details]
patch

[Antti Koivisto]

Created attachment 326117 [details]
patch

[Darin Adler]

Comment on attachment 326117 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326117&action=review

> Source/WebCore/ChangeLog:9
> +        To support accessibility features allow non-trivial @media rules in user agent stylesheet.

need a comma after "features"

> Source/WebCore/ChangeLog:48
> +            Also march rules in userAgentMediaQueryStyle.

march -> match

> Source/WebCore/css/CSSDefaultStyleSheets.cpp:125
> +    // Rulesets for these can't be global and need to be build in document context.

build -> built

> Source/WebCore/css/CSSDefaultStyleSheets.cpp:127
> +        if (is<StyleRuleMedia>(*rule)) {

I suggest using "continue" for this, just like the other if statements below.

> Source/WebCore/css/CSSDefaultStyleSheets.h:50
> +    static void initDefaultStyle(const Element*);

I don’t understand what changed that makes this need to be public instead of private now.

[Antti Koivisto]

> I don’t understand what changed that makes this need to be public instead of
> private now.

Everything was public before this patch.

[Antti Koivisto]

Created attachment 326126 [details]
patch

[WebKit Commit Bot]

Comment on attachment 326126 [details]
patch

Clearing flags on attachment: 326126

Committed r224495: <https://trac.webkit.org/changeset/224495>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.