1035106 - Add new APIs to DOMUtils: colorToRGBA, IsValidCSSColor & cssPropertyIsValid

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We have had ongoing issues with the devtools output parser and color conversion and validation. We need some APIs to help with the parser and color conversion:

jsval colorToRGBA(in DOMString aColorString) // Convert any valid color string to RGBA, returns null on failure.
e.g. DOMUtils.colorToRGBA("red") // returns rgba(255,0,0,1)

bool colorIsValid(in AString aColorString) // Check whether any color string is valid.
e.g. DOMUtils.colorIsValid("hsla(100,20%,15%,0.6") // returns true

bool cssPropertyIsValid(in AString aProperty, in AString aValue) // Check whether any CSS property is valid.
e.g. DOMUtils.cssPropertyIsValid("color", "blue !important") // returns true

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch v1

These are fairly simple APIs to prevent us having to use hidden windows. This will allow us to use some of our methods in B2G and remove thousands of lines of cruft from test logs.

Comment 2

[Cameron McCormack (:heycam)]

Relatedly, Tab just posted this to www-style: http://wiki.csswg.org/ideas/color-object

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8451501 [details] [diff] [review]
Patch v1

+++ b/dom/webidl/InspectorUtils.webidl

>+   * NOTE: Using octet for RGBA components is not generally OK, because
>+   * they can be outside the 0-255 range, but for backwards-compatible
>+   * named colors (which is what we use this dictionary for)

Except that's not what we're using the dictionary for.  We're using this for arbitrary CSS colors.

Please use a type here that would actually be ok expressing the full range of possible CSS colors.  "double" would make sense, with some comments explaining that this is in the normal 0-255-sized RGB space but might be fractional and might extend outside the 0-255 range.

>+++ b/layout/inspector/inDOMUtils.cpp
>+#include "Declaration.h"

mozilla/css/Declaration.h, please.

