There's been some discussion of feature queries over time, particularly: http://lists.w3.org/Archives/Public/www-style/2004Dec/0084.html http://lists.w3.org/Archives/Public/www-style/2009Dec/0130.html http://lists.w3.org/Archives/Public/www-style/2009Dec/0135.html http://lists.w3.org/Archives/Public/www-style/2009Dec/0141.html http://lists.w3.org/Archives/Public/www-style/2009Dec/0144.html I just added: http://lists.w3.org/Archives/Public/www-style/2011Apr/0428.html I think we should do this.

This is the current contents of https://hg.mozilla.org/users/vmenezes_mozilla.com/patches/raw-file/68ddb24da314/at-supports (I'm not sure whether that URL will go away). At the end of his summer internship, I asked Vitor to look at implementing this. He made a good bit of progress (contained in this patch), though there are a bunch of major issues in this patch. The biggest one is related to parsing: getting the CSS parser to deal with end-of-declaration being a ) rather than a } or a ; requires a bit of work; this patch required extra {} around the declarations, I think, as a workaround. I think there was something else that was pretty hack-ish as well, but I've forgotten what.

(In reply to David Baron [:dbaron] from comment #1) > The biggest one is related to parsing: getting the CSS parser to deal with > end-of-declaration being a ) rather than a } or a ; requires a bit of work; > this patch required extra {} around the declarations, I think, as a > workaround. A little more detail on this one: It's possible we could get away with just making CSSParserImpl::CheckEndProperty check for ')' in addition to the characters it checks for. That would require a good bit of code auditing. However, really, all users of CheckEndProperty are fundamentally broken -- I think the proper way of building a recursive descent parser (especially one whose parsing code might be used in other contexts, as @supports does to all the property value parsers) is to: * check only for what you *do* accept * don't accept things that you don't accept * return true if the structure is complete and false if it's incomplete However, callers of CheckEndProperty (including those that call it through ExpectEndProperty, which is most of them) *sometimes* instead follow the model of: * check for the things that you don't accept but could still lead to the overall construct being valid (because the caller accepts them), and use them to terminate and return true * otherwise read things and return false if there's something unexpected (Either way callers are required to UngetToken() anything they don't handle. It often doesn't matter, but if the unexpected thing is a parenthesis then it's pretty important because of the error handling rules.)

Rejigged the parser, which probably needs more error reporting. I haven't verified that the change to CheckEndProperty is safe. A couple of questions: 1. What should GetCssText return for a SupportsRule? For declarations that are unrecognised but otherwise parse (e.g. it has an unknown property name, a property value that doesn't parse, or an unrecognised priority) should I be storing say the whole sequence of tokens on the RHS of the ":" on the SupportsRule object? 2. The spec says that for properties we don't implement well enough, they should be considered unsupported. Do we have any of these?

For serialising the rule object, the string returned after the "@-moz-supports" is just the exact text from the original style sheet, rather than storing (and later serialising) a list of tokens.

