Closed
Bug 1035106
Opened 7 years ago
Closed 7 years ago
Add new APIs to DOMUtils: colorToRGBA, IsValidCSSColor & cssPropertyIsValid
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: miker, Assigned: miker)
References
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 5 obsolete files)
23.71 KB,
patch
|
miker
:
review+
miker
:
checkin+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
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.
Attachment #8451501 -
Flags: review?(bzbarsky)
Comment 2•7 years ago
|
||
Relatedly, Tab just posted this to www-style: http://wiki.csswg.org/ideas/color-object
![]() |
||
Comment 3•7 years ago
|
||
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().
Attachment #8451501 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Attachment #8451501 -
Attachment is obsolete: true
Attachment #8456817 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•7 years ago
|
Summary: Add new APIs to DOMUtils: colorToRGBA, colorIsValid & cssPropertyIsValid → Add new APIs to DOMUtils: colorToRGBA, IsValidCSSColor & cssPropertyIsValid
![]() |
||
Comment 5•7 years ago
|
||
> 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•7 years ago
|
||
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?
Flags: needinfo?(mratcliffe)
Assignee | ||
Comment 7•7 years ago
|
||
(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.
Flags: needinfo?(mratcliffe)
![]() |
||
Comment 8•7 years ago
|
||
Let's do that, then.
Assignee | ||
Comment 9•7 years ago
|
||
The only changes are reverting ParseProperty, creating IsValueValidForProperty, using it in CssPropertyIsValid and adding "! important" check to test.
Attachment #8456817 -
Attachment is obsolete: true
Attachment #8456817 -
Flags: review?(bzbarsky)
Attachment #8456990 -
Flags: review?(bzbarsky)
![]() |
||
Comment 10•7 years ago
|
||
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!
Attachment #8456990 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 11•7 years ago
|
||
(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!
Attachment #8456990 -
Attachment is obsolete: true
Attachment #8457460 -
Flags: review+
Assignee | ||
Comment 12•7 years ago
|
||
Updated patch title
Attachment #8457460 -
Attachment is obsolete: true
Attachment #8457889 -
Flags: review+
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8457889 [details] [diff] [review] colorToRGBA-isValidCSSColor-cssPropertyIsValid-1035106.patch https://hg.mozilla.org/integration/fx-team/rev/c1d47fd03032
Attachment #8457889 -
Flags: checkin+
Assignee | ||
Comment 14•7 years ago
|
||
Comment on attachment 8457889 [details] [diff] [review] colorToRGBA-isValidCSSColor-cssPropertyIsValid-1035106.patch Backed out: https://hg.mozilla.org/integration/fx-team/rev/47b760a88f04
Attachment #8457889 -
Flags: checkin+
![]() |
||
Comment 15•7 years ago
|
||
Hmm. Missing iid bump on inIDOMUtils.idl, maybe?
Comment 16•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #15) > Hmm. Missing iid bump on inIDOMUtils.idl, maybe? yeah looks like this was the issue.
Assignee | ||
Comment 17•7 years ago
|
||
Bumped uuid. Try: https://tbpl.mozilla.org/?tree=Try&rev=d50c9fee3b1a
Attachment #8457889 -
Attachment is obsolete: true
Attachment #8458007 -
Flags: review+
Comment 18•7 years ago
|
||
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.
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8458007 [details] [diff] [review] colorToRGBA-isValidCSSColor-cssPropertyIsValid-1035106.patch https://hg.mozilla.org/integration/fx-team/rev/ed168e7e8b0c
Attachment #8458007 -
Flags: checkin+
Assignee | ||
Updated•7 years ago
|
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/ed168e7e8b0c
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•