>+inDOMUtils::ColorToRGBA(const nsAString& aColorString, JSContext* aCx,
>+  bool isColor = cssParser.ParseColorString(aColorString, nullptr, 0, cssValue);

This will report warnings if the string is not a valid color, right?  It might make sense to change ParseColorString to allow suppressing error reporting.

>+    aValue.setNull();

This needs to be documented in the IDL.  In fact, the IDL needs to have documentation for these new methods in general.  That documentation should probably document that converting a color to RGBA is lossy, by the way (even if you allow fractional values and values outside 0-255; iirc even that is not enough to do lossless conversion of CMYK to RGBA).

>+  nsCSSUnit unit = cssValue.GetUnit();

This code doesn't seem to handle system colors, right?

Is there a reason you're not using nsRuleNode::ComputeColor here?  That still won't handle currentColor or some of the prescontext-dependent colors, but should at least do system colors.

>+  if (!ToJSValue(aCx, tuple, aValue)) {
>+    aValue.setNull();

No, if ToJSValue returns false, you want to return NS_ERROR_FAILURE or something instead of pressing on with an exception pending on aCx.

>+inDOMUtils::ColorIsValid(const nsAString& aColorString, bool *_retval)

Maybe call this isValidCSSColor?

Again, this looks like it will do error reporting as things stand and we don't want that, right?

>+  nsCOMPtr<nsIDocument> doc = nsContentUtils::GetDocumentFromContext();
>+
>+  if (!doc) {
>+    doc = nsContentUtils::GetDocumentFromCaller();
>+  }

This doesn't make much sense to me.  Why would you not care which document you end up with?

Also, at this point doc can still be null.

>+  nsRefPtr<mozilla::dom::Element> rootElement = doc->GetRootElement();

And even if doc is not null, at this point rootElement can be null.

>+  nsIDocument* ownerDoc = rootElement->OwnerDoc();

That better be the same as doc!

>+  nsCOMPtr<nsIURI> baseURI = rootElement->GetBaseURI();

Why not just the document's base URI?

But why do you need a base URI at all?

>+  attribute.Append(aValue);

So if I call 

  cssPropertyIsValid("color", "red; background: green");

this code will return true.  That seems wrong to me (and we should add a test for that).  We should do something better here, like creating a Declaration and using ParseProperty().

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: colorIsValid-colorToRgba-cssPropertyIsValid.patch

(In reply to Boris Zbarsky [:bz] from comment #3)
> Comment on attachment 8451501 [details] [diff] [review]
> Patch v1
> 
> +++ b/dom/webidl/InspectorUtils.webidl
> 
> >+   * NOTE: Using octet for RGBA components is not generally OK, because
> >+   * they can be outside the 0-255 range, but for backwards-compatible
> >+   * named colors (which is what we use this dictionary for)
> 
> Except that's not what we're using the dictionary for.  We're using this for
> arbitrary CSS colors.
> 
> Please use a type here that would actually be ok expressing the full range
> of possible CSS colors.  "double" would make sense, with some comments
> explaining that this is in the normal 0-255-sized RGB space but might be
> fractional and might extend outside the 0-255 range.
> 

Done

> >+++ b/layout/inspector/inDOMUtils.cpp
> >+#include "Declaration.h"
> 
> mozilla/css/Declaration.h, please.
> 

Done

> >+inDOMUtils::ColorToRGBA(const nsAString& aColorString, JSContext* aCx,
> >+  bool isColor = cssParser.ParseColorString(aColorString, nullptr, 0, cssValue);
> 
> This will report warnings if the string is not a valid color, right?  It
> might make sense to change ParseColorString to allow suppressing error
> reporting.
> 

To be honest I thought I would see any logging in the browser console... I didn't realize that the Error Console was still a thing.

Done, the same with ParseProperty.

> >+    aValue.setNull();
> 
> This needs to be documented in the IDL.  In fact, the IDL needs to have
> documentation for these new methods in general.  That documentation should
> probably document that converting a color to RGBA is lossy, by the way (even
> if you allow fractional values and values outside 0-255; iirc even that is
> not enough to do lossless conversion of CMYK to RGBA).
> 
> >+  nsCSSUnit unit = cssValue.GetUnit();
> 
> This code doesn't seem to handle system colors, right?
> 

Right.

> Is there a reason you're not using nsRuleNode::ComputeColor here?  That
> still won't handle currentColor or some of the prescontext-dependent colors,
> but should at least do system colors.
> 

Didn't find that one, fixed.

> >+  if (!ToJSValue(aCx, tuple, aValue)) {
> >+    aValue.setNull();
> 
> No, if ToJSValue returns false, you want to return NS_ERROR_FAILURE or
> something instead of pressing on with an exception pending on aCx.
> 

Of course, fixed.

> >+inDOMUtils::ColorIsValid(const nsAString& aColorString, bool *_retval)
> 
> Maybe call this isValidCSSColor?
> 

Done

> Again, this looks like it will do error reporting as things stand and we
> don't want that, right?
> 

Fixed

> >+  nsCOMPtr<nsIDocument> doc = nsContentUtils::GetDocumentFromContext();
> >+
> >+  if (!doc) {
> >+    doc = nsContentUtils::GetDocumentFromCaller();
> >+  }
> 
> This doesn't make much sense to me.  Why would you not care which document
> you end up with?
> 
> Also, at this point doc can still be null.
> 
> >+  nsRefPtr<mozilla::dom::Element> rootElement = doc->GetRootElement();
> 
> And even if doc is not null, at this point rootElement can be null.
> 
> >+  nsIDocument* ownerDoc = rootElement->OwnerDoc();
> 
> That better be the same as doc!
> 
> >+  nsCOMPtr<nsIURI> baseURI = rootElement->GetBaseURI();
> 
> Why not just the document's base URI?
> 
> But why do you need a base URI at all?
> 

Fixed

The whole point of that block was to meet the preconditions in CSSParserImpl::ParseProperty:
NS_PRECONDITION(aSheetPrincipal, "Must have principal here!");
NS_PRECONDITION(aBaseURI, "need base URI");

As you point out, these preconditions are not actually preconditions. Removing these two "preconditions" prevents any inappropriate error output.

> >+  attribute.Append(aValue);
> 
> So if I call 
> 
>   cssPropertyIsValid("color", "red; background: green");
> 
> this code will return true.  That seems wrong to me (and we should add a
> test for that).

Done

> We should do something better here, like creating a
> Declaration and using ParseProperty().

Done. The method no longer needs a document etc. We need to trim "!important" from the end of the property value because that is what ParseProperty expects but this method is now very simple.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> To be honest I thought I would see any logging in the browser console...

Er... you should have.  If you didn't see it there in this case, it's presumably you have it set to not show CSS warnings.

> As you point out, these preconditions are not actually preconditions. 

Hmm.  They are for producing correct values for properties that can involve URIs.  Let me think a bit about how to do this in your case.  Fundamentally, you don't _actually_ care about values, which is the only reason you don't need a base URI.

> We need to trim "!important"

And "! important" and variants.  Again, let me think about how to do this better.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I think we could do the following:

1)  Change the return value of ParseProperty from nsresult (which is always NS_OK right now) to a boolean indicating whether the parse was successful.  This should be a separate patch (though it's OK to do it in this bug) under the one already in this bug.

2)  Change ParseProperty so that it allows passing a null aDeclaration, and when null is passed skips the preconditions on aSheetPrincipal/aBaseURI, suppresses errors, and allows !important before the EOF.  Again, separate patch.

Alternately, we could create a new API on nsCSSParser (e.g. IsValueValidForProperty) that would do what we want.  It would need to duplicate a bit of ParseProperty (the scanner setup and the check for preffed off things) but could skip all the error reporting junk and declaration stuff.  This might be the simplest thing to do.  Are you up for doing that?

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> So I think we could do the following:
> 
> 1)  Change the return value of ParseProperty from nsresult (which is always
> NS_OK right now) to a boolean indicating whether the parse was successful. 
> This should be a separate patch (though it's OK to do it in this bug) under
> the one already in this bug.
> 
> 2)  Change ParseProperty so that it allows passing a null aDeclaration, and
> when null is passed skips the preconditions on aSheetPrincipal/aBaseURI,
> suppresses errors, and allows !important before the EOF.  Again, separate
> patch.
> 
> Alternately, we could create a new API on nsCSSParser (e.g.
> IsValueValidForProperty) that would do what we want.  It would need to
> duplicate a bit of ParseProperty (the scanner setup and the check for
> preffed off things) but could skip all the error reporting junk and
> declaration stuff.  This might be the simplest thing to do.  Are you up for
> doing that?

Sure, I think option 3 is the best choice here.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Let's do that, then.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: colorIsValid-colorToRgba-cssPropertyIsValid.patch

The only changes are reverting ParseProperty, creating IsValueValidForProperty, using it in CssPropertyIsValid and adding "! important" check to test.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8456990 [details] [diff] [review]
colorIsValid-colorToRgba-cssPropertyIsValid.patch

>+++ b/dom/webidl/InspectorUtils.webidl
>+   * NOTE: This tuple is in the normal 0-255-sized RGB space

  The "r", "g", and "b" values in this tuple are in the ...

The "a" value is 0-1, and should be documented accordingly.

>+++ b/layout/inspector/inIDOMUtils.idl
>+  // Convert a given CSS color string to rgba. Returns null on failure or a JS
>+  // object in the format {r:red g:green b:blue a:alpha} on success.

Just document this as returning a WebIDL InspectorRGBATuple so you don't have to re-describe the value ranges here?

>+  // Check whether a CSS property and value are a valid combination.
>+  bool cssPropertyIsValid(in AString aProperty, in AString aValue);

Please document what aValue is in this case, and in particular that it's allowed to include !important (which is not actually a part of the value of a CSS property; it's an annotation on the declaration).

And maybe aPropertyName?

Also, please document the interaction with pref-disabled properties?

>+++ b/layout/inspector/tests/test_color_to_rgba.html
>+      ok(expected === null, "color: " + color + "returns null");

Please add a space between the '"' and the 'r' of "returns".

>+    is(r, expected.r, "color: " + color + ", red component is a converted correctly");

s/is a/is/ and likewise for g and b.

>+++ b/layout/style/nsCSSParser.cpp
> CSSParserImpl::ParseColorString(const nsSubstring& aBuffer,
>-  OUTPUT_ERROR();
>+  if (aSuppressErrors) {
>+    CLEAR_ERROR();
>+  } else {
>+    OUTPUT_ERROR();

Hrm.  I guess you don't get that for free with nsAutoSuppressErrors.  That's annoying.  :(

>+CSSParserImpl::IsValueValidForProperty(const nsCSSProperty aPropID,
>+  // Check for unknown or preffed off properties
>+  if (eCSSProperty_UNKNOWN == aPropID) {

This is not actually checking for preffed-off properties.  Should it be?  What behavior do we want the DOMUtils API to have?

>+  mTempData.ClearProperty(aPropID);

We should probably CLEAR_ERROR() right after this line too.

r=me.  Thank you; this is really nice!

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: colorIsValid-colorToRgba-cssPropertyIsValid.patch

(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8456990 [details] [diff] [review]
> colorIsValid-colorToRgba-cssPropertyIsValid.patch
> 
> >+++ b/dom/webidl/InspectorUtils.webidl
> >+   * NOTE: This tuple is in the normal 0-255-sized RGB space
> 
>   The "r", "g", and "b" values in this tuple are in the ...
> 
> The "a" value is 0-1, and should be documented accordingly.
> 

Done

> >+++ b/layout/inspector/inIDOMUtils.idl
> >+  // Convert a given CSS color string to rgba. Returns null on failure or a JS
> >+  // object in the format {r:red g:green b:blue a:alpha} on success.
> 
> Just document this as returning a WebIDL InspectorRGBATuple so you don't
> have to re-describe the value ranges here?
> 

Done

> >+  // Check whether a CSS property and value are a valid combination.
> >+  bool cssPropertyIsValid(in AString aProperty, in AString aValue);
> 
> Please document what aValue is in this case, and in particular that it's
> allowed to include !important (which is not actually a part of the value of
> a CSS property; it's an annotation on the declaration).
> 

Done.

> And maybe aPropertyName?
> 

Changed... and aPropertyValue, plus propertyID for values returned by LookupProperty().

> Also, please document the interaction with pref-disabled properties?
> 

Done

> >+++ b/layout/inspector/tests/test_color_to_rgba.html
> >+      ok(expected === null, "color: " + color + "returns null");
> 
> Please add a space between the '"' and the 'r' of "returns".
> 

Done.

> >+    is(r, expected.r, "color: " + color + ", red component is a converted correctly");
> 
> s/is a/is/ and likewise for g and b.
> 

Done.

> >+++ b/layout/style/nsCSSParser.cpp
> > CSSParserImpl::ParseColorString(const nsSubstring& aBuffer,
> >-  OUTPUT_ERROR();
> >+  if (aSuppressErrors) {
> >+    CLEAR_ERROR();
> >+  } else {
> >+    OUTPUT_ERROR();
> 
> Hrm.  I guess you don't get that for free with nsAutoSuppressErrors.  That's
> annoying.  :(
> 

Yes, I almost chose to have OUTPUT_ERROR call CLEAR_ERROR when using nsAutoSuppressErrors but I controlled myself ;)

> >+CSSParserImpl::IsValueValidForProperty(const nsCSSProperty aPropID,
> >+  // Check for unknown or preffed off properties
> >+  if (eCSSProperty_UNKNOWN == aPropID) {
> 
> This is not actually checking for preffed-off properties.  Should it be? 
> What behavior do we want the DOMUtils API to have?
> 

I hadn't changed the comment. We want to process as many properties as possible so if we have an ID that is fine.

Updated comment.

> >+  mTempData.ClearProperty(aPropID);
> 
> We should probably CLEAR_ERROR() right after this line too.
>

Done.
 
> r=me.  Thank you; this is really nice!

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: colorToRGBA-isValidCSSColor-cssPropertyIsValid-1035106.patch

Updated patch title

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8457889 [details] [diff] [review]
colorToRGBA-isValidCSSColor-cssPropertyIsValid-1035106.patch

https://hg.mozilla.org/integration/fx-team/rev/c1d47fd03032

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8457889 [details] [diff] [review]
colorToRGBA-isValidCSSColor-cssPropertyIsValid-1035106.patch

Backed out:
https://hg.mozilla.org/integration/fx-team/rev/47b760a88f04

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  Missing iid bump on inIDOMUtils.idl, maybe?

Comment 16

[Carsten Book [:Tomcat]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Hmm.  Missing iid bump on inIDOMUtils.idl, maybe?

yeah looks like this was the issue.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: colorToRGBA-isValidCSSColor-cssPropertyIsValid-1035106.patch

Bumped uuid.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=d50c9fee3b1a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

FWIW, Try builds are always clobbers, so UUID bumps there won't tell you anything useful. Personally, I'd just cancel the run and re-push to fx-team instead.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8458007 [details] [diff] [review]
colorToRGBA-isValidCSSColor-cssPropertyIsValid-1035106.patch

https://hg.mozilla.org/integration/fx-team/rev/ed168e7e8b0c

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed168e7e8b0c