Bug 169245

Summary: @media rules ignored in user agent style sheet html.css
Product: WebKit Reporter: James Craig <jcraig>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dino, ggaren, koivisto, mitz, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 169987    
Bug Blocks: 168447    
Attachments:
Description Flags
patch
none
patch
darin: review+
patch none

James Craig
Reported 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>)
Attachments
patch (17.91 KB, patch)
2017-11-05 18:02 PST, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
patch (20.04 KB, patch)
2017-11-06 06:18 PST, Antti Koivisto 		darin: review+
DetailsFormatted DiffDiff
patch (20.01 KB, patch)
2017-11-06 09:26 PST, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2017-03-06 18:51:16 PST
<rdar://problem/30885951>
Dean Jackson
Comment 2 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.
Dean Jackson
Comment 3 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.
Dean Jackson
Comment 4 2017-03-22 18:11:54 PDT
But this doesn't explain why the rules are applied, since the fallback value is false.
Dean Jackson
Comment 5 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.
Dean Jackson
Comment 6 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.
Dean Jackson
Comment 7 2017-03-22 18:43:21 PDT
There isn't a StyleResolver to use when we call RuleSet::addRulesFromSheet inside CSSDefaultStyleSheets.
mitz
Comment 8 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?
Antti Koivisto
Comment 9 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.
Antti Koivisto
Comment 10 2017-11-05 18:02:28 PST
Created attachment 326094 [details] patch
Antti Koivisto
Comment 11 2017-11-06 06:18:06 PST
Created attachment 326117 [details] patch
Darin Adler
Comment 12 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.
Antti Koivisto
Comment 13 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.
Antti Koivisto
Comment 14 2017-11-06 09:26:10 PST
Created attachment 326126 [details] patch
WebKit Commit Bot
Comment 15 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>
WebKit Commit Bot
Comment 16 2017-11-06 09:45:55 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.