Bug 169245 - @media rules ignored in user agent style sheet html.css
Summary: @media rules ignored in user agent style sheet html.css
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 169987
Blocks: 168447
  Show dependency treegraph
 
Reported: 2017-03-06 18:50 PST by James Craig
Modified: 2017-11-06 09:45 PST (History)
9 users (show)

See Also:


Attachments
patch (17.91 KB, patch)
2017-11-05 18:02 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (20.04 KB, patch)
2017-11-06 06:18 PST, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff
patch (20.01 KB, patch)
2017-11-06 09:26 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2017-03-06 18:50:59 PST
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>)
Comment 1 Radar WebKit Bug Importer 2017-03-06 18:51:16 PST
<rdar://problem/30885951>
Comment 2 Dean Jackson 2017-03-22 16:16:04 PDT
Interestingly, @media (foo) is evaluated to false, but a valid falsy media query like (inverted-colors) evaluates to true.
Comment 3 Dean Jackson 2017-03-22 18:08:32 PDT
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.
Comment 4 Dean Jackson 2017-03-22 18:11:54 PDT
But this doesn't explain why the rules are applied, since the fallback value is false.
Comment 5 Dean Jackson 2017-03-22 18:18:52 PDT
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.
Comment 6 Dean Jackson 2017-03-22 18:39:21 PDT
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.
Comment 7 Dean Jackson 2017-03-22 18:43:21 PDT
There isn't a StyleResolver to use when we call
RuleSet::addRulesFromSheet inside CSSDefaultStyleSheets.
Comment 8 mitz 2017-06-26 09:33:02 PDT
(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?
Comment 9 Antti Koivisto 2017-06-26 10:55:09 PDT
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.
Comment 10 Antti Koivisto 2017-11-05 18:02:28 PST
Created attachment 326094 [details]
patch
Comment 11 Antti Koivisto 2017-11-06 06:18:06 PST
Created attachment 326117 [details]
patch
Comment 12 Darin Adler 2017-11-06 08:50:54 PST
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.
Comment 13 Antti Koivisto 2017-11-06 09:19:45 PST
> I don’t understand what changed that makes this need to be public instead of
> private now.

Everything was public before this patch.
Comment 14 Antti Koivisto 2017-11-06 09:26:10 PST
Created attachment 326126 [details]
patch
Comment 15 WebKit Commit Bot 2017-11-06 09:45:54 PST
Comment on attachment 326126 [details]
patch

Clearing flags on attachment: 326126

Committed r224495: <https://trac.webkit.org/changeset/224495>
Comment 16 WebKit Commit Bot 2017-11-06 09:45:55 PST
All reviewed patches have been landed.  Closing bug.