Bug 86146 – Implement css3-conditional's @supports rule

• Component: CSS
• Status: RESOLVED FIXED

[Pablo Flouret]

The ‘@supports’ rule is a conditional group rule whose condition tests whether the user agent supports CSS property:value pairs.

http://dev.w3.org/csswg/css3-conditional/#at-supports

[Pablo Flouret]

Created attachment 141261 [details]
Patch

CSS is not really in my area of expertise, so excuse any newbie mistakes :-). I suspect this needs more tests, suggestions very welcome.

[Andreas Kling]

Implementation aside, I worry about claiming in such explicit terms that we "support" a given property simply because we can parse it.

[Paul Irish]

Sir awesomekling, For Webkit to successfully parse a css property/value pair but not "support" it is a false positive for feature detection and should be avoided. 
For the most part, vendors to avoid successfully parsing and unsuccessfully implementing, (and Modernizr /HTML5Test/Caniuse/etc all file tickets when they don't) so I feel this dependency is fair.

[Andreas Kling]

@Paul: Fair enough, then I have no objections here. :)

[Pablo Flouret]

Ping?

(can anyone suggest relevant reviewers if they're not cc:ed?)

[Pablo Flouret]

Mozilla has this now:

https://bugzilla.mozilla.org/show_bug.cgi?id=649740

[Pablo Flouret]

Unofficial reports that it's coming to Opera as well: https://twitter.com/frivoal/status/231101124298547200

[Peter Beverloo]

(In reply to comment #7)
> Unofficial reports that it's coming to Opera as well: https://twitter.com/frivoal/status/231101124298547200

Florian works for Opera, so you can see that as a confirmation.

[Peter Beverloo (cr-android ews)]

Comment on attachment 141261 [details]
Patch

Attachment 141261 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13485882

[Peter Beverloo]

Comment on attachment 141261 [details]
Patch

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

Informal review. You can ignore the cr-android-ews failure.

Should we split up the tests into multiple files? That way, filenames can be used to identify what exactly is being tested, considering in a big list like this (without per-test comments) it may not always be obvious for someone unfamiliar with CSS Conditional Rules. Feel free to hold off on that unless a reviewer feels the same though, as it'd be a lot of work.

Ports which don't want this feature (yet) can't disable it right now. Should we add a compile-time feature define, i.e. ENABLE_CSS_CONDITIONAL_RULES?

> Source/WebCore/css/CSSGrammar.y:473
> +  | supports

The specification currently is a young Working Draft, so I think we should prefix this. Mozilla decided not to prefix (https://bugzilla.mozilla.org/show_bug.cgi?id=649740), but it's not entirely obvious to me why.

> LayoutTests/css3/supports.html:27
> +    @supports not not not not (display: none) {

This is incorrect, the declaration following a "not" negation must always be surrounded by parenthesis.

> LayoutTests/css3/supports.html:30
> +

Maybe add a test for double negation? i.e.

@supports not (not (display: block)) {}

> LayoutTests/css3/supports.html:122
> +    @supports(((((((display: none))))))) {

The latest Editor's Draft mentions this syntax for supports_rule:
"SUPPORTS_SYM S+ supports_condition group_rule_body"

Which implies that one or more space characters need to follow the @supports rule. Mozilla's implementation does not mandate this requirement. Is there some part of the specification I'm missing?

> LayoutTests/css3/supports.html:129
> +

Some idea for more tests:
- Do functions work as property values?
- What about functions that'll be evaluated later, i.e. using calc()?

Variables are valid values too, but their value is derived from parent rules. I guess that means that it's impossible to test support for properties which values use variables, despite them being valid.

[Pablo Flouret]

(In reply to comment #10)
> (From update of attachment 141261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141261&action=review
> 
> Ports which don't want this feature (yet) can't disable it right now. Should we add a compile-time feature define, i.e. ENABLE_CSS_CONDITIONAL_RULES?

Can do.
 
> > Source/WebCore/css/CSSGrammar.y:473
> > +  | supports
> 
> The specification currently is a young Working Draft, so I think we should prefix this. Mozilla decided not to prefix (https://bugzilla.mozilla.org/show_bug.cgi?id=649740), but it's not entirely obvious to me why.

I didn't prefix it because i thought it would defeat the whole purpose, and it also seemed like a simple feature with syntax that was agreed upon by enough people, but i guess you never know with the csswg :P

Mozilla has this to say (that i could find):
"[*] As prefixing this at-rule has no sense, the @supports at-rule is only supported if the user enables it by setting the config value layout.css.supports-rule.enable to true."
From https://developer.mozilla.org/en-US/docs/CSS/@supports

I'll ask the opera guys what they're planning.

> > LayoutTests/css3/supports.html:27
> > +    @supports not not not not (display: none) {
> 
> This is incorrect, the declaration following a "not" negation must always be surrounded by parenthesis.

Right, i misread the grammar.

> > LayoutTests/css3/supports.html:122
> > +    @supports(((((((display: none))))))) {
> 
> The latest Editor's Draft mentions this syntax for supports_rule:
> "SUPPORTS_SYM S+ supports_condition group_rule_body"
> 
> Which implies that one or more space characters need to follow the @supports rule. Mozilla's implementation does not mandate this requirement. Is there some part of the specification I'm missing?

Everywhere else S* is used, so i think it was just inertia on my part. Maybe the same thing happened to Cameron?.
Funnily enough, it doesn't seem like they pass the tests in this patch that don't have whitespace between the operators and the conditions (i.e. #t21 to #t24).

[Tab Atkins]

(In reply to comment #11)
> (In reply to comment #10)
> > > Source/WebCore/css/CSSGrammar.y:473
> > > +  | supports
> > 
> > The specification currently is a young Working Draft, so I think we should prefix this. Mozilla decided not to prefix (https://bugzilla.mozilla.org/show_bug.cgi?id=649740), but it's not entirely obvious to me why.
> 
> I didn't prefix it because i thought it would defeat the whole purpose, and it also seemed like a simple feature with syntax that was agreed upon by enough people, but i guess you never know with the csswg :P
> 
> Mozilla has this to say (that i could find):
> "[*] As prefixing this at-rule has no sense, the @supports at-rule is only supported if the user enables it by setting the config value layout.css.supports-rule.enable to true."
> From https://developer.mozilla.org/en-US/docs/CSS/@supports
> 
> I'll ask the opera guys what they're planning.

Please do not prefix it.  We can keep it off the stable channel until it's ready, but prefixing would, as you say, completely defeat the purpose.  We discussed this a few weeks ago at the CSSWG face-to-face.

[Peter Beverloo]

(In reply to comment #12)
> Please do not prefix it.  We can keep it off the stable channel until it's ready, but prefixing would, as you say, completely defeat the purpose.  We discussed this a few weeks ago at the CSSWG face-to-face.

Ok, agreed. In that case we definitely need a compile-time flag.

[Pablo Flouret]

Created attachment 160546 [details]
Patch

[Pablo Flouret]

(In reply to comment #14)
> Created an attachment (id=160546) [details]
> Patch

Added the ENABLE flag and a few more tests.

[WebKit Review Bot]

Comment on attachment 160546 [details]
Patch

Attachment 160546 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13599350

New failing tests:
css3/supports.html

[WebKit Review Bot]

Created attachment 160552 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Antti Koivisto]

Comment on attachment 160546 [details]
Patch

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

> Source/WebCore/css/CSSGrammar.y:663
>    ;
>  
> +supports:
> +    SUPPORTS_SYM maybe_space supports_condition '{' maybe_space block_rule_list save_block {

I think it would be good to add feature #if to the grammar too so keep track of everything that belongs to this feature. Bug 94290 has a WIP patch...

[Tab Atkins]

(In reply to comment #10)
> > LayoutTests/css3/supports.html:122
> > +    @supports(((((((display: none))))))) {
> 
> The latest Editor's Draft mentions this syntax for supports_rule:
> "SUPPORTS_SYM S+ supports_condition group_rule_body"
> 
> Which implies that one or more space characters need to follow the @supports rule. Mozilla's implementation does not mandate this requirement. Is there some part of the specification I'm missing?

Since @media has an S* there (and everyone is interop about it), we've changed @supports to match.

[Pablo Flouret]

Created attachment 169233 [details]
Patch for landing

No substantial changes. Rebased, added feature ifdefs in grammar file, skipped the tests in TestExpectation files.

[WebKit Review Bot]

Comment on attachment 169233 [details]
Patch for landing

Clearing flags on attachment: 169233

Committed r131783: <http://trac.webkit.org/changeset/131783>

[Kent Tamura]

Would you add CSS_CONDITIONAL_RULES to http://trac.webkit.org/wiki/FeatureFlags please?

[Pablo Flouret]

(In reply to comment #22)
> Would you add CSS_CONDITIONAL_RULES to http://trac.webkit.org/wiki/FeatureFlags please?

Yeah, i'll get to it once i manage to log into trac.

[Paul Irish]

Is there a complementary ticket for supportsCSS()? 
(Mozilla and Opera completed their implementation with it as well)

[Pablo Flouret]

(In reply to comment #24)
> Is there a complementary ticket for supportsCSS()? 
> (Mozilla and Opera completed their implementation with it as well)

I don't think so, but i was planning on looking at that next week. Feel free to file a bug for it and cc me.

[Tab Atkins]

(In reply to comment #25)
> (In reply to comment #24)
> > Is there a complementary ticket for supportsCSS()? 
> > (Mozilla and Opera completed their implementation with it as well)
> 
> I don't think so, but i was planning on looking at that next week. Feel free to file a bug for it and cc me.

Special very important note!  The function was renamed to just "supports()", on a new global CSS object.  Check the Editor's Draft for details.

[Radar WebKit Bug Importer]

<rdar://problem/18219801>