Status

()

defect
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: paulirish, Assigned: heycam)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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
Component: Untriaged → DOM: CSS Object Model
Product: Firefox → Core
Seems reasonable to me.  Cameron, do you want to do this?  Should be reasonably simple, I think.
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.)
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.)
Assignee: nobody → cam
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
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.
Posted patch patch (obsolete) — Splinter Review
This seems to work.  Not sure if you want to delay review until the name is more stable.
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?
It shouldn't matter, and it was easier for me create a new object than work out how to get the document's URI. :)
(Reporter)

Comment 8

7 years ago
supportsStyle() was my initial favorite. More specific than the "CSS" language and also rather terse.
(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 .
mDoc->GetDocumentURI() should be fine.
Do I need to check that mDoc is not null?  (When can it be null?)
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....
Posted patch patch (v1.1) (obsolete) — Splinter Review
Attachment #648595 - Attachment is obsolete: true
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.
Duplicate of this bug: 814923
Depends on: 814566
Summary: Implement supportsCSS() → Implement CSS.supports()
Posted patch patch (v1.2) (obsolete) — Splinter Review
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".
Attachment #648956 - Attachment is obsolete: true
Attachment #684915 - Flags: review?(dbaron)
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?
Flags: needinfo?(bzbarsky)
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.
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.
Posted patch patch (v1.3) (obsolete) — Splinter Review
Force the @supports pref on in the test.
Attachment #684915 - Attachment is obsolete: true
Attachment #684915 - Flags: review?(dbaron)
Attachment #685018 - Flags: review?(dbaron)
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?
Flags: needinfo?(bzbarsky)
Filed bug 815404 for that.
(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 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
Attachment #685018 - Flags: review?(dbaron)
Attachment #685018 - Flags: review+
Attachment #685018 - Flags: feedback?(bzbarsky)
(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.
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...
(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.
(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.
(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?
Posted patch patch (v1.4)Splinter Review
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.
Attachment #685018 - Attachment is obsolete: true
Attachment #685018 - Flags: feedback?(bzbarsky)
Attachment #695279 - Flags: feedback?(bzbarsky)
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.
Attachment #695279 - Flags: feedback?(bzbarsky) → feedback+
(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?
> 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.
https://hg.mozilla.org/mozilla-central/rev/a981b50c1d43
https://hg.mozilla.org/mozilla-central/rev/0b0b040b16d8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.