(In reply to Cameron McCormack (:heycam) from comment #3) > 2. The spec says that for properties we don't implement well enough, they > should be considered unsupported. Do we have any of these? dbaron tells me there aren't any.

Without some unnecessary changes to token serialisation.

just looking at the patch for the Scanner::StartRecording/StopRecording code - it would be awesome if I could get it as a separate patch, so I can reuse it for the variables implementation, but it's not a dealbreaker for me. + case nsIDOMCSSRule::MOZ_SUPPORTS_RULE: { + nsCOMPtr<nsIDOMCSSMediaRule> mediaRule = do_QueryInterface(rule); + nsCOMPtr<nsIDOMCSSRuleList> childRules; + mediaRule->GetCssRules(getter_AddRefs(childRules)); + SearchRuleList(childRules, aBaseURL); + } break; this does not seem correct to me - but I may be on crack/not overly familiar with the inspector code.

You're right, should be s/media/supports/ and s/Media/Supports/.

Comment on attachment 643341 [details] [diff] [review] Implement @-moz-supports (v1.1) Comments so far; I'm up to nsCSSRules.cpp, but I mostly skipped the reftests: Do we want this prefixed or unprefixed? Sounds like unprefixed. So please unprefix everything. No need to rev IID of nsIDOMCSSRule for a constant addition, I think. nsCSSToken::AppendToString changes (and new helper function) can probably go, as can mHasLeadingDot. Would be good to contribute tests to CSS WG. They look well-written; probably just need "This page should have a green background." text in all of them, and the metadata. Except for the prefixes... RightTrim should take nsSubstring&, not nsAString& (which is a compatibility name). Except really you should just use nsString::Trim(" ", false, true) rather than writing your own. (But why not trim leading spaces, actually? That seems better; SupportsRule::GetCssText would need to append a space afte rthe @supports, though.) ParseSupportsCondition: >+ REPORT_UNEXPECTED_EOF("\"not\" or \"(\""); This is wrong. This is expected to take the name of a string, and the string goes in dom/locales/en-US/chrome/layout/css.properties . See other examples for the expected part of the sentence that you should fill in for an unexpected EOF and the naming scheme for the messages. >+ if (mToken.IsSymbol('(')) { >+ return ParseSupportsConditionInParensAfterOpenParen(aConditionMet) && >+ ParseSupportsConditionTerms(aConditionMet); >+ } >+ >+ UngetToken(); >+ return false; This could be simplified to: + UngetToken(); + return ParseSupportsConditionInParens(aConditionMet) && + ParseSupportsConditionTerms(aConditionMet); since ParseSupportsConditionInParens is just reading a parenthesis combined with ParseSupportsConditionInParensAfterOpenParen. (It's not worth optimizing for errors; most CSS parses successfully.) ParseSupportsConditionInParensAfterOpenParen: >+ if (!ParseSupportsConditionInParensInsideParens(aConditionMet)) { >+ SkipUntil(')'); >+ return false; >+ } >+ >+ if (!ExpectSymbol(')', true)) { >+ SkipUntil(')'); >+ return false; >+ } These two ifs could be combined with ||. ParseSupportsConditionInParensInsideParens: >+ REPORT_UNEXPECTED_EOF("\"not\" or \"(\" or identifier"); same as before >+ aConditionMet = ParseProperty(propID) && >+ ParsePriority() != ePriority_Error; Hmm. I need to make the spec clearer about !important handling. >+ return ParseSupportsConditionNegationAfterNot(aConditionMet) && >+ ParseSupportsConditionTerms(aConditionMet); This is wrong. It allows: (not (color:red) and (color:blue)) which should not be allowed (since only one of not, and, or or should be allowed at a given level of ()). You should remove the part after the && and add a test. ParseSupportsConditionTerms: You should comment at the final |return false| that it doesn't matter what the return value is, but we may as well fail sooner. ParseSupportsConditionTermsAfterOperator: Same for the final return false in this function. in general, through the parsing code: There are a bunch of errors cases where no error is reported. The error reporting doesn't need to be that detailed (at a bare minimum, ParseSupportsRule should report an error on failure), but there should be some. - reftests - should go in w3c-css and follow the relevant rules

(In reply to David Baron [:dbaron] from comment #9) > No need to rev IID of nsIDOMCSSRule for a constant addition, I think. This isn't strictly necessary, because adding a value to the enum doesn't change the vtable, but it doesn't hurt.

Address review comments so far.

Comment on attachment 644411 [details] [diff] [review] Implement @supports. (v1.2) Review of attachment 644411 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/locales/en-US/chrome/layout/css.properties @@ +114,5 @@ > PEBadFontBlockStart=Expected '{' to begin @font-face rule but found '%1$S'. > PEBadFontBlockEnd=Expected '}' to end @font-face rule but found '%1$S'. > PEAnonBoxNotAlone=Did not expect anonymous box. > +PESupportsConditionStartEOF=starting 'not' or '(' of supports condition > +PESupportsConditionInParensStartEOF=starting 'not', or '(' or identifier of supports condition or declaration I'm not sure if it's me or the strings, but I don't understand these two, maybe rephrase them? Probably not only good for localizers.

The problem is that those are part of a larger sentence. The whole thing looks something like: PEUnexpEOF2=Unexpected end of file while searching for %1$S. and the strings being added here would be used as a param for the %1$S... Yes, this is mildly suboptimal; perhaps that prefix should just be copy/pasted into all the EOF strings.

... or there should be a comment that clarifies that, probably on each one that's used. But just expanding them sounds better, as that allows localizers to re-order the sentence in full, and the segment might not cut the way it does in English.

I'd really rather not expand them all; there are a *lot* of them and they're all used in only the one context.

(And, in general, they should probably be simpler than the ones here; I think the "of supports condition" bit isn't needed.)

Comment on attachment 644411 [details] [diff] [review] Implement @supports. (v1.2) So based on discussions of namespacing, this should probably be mozilla::CSSSupportsRule, and we shouldn't be using the mozilla::css namespace. SupportsRule::SizeOfIncludingThis should also include the buffer for mCondition. SupportsRule needs to implement nsIStyleRule::List. (Are you compiling non-DEBUG builds only? You should really build with --enable-debug.) nsCSSScanner.h: The comments should more clearly explain that StopRecording excludes the most-recently-read token. That said, it's not clear to me why you want it that way. Why do you not want the last token? (That would remove the need for mRecordEndOffset, which is the most expensive part of recording since it needs to be updated constantly.) And to follow up on the EOF strings, I think it would be sufficient to say: 'not' or '(' 'not', '(', or identifier And you still need some more error reporting per my previous comment; I haven't looked at how you addressed the other comments. But r=dbaron with all the comments addressed. We should think about whether we want to put the whole feature behind a pref, though. I'm inclined to think it's not needed, but not sure about that.

(In reply to David Baron [:dbaron] from comment #17) > So based on discussions of namespacing, this should probably be > mozilla::CSSSupportsRule, and we shouldn't be using the mozilla::css > namespace. OK. > SupportsRule::SizeOfIncludingThis should also include the buffer for > mCondition. OK. > SupportsRule needs to implement nsIStyleRule::List. (Are you compiling > non-DEBUG builds only? You should really build with --enable-debug.) I am compiling debug builds, but List is implemented by GroupRule, which is why I didn't get a compilation error. I've done so now though. > nsCSSScanner.h: > > The comments should more clearly explain that StopRecording excludes > the most-recently-read token. OK. > That said, it's not clear to me why you want it that way. Why do you > not want the last token? (That would remove the need for > mRecordEndOffset, which is the most expensive part of recording since > it needs to be updated constantly.) It's because at the end of the supports condition, the parser has got the next token from the scanner which is the "{", but we don't want to include that in the mCondition string. I guess we could search from the end of the string to find the "{" to remove it. That's simpler, so I've just gone ahead and done that. > And to follow up on the EOF strings, I think it would be sufficient to > say: > 'not' or '(' > 'not', '(', or identifier OK. > And you still need some more error reporting per my previous comment; > I haven't looked at how you addressed the other comments. Can you point out specifically where more error reporting is required? I can't see a point where an unexpected token is encountered that causes the parsing function to return false which doesn't already report it. > But r=dbaron with all the comments addressed. > > We should think about whether we want to put the whole feature behind > a pref, though. I'm inclined to think it's not needed, but not sure > about that. How do we decide this?

(In reply to Cameron McCormack (:heycam) from comment #18) > Can you point out specifically where more error reporting is required? I > can't see a point where an unexpected token is encountered that causes the > parsing function to return false which doesn't already report it. For example, I don't see anything that would report an error for: @supports foo {} since in that case ParseSupportsRule calls ParseSupportsCondition, which in turn calls ParseSupportsConditionNegation, which in turn finds that the next token is not a 'not' and returns false. This is far from the only case, but it's probably reasonable to address it with a very small number of error reports.

Thanks. I've added three error reporting instances now: * Peeking ahead in ParseSupportsRule after parsing the condition to ensure there is a "{" coming up (because ParseGroupRule doesn't do any error reporting for that). * Checking that the next token is "not" before calling ParseSupportsConditionNegation at the end of ParseSupportsCondition, and reporting that "not" or "(" is expected. * Reporting an expected "not" if EOF or some other token is found at the start of ParseSupportsConditionNegation. I think an error should be reported in all cases when there is a parsing failure now.

After addressing comments.

This puts the support for @supports behind the layout.css.supports-rule.enabled pref.

And now with the DOMClassInfo stuff to turn off window.CSSSupportsRule when the pref is not set. r?bz for that bit and r?dbaron for the rest of the behind-the-pref changes.

Comment on attachment 647438 [details] [diff] [review] Implement @supports. (v1.5) r=me on the classinfo bits.

Comment on attachment 647438 [details] [diff] [review] Implement @supports. (v1.5) Review of attachment 647438 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/css/nsIDOMCSSSupportsRule.idl @@ +16,5 @@ > + unsigned long insertRule(in DOMString rule, > + in unsigned long index) > + raises(DOMException); > + void deleteRule(in unsigned long index) > + raises(DOMException); I'd prefer removing those |raises(DOMException)|, for all the same reasons we dropped them from WebIDL. ::: layout/style/nsCSSRules.cpp @@ +2214,5 @@ > +} > + > +/* virtual */ bool > +CSSSupportsRule::UseForPresentation(nsPresContext* aPresContext, > + nsMediaQueryResultCacheKey& aKey) Indentation

Comment on attachment 647438 [details] [diff] [review] Implement @supports. (v1.5) Review of attachment 647438 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating the strings in general to be more simple. Please change the key of the one changed string still, though. ::: dom/locales/en-US/chrome/layout/css.properties @@ +21,5 @@ > PEGatherMediaNotIdent=Expected identifier in media list but found '%1$S'. > PEImportNotURI=Expected URI in @import rule but found '%1$S'. > PEImportBadURI=Invalid URI in @import rule: '%1$S'. > PEImportUnexpected=Found unexpected '%1$S' within @import. > +PEGroupRuleEOF=end of @media, @supports or @-moz-document rule You'll need to change the key here to make localizers aware of the changed semantics here. @@ +120,5 @@ > +PESupportsConditionExpectedOpenParen=Expected '(' while parsing supports condition but found '%1$S'. > +PESupportsConditionExpectedCloseParen=Expected ')' while parsing supports condition but found '%1$S'. > +PESupportsConditionExpectedStart=Expected 'not' or '(' while parsing supports condition but found '%1$S'. > +PESupportsConditionExpectedNot=Expected 'not' while parsing supports condition but found '%1$S'. > +PESupportsGroupRuleStart=Expected '{' to begin @supports rule but found '%1$S'. In general I'd advocate for localization notes to detail on what the param is on these, but it's obvious, probably more like this than with more text to read.

Comment on attachment 647438 [details] [diff] [review] Implement @supports. (v1.5) r=dbaron with the key name changed (as Axel pointed out -- you can just stick a '2' on the end of the name) and with the layout/reftests/reftest.list change that was in the previous patch included again (so that you actually run the tests)

(In reply to David Baron [:dbaron] from comment #27) > r=dbaron with the key name changed (as Axel pointed out -- you can just > stick a '2' on the end of the name) and with the > layout/reftests/reftest.list change that was in the previous patch included > again (so that you actually run the tests) I'd moved the tests to layout/reftests/w3c-css/submitted/conditional3 and changed layout/reftests/w3c-css/submitted/reftest.list so it should be picked up.

I've created: https://developer.mozilla.org/en-US/docs/DOM/CSSSupportsRule https://developer.mozilla.org/en-US/docs/CSS/@supports Updated: https://developer.mozilla.org/en-US/docs/DOM/CSSRule https://developer.mozilla.org/en-US/docs/Firefox_17_for_developers