Last Comment Bug 779917 - Implement CSS.supports()
: Implement CSS.supports()
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla20
Assigned To: Cameron McCormack (:heycam)
:
Mentors:
http://dev.w3.org/csswg/css3-conditio...
: 814923 (view as bug list)
Depends on: 814566
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-02 10:35 PDT by Paul Irish
Modified: 2013-08-04 03:11 PDT (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (12.73 KB, patch)
2012-08-02 19:53 PDT, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
patch (v1.1) (12.77 KB, patch)
2012-08-03 23:30 PDT, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
patch (v1.2) (15.28 KB, patch)
2012-11-24 22:06 PST, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
patch (v1.3) (15.33 KB, patch)
2012-11-25 20:03 PST, Cameron McCormack (:heycam)
dbaron: review+
Details | Diff | Splinter Review
patch (v1.4) (18.44 KB, patch)
2012-12-22 20:49 PST, Cameron McCormack (:heycam)
bzbarsky: feedback+
Details | Diff | Splinter Review

Description Paul Irish 2012-08-02 10:35:40 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/537.3 (KHTML, like Gecko) Chrome/22.0.1223.1 Safari/537.3

Steps to reproduce:

http://dev.w3.org/csswg/css3-conditional/#window-api defines an imperative API for checking support through the @supports conditional CSS functionality just implemented in Bug 649740
Comment 1 Boris Zbarsky [:bz] 2012-08-02 11:54:38 PDT
Seems reasonable to me.  Cameron, do you want to do this?  Should be reasonably simple, I think.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-02 12:02:00 PDT
We might want to get a slightly wider consensus on the name; I changed the name to supportsCSS and haven't yet discussed that name with anyone.  (I made that change because, in the context of the window object, I felt that having "CSS" in the name was important to provide additional context, and otherwise I wanted it to be short -- which means I didn't provide anything in the name to indicate that it has two arguments, a property-value pair.)
Comment 3 Cameron McCormack (:heycam) 2012-08-02 15:47:28 PDT
I can do it.  I'm also not sure about the name.  "supportsCSS" feels to me like it could take a string of some top-level CSS like "@media all { h1 { color: green } }".  Is "supportsProperty" any better?  (That might sound like it takes only a property name.)
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-02 16:13:24 PDT
Tab's original was supportsProperty, but that felt pretty odd to me on the window object (which is not at all CSS specific), particularly given the similarity in name to ECMA's defineProperty.  And supportCSSProperty feels rather long to me.
Comment 5 Cameron McCormack (:heycam) 2012-08-02 19:53:41 PDT
Created attachment 648595 [details] [diff] [review]
patch

This seems to work.  Not sure if you want to delay review until the name is more stable.
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-08-03 00:31:58 PDT
Comment on attachment 648595 [details] [diff] [review]
patch

Review of attachment 648595 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +3984,5 @@
> +
> +  nsCSSParser parser;
> +  parser.SetErrorReporting(false);
> +
> +  nsRefPtr<nsSimpleURI> uri(new nsSimpleURI);

