Bug 86146

Summary: Implement css3-conditional's @supports rule
Product: WebKit Reporter: Pablo Flouret <pf>
Component: CSSAssignee: Pablo Flouret <pf>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, allan.jensen, cmarcelo, davidbarr, dbates, dglazkov, dstorey, eoconnor, ericbidelman, gyuyoung.kim, hyatt, jackalmage, kling, koivisto, komoroske, macpherson, menard, mihnea, owp-launch-tracking, paulirish, peter, peter+ews, rakuco, simon.fraser, syoichi, tabatkins, vestbo, webkit-bug-importer, webkit.bugzilla, webkit.review.bot
Priority: P2 Keywords: InRadar, WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 94290    
Bug Blocks: 116554, 134358    
Attachments:
Description Flags
Patch
none
Patch
koivisto: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-04
none
Patch for landing none

Description Pablo Flouret 2012-05-10 14:20:11 PDT
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
Comment 1 Pablo Flouret 2012-05-10 14:38:07 PDT
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.
Comment 2 Andreas Kling 2012-05-16 11:35:10 PDT
Implementation aside, I worry about claiming in such explicit terms that we "support" a given property simply because we can parse it.
Comment 3 Paul Irish 2012-05-17 17:49:20 PDT
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.
Comment 4 Andreas Kling 2012-05-17 18:12:02 PDT
@Paul: Fair enough, then I have no objections here. :)
Comment 5 Pablo Flouret 2012-05-31 14:43:08 PDT
Ping?

(can anyone suggest relevant reviewers if they're not cc:ed?)
Comment 6 Pablo Flouret 2012-08-02 09:51:53 PDT
Mozilla has this now:

https://bugzilla.mozilla.org/show_bug.cgi?id=649740
Comment 7 Pablo Flouret 2012-08-02 12:58:27 PDT
Unofficial reports that it's coming to Opera as well: https://twitter.com/frivoal/status/231101124298547200
Comment 8 Peter Beverloo 2012-08-02 13:00:57 PDT
(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.
Comment 9 Peter Beverloo (cr-android ews) 2012-08-14 05:44:30 PDT
Comment on attachment 141261 [details]
Patch

Attachment 141261 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13485882
Comment 10 Peter Beverloo 2012-08-22 06:52:11 PDT
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.
Comment 11 Pablo Flouret 2012-08-22 18:05:07 PDT
(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).
Comment 12 Tab Atkins 2012-08-22 18:58:14 PDT
(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.
Comment 13 Peter Beverloo 2012-08-23 04:04:12 PDT
(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.
Comment 14 Pablo Flouret 2012-08-24 19:51:38 PDT
Created attachment 160546 [details]
Patch
Comment 15 Pablo Flouret 2012-08-24 19:53:46 PDT
(In reply to comment #14)
> Created an attachment (id=160546) [details]
> Patch

Added the ENABLE flag and a few more tests.
Comment 16 WebKit Review Bot 2012-08-24 21:07:50 PDT
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
Comment 17 WebKit Review Bot 2012-08-24 21:07:56 PDT
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
Comment 18 Antti Koivisto 2012-10-04 11:13:28 PDT
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...
Comment 19 Tab Atkins 2012-10-09 14:19:19 PDT
(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.
Comment 20 Pablo Flouret 2012-10-17 12:27:21 PDT
Created attachment 169233 [details]
Patch for landing

No substantial changes. Rebased, added feature ifdefs in grammar file, skipped the tests in TestExpectation files.
Comment 21 WebKit Review Bot 2012-10-18 11:43:22 PDT
Comment on attachment 169233 [details]
Patch for landing

Clearing flags on attachment: 169233

Committed r131783: <http://trac.webkit.org/changeset/131783>
Comment 22 Kent Tamura 2012-10-18 19:13:19 PDT
Would you add CSS_CONDITIONAL_RULES to http://trac.webkit.org/wiki/FeatureFlags please?
Comment 23 Pablo Flouret 2012-10-19 10:42:05 PDT
(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.
Comment 24 Paul Irish 2012-10-20 15:23:07 PDT
Is there a complementary ticket for supportsCSS()? 
(Mozilla and Opera completed their implementation with it as well)
Comment 25 Pablo Flouret 2012-10-20 18:49:09 PDT
(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.
Comment 26 Tab Atkins 2012-10-20 18:51:29 PDT
(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.
Comment 27 Radar WebKit Bug Importer 2014-09-03 14:01:11 PDT
<rdar://problem/18219801>