288796 - Add public ParseColorString method

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Patch adds a ParseColorString method to nsICSSParser, for use by things (such as
<canvas>) that need to parse CSS colors outside of a stylesheet.  It also adds a
HandleAlphaColors flag which can be set to allow the parser to understand
rgba()/hsla(), default to false.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: cssparser-color.patch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My fear of recycled parsers suggests I'd prefer not having SetHandleAlphaColors,
but instead having the boolean be a parameter to ParseColor, which would set the
member to the parameter at the start of the function and to PR_FALSE at the end.

I'll look more closely tomorrow...

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Either way works for me; I assumed you meant have a setter method like the
current flags.  Would that parameter have a C++ default of PR_FALSE, to avoid
having to change current callers?  (And to indicate common usage?)

Let me know if you'd like me to make the change.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Not a parameter to anything that exists now -- just a parameter to the
nsICSSParser method, to avoid having a setter on nsICSSParser and to avoid
accidentally leaving the parser in the state.  (i.e., modeled on mHTMLMediaMode)

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: cssparser-color-2.patch

Try this one on for size.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 179931 [details] [diff] [review]
cssparser-color-2.patch

>+                              nscolor& aColor,
>+                              PRBool aHandleAlphaColors);

Switch the order of these two so the out param is last, and use nscolor*
instead of nscolor& .  (This is basically an XPCOM interface.)

>Index: nsCSSParser.cpp

Could you change SetParsingCompoundProperty, SetCaseSensitive, and SetSVGMode
so they assert that their argument is either PR_TRUE or PR_FALSE (since PRBool
can hold other values, unfortunately).

>+  mHandleAlphaColors = aHandleAlphaColors;
>+
>+  rv = InitScanner(aBuffer, aURL, aLineNumber, aURL);
>+  if (NS_FAILED(rv))
>+    return rv;

Switch the order of these two things (so mHandleAlphaColors isn't touched until
you know you will undo it).

>+  nsCSSValue value;
>+  if (ParseColor(rv, value)) {
>+    if (value.GetUnit() == eCSSUnit_String) {
>+      nsAutoString s;
>+      nscolor rgba;
>+      if (NS_ColorNameToRGB(value.GetStringValue(s), &rgba)) {

It looks like you can assert that this succeeded rather than checking it, based
on ParseColor.

>+        aColor = rgba;
>+        rv = NS_OK;
>+      } else {
>+        rv = NS_ERROR_FAILURE;
>+      }


>+    } else if (value.GetUnit() == eCSSUnit_Color) {
>+      aColor = value.GetColorValue();
>+      rv = NS_OK;
>+    } else if (value.GetUnit() == eCSSUnit_Integer) {
>+      rv = NS_ERROR_FAILURE;

Are you really supposed to be ignoring system colors and currentColor?	If so,
that should be documented in the header file (nsICSSParser.h), and have a
comment here explaining that that's what this is doing.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: cssparser-color-3.patch

Changes made

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Fixed missing = 0; in nsICSSParser interface

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 180503 [details] [diff] [review]
cssparser-color-3.patch

This still doesn't handle currentColor in the integer case -- you need an
additional parameter to the parser to handle that.  I'm not crazy about the
nsILookAndFeel dependency, but I guess it's OK.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Is it ok to just add in a comment and return failure for currentColor?  Or
better yet, specify that in the currentColor case, that the nscolor param won't
be modified?

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: cssparser-color-4.patch

Added comment in nsICSSParser.h describing handling of colors that aren't fully
specified by the string (excluding colors that can be obtained from LaF), and
handle the <0 case for Integer values.	Also cleaned up unnecessary setting of
rv to error conditions.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 180994 [details] [diff] [review]
cssparser-color-4.patch

>+      // this is NS_COLOR_CURRENTCOLOR, NS_COLOR_MOZ_HYPERLINKTEXT, etc.
>+      // which we don't handle as per the ParseColorString definition
>+      rv = NS_ERROR_FAILURE;

This is probably something we want to fix, so add "XXX" at the start of the
comment, and r=dbaron.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

XXX added; r+sr=dbaron or should I poke around for a quick sr?

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 180994 [details] [diff] [review]
cssparser-color-4.patch

a=asa

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Landed, thanks!