I have no idea if this is correct... Why not just use the URLs from the document?
Comment 7 Cameron McCormack (:heycam) 2012-08-03 00:35:51 PDT
It shouldn't matter, and it was easier for me create a new object than work out how to get the document's URI. :)
Comment 8 Paul Irish 2012-08-03 01:20:56 PDT
supportsStyle() was my initial favorite. More specific than the "CSS" language and also rather terse.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-03 10:16:51 PDT
(In reply to :Ms2ger (Back and backlogged) from comment #6)
> > +  nsRefPtr<nsSimpleURI> uri(new nsSimpleURI);
> 
> I have no idea if this is correct... Why not just use the URLs from the
> document?

It is absolutely not correct.  Which URI class is correct to use depends on the scheme; nsSimpleURI is the correct class for about: URLs but not for http: URLs.  The only correct way to create a URI object is as described in https://wiki.mozilla.org/Gecko:Overview#URI_objects .
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-03 10:19:11 PDT
mDoc->GetDocumentURI() should be fine.
Comment 11 Cameron McCormack (:heycam) 2012-08-03 22:58:49 PDT
Do I need to check that mDoc is not null?  (When can it be null?)
Comment 12 Boris Zbarsky [:bz] 2012-08-03 23:00:39 PDT
It might be able to go null in cases when a page holds on to a window object for an iframe that has been removed from the document....
Comment 13 Cameron McCormack (:heycam) 2012-08-03 23:30:11 PDT
Created attachment 648956 [details] [diff] [review]
patch (v1.1)
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-30 15:39:48 PDT
http://lists.w3.org/Archives/Public/www-style/2012Aug/0749.html has the group decisions from San Diego about this; spec should get revised soon, but not yet.
Comment 15 Cameron McCormack (:heycam) 2012-11-24 22:01:36 PST
*** Bug 814923 has been marked as a duplicate of this bug. ***
Comment 16 Cameron McCormack (:heycam) 2012-11-24 22:06:56 PST
Created attachment 684915 [details] [diff] [review]
patch (v1.2)

I wasn't totally sure what to do if a premature end of the string was reached within a condition or property value.  The behaviour that fell out was the same as EOF handling in style sheets.  For example these return true:

  CSS.supports("(color: green) /*")
  CSS.supports("(color: green")
  CSS.supports("(font-family: 'Helvetica")
  CSS.supports("color", "green /*")
  CSS.supports("font-family", "'Helvetica")

and these false:

  CSS.supports("(font-family: 'Helvetica\n")
  CSS.supports("font-family", "'Helvetica\n")

Another I was unsure about was:

  CSS.supports("Color", "green")

That is currently returning true, but the spec says it should return true only if it is "a literal match for the name of a CSS property that the UA supports".
Comment 17 Cameron McCormack (:heycam) 2012-11-24 22:09:59 PST
bz: I'm using static Web IDL operations, and they generate static member functions that take an extra nsISupports* for the global object.  I don't need those in my supports() functions; is it worth being able to turn this argument off with something in Bindings.conf if it's not needed?
Comment 18 Masatoshi Kimura [:emk] 2012-11-25 19:46:56 PST
Comment on attachment 684915 [details] [diff] [review]
patch (v1.2)

>+if (!SpecialPowers.getBoolPref("layout.css.supports-rule.enabled")) {
>+  todo(false, "skipping test because pref is disabled");
>+} else {
Is it impossible to enable the pref for tests?
This check is virtually making the test useless.
Comment 19 Cameron McCormack (:heycam) 2012-11-25 19:49:43 PST
I don't think it's useless, since the test will still run on the mozilla-central and aurora builders, but you are right that it would be better to force the pref on like I do for the @supports reftests.
Comment 20 Cameron McCormack (:heycam) 2012-11-25 20:03:30 PST
Created attachment 685018 [details] [diff] [review]
patch (v1.3)

Force the @supports pref on in the test.
Comment 21 Boris Zbarsky [:bz] 2012-11-26 11:12:06 PST
Yeah, it might be worth having an annotation in the IDL to indicate that the function's behavior does not depend on the global and hence doesn't need it...  File a bug on that?
Comment 22 Cameron McCormack (:heycam) 2012-11-26 15:37:13 PST
Filed bug 815404 for that.
Comment 23 Masatoshi Kimura [:emk] 2012-11-26 17:22:54 PST
(In reply to Cameron McCormack (:heycam) from comment #19)
> I don't think it's useless, since the test will still run on the
> mozilla-central and aurora builders, but you are right that it would be
> better to force the pref on like I do for the @supports reftests.
Meant to say "useless to catch the regression" because the test will never fail as long as the pref is disabled.
Anyway, thanks to updating the patch promptly.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-12-21 12:26:25 PST
Comment on attachment 685018 [details] [diff] [review]
patch (v1.3)


CSS.webidl:

 - should have "conditionText" rather than "declaration"

>+ * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
>+ * liability, trademark and document use rules apply.

I don't think this is right.

layout/style/Makefile.in:

Put it only in EXPORTS_mozilla/dom and not in EXPORTS

nsCSSParser:

Could you name the new methods Evaluate rather than Parse?

>+  bool parsedOK = ParseProperty(propID);
>+  if (parsedOK && GetToken(true)) {
>+    parsedOK = false;
>+  }

Could you just do:

  bool parsedOK = ParseProperty(propID) && !GetToken(true);

(and likewise in the next method)




I wonder if:
 * it would be useful to pass a sheet URI for error reporting
 * are there any differences in what's valid depending on whether
   there's a base URI or not?
 * are there any differences in what's valid depending on the principal?

If so, maybe it's worth having the CSS object remember what document it
came from (probably two separate URIs, I suspect)?

I'd like Boris's opinions on those -- and probably also a brief look at the WebIDL end as well.

I probably should have redirected this review to bzbarsky at the
beginning.



The test would probably have been better written as a testharness.js
test, but at this point I wouldn't rewrite it.

r=dbaron with that contingent on bz's feedback
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-12-21 12:28:16 PST
(In reply to Cameron McCormack (:heycam) from comment #17)
> bz: I'm using static Web IDL operations, and they generate static member
> functions that take an extra nsISupports* for the global object.  I don't
> need those in my supports() functions; is it worth being able to turn this
> argument off with something in Bindings.conf if it's not needed?

I think the "I wonder if:" list in comment 24 rather contradicts this.
Comment 26 Boris Zbarsky [:bz] 2012-12-21 21:33:14 PST
The WebIDL builds look fine modulo the naming nit.

For the rest, in order:

1)  I think we should probably suppress error reporting during supports() calls.  Are we
    not doing that?

2)  The only thing that we treat as invalid or not depending on base URI are relative-URI
    @import rules.

3)  If you parse anything that might have attempts to call SetValueToURL you need to set 
    a principal.  If you don't, we'll just treat it as invalid CSS.  At the moment that
    seems to include anything involving -moz-image-rect, font src stuff, and anything
    involving VARIANT_URL.  Note that we don't _do_ anything with the principal per se,
    but URLValue requires a non-null principal...
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-12-21 21:55:10 PST
(In reply to Boris Zbarsky (:bz) from comment #26)
> 3)  If you parse anything that might have attempts to call SetValueToURL you
> need to set 
>     a principal.  If you don't, we'll just treat it as invalid CSS.  At the
> moment that
>     seems to include anything involving -moz-image-rect, font src stuff, and
> anything
>     involving VARIANT_URL.  Note that we don't _do_ anything with the
> principal per se,
>     but URLValue requires a non-null principal...

So this seems relevant to CSS.supports(), since people may well pass values that have url() in them.

If we're doing the work to pass a principal through, it seems we may as well pass the base and sheet (error reporting) URLs through as well.
Comment 28 Cameron McCormack (:heycam) 2012-12-22 18:58:39 PST
(In reply to David Baron [:dbaron] from comment #24)
> >+ * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> >+ * liability, trademark and document use rules apply.
> 
> I don't think this is right.

Why is that not right?  There are other .webidl files in that directory that are based on IDL snippets from W3C specs which have a similar copyright notice and that don't, so I'm not really sure what the rule is here.
Comment 29 Cameron McCormack (:heycam) 2012-12-22 19:21:47 PST
(In reply to Boris Zbarsky (:bz) from comment #26)
> The WebIDL builds look fine modulo the naming nit.
> 
> For the rest, in order:
> 
> 1)  I think we should probably suppress error reporting during supports()
> calls.  Are we
>     not doing that?

We're not explicitly, but I'm not OUTPUT_ERRORing them at the end.  I can use the nsAutoSuppressErrors to handle this explicitly (and avoid the overhead of building up the error strings).

(In reply to David Baron [:dbaron] from comment #27)
> If we're doing the work to pass a principal through, it seems we may as well
> pass the base and sheet (error reporting) URLs through as well.

I'm not sure it make sense to pass a sheet URL.  In

  <!DOCTYPE html>
  <script>CSS.supports("blah")</script>

what would it be?
Comment 30 Cameron McCormack (:heycam) 2012-12-22 20:49:51 PST
Created attachment 695279 [details] [diff] [review]
patch (v1.4)

Does it make sense to use the document's URI and principal like this?

Still not sure it is worth passing anything except the principal, given we won't report any CSS errors.
Comment 31 Boris Zbarsky [:bz] 2012-12-24 23:59:21 PST
Comment on attachment 695279 [details] [diff] [review]
patch (v1.4)

You should probably error out if "win" comes out null in GetParsingInfo, or something.  Though maybe using a null doc/base URI is ok.

You can use win->GetDoc() to get an nsIDocument* as retval without the XPCOM goop.  If it's null, again throw or us null URIs.

Apart from that, this looks fine.  The only thing I'd maybe change is using a struct return value that stores raw nsIURI* or something so we don't have to do the extra refcounting.
Comment 32 Cameron McCormack (:heycam) 2012-12-25 18:48:50 PST
(In reply to Boris Zbarsky (:bz) from comment #31)
> You should probably error out if "win" comes out null in GetParsingInfo, or
> something.  Though maybe using a null doc/base URI is ok.

Can win ever be null?  Only if aGlobal can be null I guess?
Comment 33 Cameron McCormack (:heycam) 2012-12-25 21:56:33 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a981b50c1d43
Comment 34 Boris Zbarsky [:bz] 2012-12-25 23:50:11 PST
> Can win ever be null?  Only if aGlobal can be null I guess?

I'm not sure what exactly can happen with sandboxes and JS components and whatnot there...  in general, aGlobal can in theory be null or can be just a non-window (e.g. a sandbox PrincipalHolder), but I'm not sure whether that can happen in practice.
Comment 35 Cameron McCormack (:heycam) 2012-12-26 01:00:52 PST
Added a null check:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0b040b16d8

Note You need to log in before you can comment on or make changes to this bug.