1295456 - Implement css-color-4 changes to color function syntax

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

CSS Color Module Level 4 mentions that alpha value accepts percentrage.  We should support it.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I would like to do this.

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] support percentage opacity value in colors. v1

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: test_color.html

test page for percentage opacity

Comment 4

[Daniel Holbert [:dholbert]]

As I noted in bug 1295451, I think we should make *all* of CSS Color 4's changes to rgb/rgba-expressions at approximately the same time.  So before we take patches for these bugs, we probably need plans for e.g. comma-less rgb() / rgba() expressions, and for making rgba() and rgb() take the same syntax, and perhaps some other changes as well.

(That way, we'll be able to more clearly communicate in release notes / developer outreach / etc. that we're supporting "CSS Color 4 rgb/rgba expressions" in a particular release.)

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Percentage alpha also affects hsl() / hsla(). 
According to changes in CSS Color 4 spec[1], I think we could also support angle value for hues in hsl() / hsla() at the same time.

[1] https://drafts.csswg.org/css-color/#changes-from-3

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

hsla() and rgba() use the same parser for alpha value, so the attachment 8781810 [details] [diff] [review] could handle both case.

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Hiroyuki,

I would like to implement the change of
https://drafts.csswg.org/css-color/#changes-from-3
for my first css-parser practice(the alias of rgba/rgb hsl/hsla and the percentage value of alpha).

Is there any duplicate work with this and bug 1295451?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Though I am not a CSS staff guy, as far as I can tell, there is no bug for the changes.

We have a bug for color() function, a new feature of CS Color Module Level 4.  It's bug 1128204.

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I will also implement the alias for rgba()/rgb() and hsl()/hsla().

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [wip] make rgb() as an alias of rgba(). v1

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [wip] make rgb() as an alias of rgba(). v2

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [wip] make hsl() as an alias of hsla(). v1

Comment 14

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P1. accept two unexpected characters report

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P2. update css parser error messages

Comment 16

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3. support percentage opacity value in color function

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4. make function rgb() as an alias of rgba() and support css-color-4 comma-less expression

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5. make function hsl() as an alias of hsla() and support css4 comma-less expression

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6. remove unused function

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8783297 [details] [diff] [review]
P3. support percentage opacity value in color function

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

::: layout/style/nsCSSParser.cpp
@@ +6958,5 @@
>      return false;
>    }
>  
> +  // eCSSToken_Number or eCSSToken_Percentage.
> +  if (mToken.mType != eCSSToken_Number && mToken.mType != eCSSToken_Percentage) {

https://drafts.csswg.org/css-color/#rgb-functions

<alpha-value> = <number> | <percentage>

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8783298 [details] [diff] [review]
P4. make function rgb() as an alias of rgba() and support css-color-4 comma-less expression

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

::: layout/style/nsCSSParser.cpp
@@ +6687,5 @@
>        }
>        break;
>      case eCSSToken_Function:
> +      if (mToken.mIdent.LowerCaseEqualsLiteral("rgb") ||
> +          mToken.mIdent.LowerCaseEqualsLiteral("rgba")) {

merge rgb() and rgba()

@@ +6808,5 @@
> +                             ComponentType& aB,
> +                             ComponentType& aA)
> +{
> +  const char seperator = ',';
> +  const char whiteSpaceOpacitySeperator = '/';

rgb() = rgb( <percentage>{3} [ / <alpha-value> ]? ) |
        rgb( <number>{3} [ / <alpha-value> ]? )

or

rgb() = rgb( <percentage>#{3} , <alpha-value>? ) |
        rgb( <number>#{3} , <alpha-value>? )

@@ -6816,5 @@
>      REPORT_UNEXPECTED_EOF(PEColorComponentEOF);
>      return false;
>    }
>  
> -  if (mToken.mType != eCSSToken_Number || !mToken.mIntegerValid) {

https://drafts.csswg.org/css-color/#rgb-functions

The <number> syntax allows full <number>s, not just <integer>s, for authoring convenience.

@@ +6994,5 @@
>    return true;
>  }
>  
>  bool
> +CSSParserImpl::ParseColorOpacity(uint8_t& aOpacity, char aSeperator)

Similar to the original ParseColorOpacity() function, but it also check the separator before opacity value.

Comment 22

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8783299 [details] [diff] [review]
P5. make function hsl() as an alias of hsla() and support css4 comma-less expression

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

::: layout/style/nsCSSParser.cpp
@@ +6714,5 @@
>          SkipUntil(')');
>          return CSSParseResult::Error;
>        }
> +      else if (mToken.mIdent.LowerCaseEqualsLiteral("hsl") ||
> +               mToken.mIdent.LowerCaseEqualsLiteral("hsla")) {

merge hsl() and hsla().

@@ +6900,3 @@
>  {
> +  const char seperator = ',';
> +  const char whiteSpaceOpacitySeperator = '/';

hsl() = hsl( <hue> <percentage> <percentage> [ / <alpha-value> ]? )

or

hsl() = hsl( <hue>, <percentage>, <percentage>, <alpha-value>? )

<hue> = <number> | <angle>

I haven't implement the <angle> yet.

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7. fix typo

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8783294 [details] [diff] [review]
P1. accept two unexpected characters report

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

Please improve the commit message. The current message ("accept two unexpected characters report") is extremely vague & hard to understand.

Perhaps something like:
  Extend the CSS Parser's error-reporter to support error-message templates that have a token and two character values.

r=me with the commit message clarified along those lines, and with nits below addressed.

::: layout/style/ErrorReporter.cpp
@@ +332,5 @@
> +  nsAutoString tokenString;
> +  aToken.AppendToString(tokenString);
> +  const char16_t charStrOne[] = { aCharOne, '\0' };
> +  const char16_t charStrTwo[] = { aCharTwo, '\0' };
> +  const char16_t *params[] = { tokenString.get(), charStrOne, charStrTwo};

add a space before "}" here.

::: layout/style/nsCSSParser.cpp
@@ -6928,5 @@
>    REPORT_UNEXPECTED_TOKEN_CHAR(PEColorComponentBadTerm, aStop);
>    return false;
>  }
>  
> -

Please revert this blank-line-removal -- it's best not to mix whitespace-only changes into patches that make functional changes, particularly when the whitespace changes are in unrelated parts of the file.

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8783295 [details] [diff] [review]
P2. update css parser error messages

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

As above, the commit message ("update css parser error messages") is a bit too vague here.  ("update" could mean a lot of different things -- reword a bunch of the messages? Pull in some updates from some upstream source? fix typos?)

Please clarify to something like this:
> Add 2 new error message templates to CSS Parser (to be used when parsing CSS color values).

r=me with that.

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8783297 [details] [diff] [review]
P3. support percentage opacity value in color function

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

Commit message nit -- the commit message currently says:
 "support percentage opacity value in color function"

Please replace:
 s/color function/CSS color functions/

Specifically:
 - add "CSS" to better-indicate what part of the code this patch is touching
 - pluralize "functions" because otherwise it sounds like you're talking about one particular "color function". (e.g. some nonexistent "color()")

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8783302 [details] [diff] [review]
P7. fix typo

Jumping ahead to "P7":

r- on this one, because it looks like it's simply fixing a group of typos that you're introducing in the earlier patches on this same bug.

Please just fix the typo *up-front* in the earlier patches, rather than introducing the typo and then using an extra patch/commit to fix it.

(You should be able to do this relatively easily by doing a search-and-replace on your earlier patch files themselves.)

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8783298 [details] [diff] [review]
P4. make function rgb() as an alias of rgba() and support css-color-4 comma-less expression

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

I'll get to more substantial review comments on this patch later, but for now, one commit message nit for both P4 and P5:

> "make function rgb() as an alias of rgba()"
> "make function hsl() as an alias of hsla()

Please rewrite like so:
  s/make...as/change...to be/

Comment 29

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

Comment on attachment 8783295 [details] [diff] [review]
P2. update css parser error messages

It would be more useful, for those looking at history in the future, if these were introduced in the same patch that uses them, rather than being a separate patch.

Comment 30

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Another problem is that, should we print the comma-less string for serialization?

from css object model, there is no new change for the comma-less case.
https://drafts.csswg.org/cssom/#serialize-a-css-component-value

Comment 31

[Daniel Holbert [:dholbert]]

I agree with dbaron's comment 29... Could you remove P2, and put its contents (2 new lines) into patches 4 and 5? (adding each line in the same patch that uses it.)

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #30)
> Another problem is that, should we print the comma-less string for
> serialization?

Unless there's a good reason not to, I'd say we should probably use commas in the serialization...
 (1) ...to approximately follow the CSSOM spec-text that you linked to there
 (2) ...and to avoid changing our behavior for existing legacy content (where inputs & serializations currently will both include commas).

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8783295 [details] [diff] [review]
P2. update css parser error messages

(changing r+ to r- on P2, per first half of my previous comment in light of dbaron's thoughts)

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 8783298 [details] [diff] [review]
P4. make function rgb() as an alias of rgba() and support css-color-4 comma-less expression

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

My review comments for P4 here largely apply to P5 as well.

::: layout/style/nsCSSParser.cpp
@@ +1170,5 @@
>    // ParseColorOpacity will enforce that the color ends with a ')'
>    // after the opacity
>    bool ParseColorOpacity(uint8_t& aOpacity);
>    bool ParseColorOpacity(float& aOpacity);
> +  // ParseColorOpacity() will try to check whether if there has a opacity value.

The wording here is a bit clumsy & confusing ("try to check whether if there has" -- there are some redundant words thrown in there, and it's not really clear what it's trying to say)

I think this line wants to be replaced with something clearer & more readable like:
  // The ParseColorOpacity methods below attempt to parse an optional separator and
  // opacity (alpha) component inside of a CSS color function (e.g. "rgba()").

@@ +1171,5 @@
>    // after the opacity
>    bool ParseColorOpacity(uint8_t& aOpacity);
>    bool ParseColorOpacity(float& aOpacity);
> +  // ParseColorOpacity() will try to check whether if there has a opacity value.
> +  // If it doesn't, set a default for aOpacity. The 'aSeperator' is the

This line needs rewording as well. In particular:
 - "If it doesn't" is vague (if *what* doesn't *what*? I think you're saying "If no valid opacity is found"? Not sure though.)
 - "Set a default" is vague (what default?)
 - Unclear whether we return true or false in this "if it doesn't" case.

@@ +1173,5 @@
>    bool ParseColorOpacity(float& aOpacity);
> +  // ParseColorOpacity() will try to check whether if there has a opacity value.
> +  // If it doesn't, set a default for aOpacity. The 'aSeperator' is the
> +  // seperator before the opacity value.
> +  // This function is used for number opacity value. The value is in [0, 255].

The comment
 "The value is in [...range...]"
needs rewording here.  It's unclear whether it's declaring an expectation about the parsed value, or a promise about the returned value, or both.

This phrase gets repeated several times for functions below this one -- please clarify it & apply whatever clarification you use to all of its repetitions.

@@ +1179,5 @@
> +  // This function is used for percentage opacity value. The value is in
> +  // [0.0f, 1.0f].
> +  bool ParseColorOpacity(float& aOpacity, char aSeperator);
> +  // Parse the number value. The value is in [0, 255].
> +  bool ParseColorComponent(uint8_t& aComponent);

Please add a blank line of separation between the chunk of ParseColorOpacity() functions & the chunk of ParseColorComponent functions, so they're more clearly separated.

Also, "Parse the number value" is too vague to be helpful as documentation here. Please reword like so, perhaps:
 s/Parse the number value/Parse a <number> color component/

@@ +1181,5 @@
> +  bool ParseColorOpacity(float& aOpacity, char aSeperator);
> +  // Parse the number value. The value is in [0, 255].
> +  bool ParseColorComponent(uint8_t& aComponent);
> +  // Parse the percentage value. The value is in [0.0f, 1.0f]
> +  bool ParseColorComponent(float& aComponent);

Same here -- this should perhaps say "Parse a <percentage> color component."

@@ +6808,5 @@
> +                             ComponentType& aB,
> +                             ComponentType& aA)
> +{
> +  const char seperator = ',';
> +  const char whiteSpaceOpacitySeperator = '/';

It's confusing to name this "whiteSpaceOpacitySeparator". When I saw this used in code, I assumed it was some sort of whitespace token.

Since this is only used once, you could probably just get rid of the variable entirely and pass '/' directly at that one usage-site.  But if you'd really like to put it in a named variable, it might be better to name it something like "alphaSeparatorInNoCommaExpressions", perhaps. (emphasizing "no-comma expressions" instead of "whitespace", to avoid giving the impression that this is a whitespace token).

@@ +6815,5 @@
> +  if (!ParseColorComponent(aR)) {
> +    return false;
> +  }
> +  // Check the color function which uses white space or seperator to seperate
> +  // each component.

This is a little hard to understand.  Maybe just reword as:
  // Look for whitespace and/or separator after "r" component:

@@ +6847,5 @@
> +      return false;
> +    }
> +    if (!RequireWhitespace()) {
> +      REPORT_UNEXPECTED_TOKEN_CHAR(PEColorComponentBadTerm, ' ');
> +      return false;

A few things here:

(1) Are you sure we need to *require* whitespace (and fail if we don't find it) in this no-comma scenario?

In practice, the tokenizer should already be breaking at whitespace boundaries anyway -- the only special case to care about is that it also breaks at comment boundaries.  So I think this early-return will only really be triggered for style like this:
 rgb(10/**/20/**/30/**/)
...and I'm not sure that style really merits a failure. It looks like we parse syntax like that for other list-valued properties, e.g. we parse "border-width: 0/**/0;" just fine, despite the lack of a space separator. And the CSS Syntax here is "<number>{3}", and the documentation for {3} syntax doesn't mention any whitespace requirements at all. For reference:
 https://drafts.csswg.org/css-color/#rgb-functions
 https://drafts.csswg.org/css-values-4/#mult-num

(2) Regardless of what the correct behavior here is for the just-discussed whitespace requirements, please include testcases that have no whitespace (i.e. whose components are only separated by comments), to be sure that such values are accepted/rejected as we expect them to be.

(3) I think this high-level structure...
 if (hasSeparator) { parse g, b, a } else { parse g, b, a }
...is too repetitive (the whole g/b/a thing being repeated twice).  The if/else clauses need to be collapsed together, to avoid all the high-level repetition.

(4) For the separator-parsing & error-reporting, I think it might be worth keeping that inside of ParseNumberColorComponent() via an "aStop" arg, with "aStop" converted into a 'Maybe<char>' so that we can optionally avoid requiring it, as-appropriate. Then I imagine this code would look something like:
  if (!ParseColorComponent(aG,
                           hasSeparator? Nothing() : Some(','))) {
    return false;
  }
  if (!ParseColorComponent(aB, Nothing()) {
    // (note: no required separator after 'B'; we'll check before Alpha)
    return false;
  }

  char opacitySeparator = hasSeparator ? ',' : '/';
  if (!ParseColorOpacityAndCloseParen(aA, opacitySeparator)) {
    return false;
  }

  return true;

Comment 34

[Daniel Holbert [:dholbert]]

(Side note: I ran into a DevTools bug when testing whether whitespace is really required in CSS syntax -- I filed bug 1297890 on that.  I'm mentioning it here in case you happen to run into the same bug when coming up with testcases here. :))

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 8783298 [details] [diff] [review]
P4. make function rgb() as an alias of rgba() and support css-color-4 comma-less expression

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

One more nit on P4:

::: layout/style/nsCSSParser.cpp
@@ +6691,5 @@
> +          mToken.mIdent.LowerCaseEqualsLiteral("rgba")) {
> +        // rgb() = rgb( <percentage>{3} [ / <alpha-value> ]? ) |
> +        //         rgb( <number>{3} [ / <alpha-value> ]? ) |
> +        //         rgb( <percentage>#{3} , <alpha-value>? ) |
> +        //         rgb( <number>#{3} , <alpha-value>? )

This syntax on the last two lines is wrong, actually! (It's wrong in the spec, too -- which explains why it's wrong here. :))

This version of the syntax implies that a trailing comma would be *required* after the "b" component, regardless of whether an alpha value is present. So "rgb(0,0,0)" would be invalid, and "rgb(0,0,0,)" would be valid.

That's clearly a mistake in the spec -- the comma should be grouped with the <alpha-value> in being optional, just like the "/" is in the first version of the syntax.

Could you file a spec bug on that (using the "GitHub issues" link at the top of the spec), and use what you think would be the correct syntax here? (and make a reminder to update this syntax after the spec bug has been fixed)

This is broken for the "hsl()" legacy syntax as well, both in the spec and in your quote in P5.  So, your github issue should probably cover the typo in both locations (for rgb & hsl).

Comment 36

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

(In reply to Daniel Holbert [:dholbert] from comment #35)
> This version of the syntax implies that a trailing comma would be *required*
> after the "b" component, regardless of whether an alpha value is present. So
> "rgb(0,0,0)" would be invalid, and "rgb(0,0,0,)" would be valid.
> 
> That's clearly a mistake in the spec -- the comma should be grouped with the
> <alpha-value> in being optional, just like the "/" is in the first version
> of the syntax.

Nope, see https://drafts.csswg.org/css-values-3/#comb-comma

(I almost filed a bug on this recently, but then remembered that.)

Comment 37

[Daniel Holbert [:dholbert]]

Ah, thanks! Disregard comment 35 then.

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8783299 [details] [diff] [review]
P5. make function hsl() as an alias of hsla() and support css4 comma-less expression

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

I'm marking P5 as r- for now, and not looking at it too thoroughly at this point, since most of my P4 review comments apply to P5 as well.

I'll gladly take a closer look after you've addressed the above comments throughout P4 and P5.

Comment 39

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I will update the p5 and p6 and then send the review request again.

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8783300 [details] [diff] [review]
P6. remove unused function

P6 looks fine, though consider rewording commit message to clarify what it's doing (and that the function is only just now becoming unused via other related patches), e.g. something like:

> Remove now-unused single-arg ParseColorOpacity functions from nsCSSParser

Comment 41

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8783294 [details] [diff] [review]
P1. accept two unexpected characters report

After refining the css parser patch, this change could be skipped.

Comment 42

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8783295 [details] [diff] [review]
P2. update css parser error messages

After refining the css parser patch, this change could be skipped.

Comment 43

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3. support percentage opacity value in CSS color functions. v2, r=dholbert

Comment 44

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v2

Accept rgb(10/**/20/**/30/**/); the comment only expression.
Refine the redundant code path in rgba() color function.
Update the non-clear comment for functions.

Comment 45

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5. change CSS color function hsl() to be an alias of hsla() and support css-color-4 comma-less expression. v2

Comment 46

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6. remove now-unused single-arg ParseColorOpacity functions from nsCSSParser. v2. r=dholbert

rebase and update the comment.

Comment 47

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7. reftest for CSS color functions. v1

test for the css color functions alias.
test for comment only expression in comma-less color function syntax.
test for the default opacity value.

Comment 48

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8785309 [details] [diff] [review]
P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v2

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

::: layout/style/nsCSSParser.cpp
@@ +6723,4 @@
>            float r, g, b, a;
> +          bool hasOpacity;
> +          if (ParseRGBColor(r, g, b, a, hasOpacity)) {
> +            if (hasOpacity) {

I want to recognize the css string has opacity or not.

Our serializer has different output for that.
https://hg.mozilla.org/mozilla-central/annotate/24763f58772d45279a935790f732d80851924b46/layout/style/nsCSSValue.cpp#l1522

Comment 49

[Daniel Holbert [:dholbert]]

Comment on attachment 8785307 [details] [diff] [review]
P3. support percentage opacity value in CSS color functions. v2, r=dholbert

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

::: layout/style/nsCSSParser.cpp
@@ +6957,5 @@
>    }
>  
> +  // eCSSToken_Number or eCSSToken_Percentage.
> +  if (mToken.mType != eCSSToken_Number && mToken.mType != eCSSToken_Percentage) {
> +    REPORT_UNEXPECTED_TOKEN(PEExpectedPercentOrNumber);

PEExpectedPercentOrNumber isn't an error message that exists in the current tree. It looks like you were adding it in (now-obsoleted) patch labeled "P2" here -- which means

Please rescue that change from P2, and include it in this patch here.

Comment 50

[Daniel Holbert [:dholbert]]

(er, sorry for trailing off mid-comment there. :) you get the point, anyway.)

Comment 51

[Daniel Holbert [:dholbert]]

Comment on attachment 8785309 [details] [diff] [review]
P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v2

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

::: layout/style/nsCSSParser.cpp
@@ +1154,5 @@
>    bool ParseBoxCornerRadiiInternals(nsCSSValue array[]);
>    bool ParseBoxCornerRadii(const nsCSSProperty aPropIDs[]);
>  
>    int32_t ParseChoice(nsCSSValue aValues[],
>                        const nsCSSProperty aPropIDs[], int32_t aNumIDs);

Note: you probably want to rebase your patches on top of a more recent tree!  This ^^ particular line of contextual code changed 2 weeks ago, in bug 1293739 comment 21 (where we renamed "nsCSSProperty" to "nsCSSPropertyID").

Your patch still applies with fuzz, fortunately:
curl -l https://bug1295456.bmoattachments.org/attachment.cgi?id=8785309 | patch -p1 -F8
...but that may not be true anymore if further contextual changes happen.

Comment 52

[Daniel Holbert [:dholbert]]

Comment on attachment 8785309 [details] [diff] [review]
P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v2

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

::: layout/style/nsCSSParser.cpp
@@ +1174,5 @@
>    // after the opacity
>    bool ParseColorOpacity(uint8_t& aOpacity);
>    bool ParseColorOpacity(float& aOpacity);
> +  // The ParseColorOpacity methods below attempt to parse an optional separator
> +  // and the opacity (alpha) component with the close parenthesis inside of CSS

My mistake, sorry -- "optional separator and the opacity" isn't precise enough (and that's entirely my fault, since this is taken from my suggested comment-text in comment 33).  That phrase sounds like it's saying *only* the separator is optional.  But really it's the separator+opacity chunk as a whole that's optional.

Maybe replace with this wording:
  // The ParseColorOpacity methods below attempt to parse an optional
  // [ separator <alpha-value> ] expression, followed by a close-parenthesis,
  // at the end of a css color function (e.g. "rgba()" and "hsla()").

@@ +1176,5 @@
>    bool ParseColorOpacity(float& aOpacity);
> +  // The ParseColorOpacity methods below attempt to parse an optional separator
> +  // and the opacity (alpha) component with the close parenthesis inside of CSS
> +  // color functions (e.g. "rgba()", and "hsla()"). The range of opacity
> +  // component is [0, 255]. If there is no opacity component, the 'aHasOpacity'

This statement about "The range of opacity component" ONLY applies to the first of the methods below -- but that's confusing/unclear here, since this is immediately following a sentence about "The ParseColorOpacity methods below". [plural]

Please could clarify this by:
 - adding a linebreak before "The range" (to separate it better from the higher-level comment about "methods below")
 - Begin the sentence with "For this first version, the range..." to make it extra-clear that you're now specifically documenting the first of the methods, rather than the methods as a group.

@@ +1177,5 @@
> +  // The ParseColorOpacity methods below attempt to parse an optional separator
> +  // and the opacity (alpha) component with the close parenthesis inside of CSS
> +  // color functions (e.g. "rgba()", and "hsla()"). The range of opacity
> +  // component is [0, 255]. If there is no opacity component, the 'aHasOpacity'
> +  // will be true and the 'aOpacity' will be 255.

Typo: s/true/false/

Also: even with that fixed, it's unclear whether this last sentence is talking about the expected initial vs. final state of these args.  (Right now it says "If [condition], [arg] will be true", which could be misunderstood as a requirement on what values the caller should be passing in these args.)

Please replace this last sentence with the following (or something like it):
If this function simply encounters a close-paren (without any [separator <alpha-value>]), this function will still succeed (i.e. return true), with outparam 'aHasOpacity' set to false and 'aOpacity' set to 255.

@@ +1188,5 @@
> +  bool ParseColorOpacityAndCloseParen(bool& aHasOpacity,
> +                                      float& aOpacity,
> +                                      char aSeparator);
> +
> +  // Parse a <number> color component. The range of color component is [0, 255].

Add: "If |aSeparator| is provided, this function will also attempt to parse that character after parsing the color component."

(Otherwise it's unclear what aSeparator means, and especially unclear whether the function expects to find it before or after the color-component.  This is particularly important to document, since it's the *reverse* of the ParseColorOpacityAndCloseParen() functions above, which expect the separator *before* the opacity instead of after.)

@@ +6707,4 @@
>          if (GetToken(true)) {
>            UngetToken();
>          }
> +        if (mToken.mType == eCSSToken_Number) { // number

Please change the comment to // <number> here (with <> CSS syntax), to make the point of this comment a little clearer (you're trying to tie this case to the broader comment above with the CSS syntax). Otherwise, this "_Number ... // number" looks a bit like unnecessary/excessive documentation. :)

Same goes for the "} else { // percentage" line further down -- make that <percentage>

@@ +6711,5 @@
>            uint8_t r, g, b, a;
> +          bool hasOpacity;
> +          if (ParseRGBColor(r, g, b, a, hasOpacity)) {
> +            if (hasOpacity) {
> +              aValue.SetIntegerColorValue(NS_RGBA(r, g, b, a),

I think we should get rid of this "hasOpacity" check -- see my note below for the similar percentage case.

@@ +6723,4 @@
>            float r, g, b, a;
> +          bool hasOpacity;
> +          if (ParseRGBColor(r, g, b, a, hasOpacity)) {
> +            if (hasOpacity) {

RE your notes on this from comment 48, about how our serialization code cares about whether opacity was specified -- I think our serialization code is *really* trying to care about "did the author use rgb() syntax vs rgba syntax", since those functions were different (until now!!!) and we didn't want to arbitrarily switch between them.

But now, rgb() and rgba() are aliases of each other! And when we get to this point in the parsing code, we actually don't know which function the author provided, since this code is now inside of a "is mToken rgb or rgba" check!

And particular: with this patch right now, it seems to me like the serializer *would* arbitrarily expand the specified value "rgb(1,2,3,0.5)" to use "rgba" as the function-name, and it would arbitrarily shorten "rgba(1,2,3)" to use "rgb" as the function-name.  That doesn't make sense in a modern world where rgb and rgba are aliases. If we're embracing css-color-4, we should just treat rgb() as the correct & compact form, and relegate rgba() to being a legacy thing, as the spec says it is.  And since this new function supports opacity being optional, there's also no reason we need to preserve opacity being specified vs. unspecified -- we should simply fix the serializer to omit the "A" component for fully-opaque colors, IMO [for brevity].

SO: I don't think we need to make this "hasOpacity" distinction.  We should just call aValue.SetFloatColorValue(r, g, b, a, eCSSUnit_PercentageRGBAColor) here unconditionally.  (Unless there's a spec-mandated thing that I'm missing).

FOLLOWUP THOUGHT: Maybe that also means we can get rid of the "hasOpacity" arg completely from all of this patch's functions...? Since, I think this is the only place where we care about it...

Comment 53

[Daniel Holbert [:dholbert]]

Comment on attachment 8785309 [details] [diff] [review]
P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v2

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

A few more review comments on the latest "P4" patch:

::: layout/style/nsCSSParser.cpp
@@ +6833,5 @@
> +  // rgb() = rgb( component{3} [ / <alpha-value> ]? )
> +  // the expression with comma:
> +  // rgb() = rgb( component#{3} , <alpha-value>? )
> +  const char separator = ',';
> +  const char nonCommaExpressionAlphaSeparator = '/';

"separator" is too generic of a variable name for ',' here.  Let's call this "commaSeparator" to be clearer.  (In other helper functions, we have the arg "aSeparator" whose naming makes sense, since it might be several different characters and/or might not even be present. But in this function, we know exactly what this local-variable is, so we might as well be clear about it.)

Also, let's just get rid of the "nonCommaExpressionAlphaSeparator" variable entirely, since that's only used once now, and it's usage isn't until much further down. (Ideally, '/' should end up being folded into a conditionally-defined variable called something like "separatorBeforeAlpha" which is either ',' or '/' -- see my comment further down with more details on that.)

@@ +6840,5 @@
> +  if (!ParseColorComponent(aR, Nothing())) {
> +    return false;
> +  }
> +  // Look for the separator after "r" component to determinate the expression
> +  // is separator-less or not.

Three changes:
 (1) s/the separator/a comma separator/
 (2) s/separator-less/comma-less/

(That makes it a bit more concrete/clear what we're actually doing.  It's also more accurate, because the term "separator-less" isn't actually correct here -- even if we don't find a comma, we might not be "separator-less" because we may find a "/" separator later on.)

 (3) Grammar fix: s/determine the/determine if the/

@@ +6841,5 @@
> +    return false;
> +  }
> +  // Look for the separator after "r" component to determinate the expression
> +  // is separator-less or not.
> +  bool hasSeparator = ExpectSymbol(separator, true);

s/hasSeparator/hasComma/  (or hasCommaSeparator, but that might make some of your lines too long further down)

@@ +6850,5 @@
> +  if (ParseColorComponent(aG, hasSeparator ? Some(separator) : Nothing()) &&
> +      ParseColorComponent(aB, Nothing()) &&
> +      ParseColorOpacityAndCloseParen(aHasOpacity,
> +                                     aA,
> +                                     hasSeparator ? separator : nonCommaExpressionAlphaSeparator)) {

This line is much too long. Let's just pull the ternary expression out into a local variable, defined just before this "if" check -- something like:

  const char separatorBeforeAlpha = hasComma ? commaSeparator : '/';

...and then just pass separatorBeforeAlpha as the 3rd arg in this call here.

@@ +6873,5 @@
>    }
>  
>    float value = mToken.mNumber;
> +
> +  if (aSeparator && !ExpectSymbol(aSeparator.value(), true)) {

s/aSeparator.value()/*aSeparator/

Please make that change throughout this patch.

(Maybe<> values are expected to feel "pointer-like" and to be treated semantically like pointers -- e.g. checked for truthiness to see if they're valid [as you already do here], & dereferenced with "*" [as you should do here].  That's preferred over the longhand .value() accessor.)

@@ +6874,5 @@
>  
>    float value = mToken.mNumber;
> +
> +  if (aSeparator && !ExpectSymbol(aSeparator.value(), true)) {
> +    REPORT_UNEXPECTED_TOKEN(aSeparator.value());

REPORT_UNEXPECTED_TOKEN's first parameter is supposed to be the name of an error-message -- and aSeparator is not the name of an error message.

I think you should be calling it like so:
  REPORT_UNEXPECTED_TOKEN(PEColorComponentBadTerm, *aSeparator);

@@ +6882,2 @@
>    if (value < 0.0f) value = 0.0f;
>    if (value > 255.0f) value = 255.0f;

(The css-color-4 spec says that clamping here in the parser (to the 0...255 range).  No need to change that here, though -- we can fix that elsewhere. bug 1295107 sort of covers this, though its patches only work for percentages right now. I've just requested in bug 1295107 comment 17 that we file a followup that lets us parse out-of-range numeric color-components as well.)

@@ +6902,5 @@
>  
>    float value = mToken.mNumber;
> +
> +  if (aSeparator && !ExpectSymbol(aSeparator.value(), true)) {
> +    REPORT_UNEXPECTED_TOKEN(aSeparator.value());

As above, this should be
  REPORT_UNEXPECTED_TOKEN(PEColorComponentBadTerm, *aSeparator);

@@ +7017,5 @@
> +
> +  uint8_t value = nsStyleUtil::FloatToColorComponent(floatOpacity);
> +  // Need to compare to something slightly larger
> +  // than 0.5 due to floating point inaccuracies.
> +  NS_ASSERTION(fabs(255.0f*mToken.mNumber - value) <= 0.51f,

Two things (both of which are bugs in the old code that this is copypasted from, which you might as well clean up in this new version):
 (1) Please add spaces around "*".
 (2) s/mToken.mNumber/floatOpacity/

@@ +7060,5 @@
> +CSSParserImpl::ParseColorOpacityAndCloseParen(bool& aHasOpacity,
> +                                              float& aOpacity,
> +                                              char aSeparator)
> +{
> +  if (ExpectSymbol(')', true)) {

Please add an explanatory comment to this early return (since it's not obvious from context/naming that this stuff is optional).

e.g.
    // Optional [separator <alpha-value>] was omitted. Default to 1 & succeed.

Comment 54

[Daniel Holbert [:dholbert]]

Comment on attachment 8785312 [details] [diff] [review]
P5. change CSS color function hsl() to be an alias of hsla() and support css-color-4 comma-less expression. v2

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

r- on p5 for now -- my above comments from P4 about the following topics all apply to this patch as well:
 - do we need to care about aHasOpacity/hasOpacity at all (probably not)
 - "separator"-related variable renamings/collapsings
 - pulling the "hasSeparator" (or "hasComma") ternary expression out into a local variable.

I'll take a closer look once you've addressed these. Thanks!

Comment 55

[Daniel Holbert [:dholbert]]

Comment on attachment 8785315 [details] [diff] [review]
P7. reftest for CSS color functions. v1

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

A few high-level comments on the reftest patch:

(1) For most of the syntax in this test, you should also add the new values to the "other_values" list for the "color" property, here:
  https://dxr.mozilla.org/mozilla-central/rev/26e22af660e543ebb69930f082188b69ec756185/layout/style/test/property_database.js#2774
That's where we aim to list all the valid formulations of a given type of value (including interesting edge cases).  On top of that, we can test the *rendering* in reftests, but we should also verify that the values at least parse correctly, and property_database.js is how we do that. [via mochitests that use property_database.js]

(2) Commit message is currently: "Bug 1295456 - reftest for CSS color functions".  That's not specific enough -- it should say "reftests" (plural) and should mention "css-color-4's changes to color functions", since that's really what it's testing.  And if you add to property_database.js (as I suggest above), you could shorten "reftests" to just "tests" to be broader.
So, this should perhaps end up saying: "Bug 1295456 - add tests for css-color-4's changes to color functions"

::: layout/reftests/backgrounds/background-color-hsla-ref.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <style>
> +      #p1 {background-color:hsl(120,100%,50%,0.2);}

General formatting note:
 (1) please add spaces around the ":" and inside the {} curly-braces, in all of the files in this patch, for better readability.
 (2) Ideally, add a space after each comma in the hsl() expressions, too (at least in the reference cases, where you're not trying to trigger edge cases), also for readability.  (you have spaces after commas in a few of the styles but not most of them).
 (3) Please add "-1" to the test filenames here. (We generally include a number in reftest names, foo-1.html/foo-1-ref.html, so that we can easily add additional flavors of the test down the line without having to be super-creative about the naming of those new tests.)

@@ +6,5 @@
> +      #p2 {background-color:hsl(120/**/,100%,/**/75%,0.4);}
> +      #p3 {background-color:hsl(120,100%,25%,0.8);}
> +      #p4 {background-color:hsl(120 60% 70% / 1.0);}
> +      #p5 {background-color:hsl(290 100%/**/50%);}
> +      #p6 {background-color:hsl(290, 60% ,70%,1.0);}

This CSS isn't appropriate for a *reference case* for several reasons:

 (1) Ideally, we want reftests to *fail* if they're testing for a feature & the feature isn't implemented.  So, ideally the reftest's *reference case* should avoid using/exercising the new feature at all.  The *testcase* should depend on the new feature, but if possible the reference case should render the same regardless of whether the new feature is implemented.  (That way, it'll reliably fail -- with the testcase rendering incorrectly and the reference case showing the correct expected rendering -- in a build without the feature implemented).  SO: this reference case should not use *any* of the fancy new CSS-color-4 parsing features -- no hsl()-with-4-components, no comma-less syntax.  Please make this just use good ol' legacy hsl/hsla() color expressions with commas, and use hsl() if there are 3 components (in #p5) and hsla(...) if there are 4 components (all the others, I think).

 (2) Reference cases aren't meant to exercise edge cases -- that's what the testcase is for.  (If we fail to correctly handle the edge case, we still would like the reference case to show the expected rendering.) So, we shouldn't be including funky syntax that tries to trigger edge-case behavior here. SO: please drop the mid-color-function /**/ comments from this file, and drop the space-before-comma in #p6 ("60% ,")


I notice you're using hsla() exclusively in the testcase, and hsl() exclusively in the reference case. (which is part of why the testcase is relying on hsl()-with-4-components)  It is indeed useful to test that these are aliases, but the best way to do that would be to create *two testcases*, e.g. background-color-hsla-1a.html and background-color-hsla-1b.html, where one testcase exlusively uses hsl() and the other exclusively uses hsla(). And then you'd compare them both against the reference case, which uses legacy-friendly syntax (hsl with 3 components, hsla with 4 components) to reliably produce an expected rendering.

::: layout/reftests/backgrounds/background-color-rgba-ref.html
@@ +6,5 @@
> +#p2 { background-color:rgb(0 255 0 / 40%); }
> +#p3 { background-color:rgb(0/*test comment*/0/*test comment*/255/80%); }
> +#p4 { background-color:rgb(192,/*test comment*/192,/*test comment*/192,/*test comment*/100%); }
> +#p5 { background-color:rgb(255 /*test comment*/ 255 /*test comment*/ 0); }
> +#p6 { background-color:rgb(255,0,255,100%); }

All my comments for the hsla reftest apply to this reftest as well.

Comment 56

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #55)
> Comment on attachment 8785315 [details] [diff] [review]
> P7. reftest for CSS color functions. v1
[...]
> I notice you're using hsla() exclusively in the testcase, and
> hsl() exclusively in the reference case. (which is part of
> why the testcase is relying on hsl()-with-4-components)

Sorry, I meant to say "why the reference case is relying" there (not "the testcase").  Anyway, hopefully you get the point. :)

Comment 57

[Daniel Holbert [:dholbert]]

Also: we'd like for these reftests to be submittable to the CSS Working group [to benefit other browsers & sanity-check our assumptions], which means:
 (1) they should live in a new subdirectory inside of layout/reftests/w3c-css/submitted, called "css-color-4"
 (2) they need a bit more metadata in each testcase -- in particular, each testcase-file should have <link rel="match"> to (redundantly) provide the name of the reference file, and also each file should have <link>s for "author" (you) and "help" (to point at the relevant chunk of spec text)  See other testcases inside of that w3c-css/submitted subtree for examples, and see documentation in the "reftest" section at http://testthewebforward.org/

Comment 58

[Daniel Holbert [:dholbert]]

Oh, and also -- the reftest files should really be named like "foo-001.html" (or "foo-001a.html") with a 3-digit number (*not* like "foo-1.html" as I initially suggested), because the CSSWG uses 3-digit numbering for their reftest suite.

Comment 59

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3. support percentage opacity value in CSS color functions. v3

add PEExpectedPercentOrNumber message in css.properties file.

Comment 60

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v3

remove the aHasOpacity/hasOpacity checking in this patch.
rename separator variables.
put separator-before-alpha ternary expression into a local variable.

Comment 61

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5. change CSS color function hsl() to be an alias of hsla() and support css-color-4 comma-less expression. v3

remove the aHasOpacity/hasOpacity checking in this patch.
rename separator variables.
put separator-before-alpha ternary expression into a local variable.

Comment 62

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6. remove now-unused single-arg ParseColorOpacity functions from nsCSSParser. v3. r=dholbert

rebase.

Comment 63

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8787064 [details] [diff] [review]
P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v3

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

::: layout/style/nsCSSParser.cpp
@@ +6887,1 @@
>    if (value < 0.0f) value = 0.0f;

I still don't remove the clamping for both <number> and <percentage> parsing.

Since we use uint8_t here, it can't contain all the result of <number> from "aComponent = NSToIntRound(value)" without clamping.

If we want to extend the value of eCSSUnit_PercentageRGBAColor/eCSSUnit_PercentageRGBColor and eCSSUnit_RGBColor/eCSSUnit_RGBAColor, maybe we could remove these clamping in bug 1295107 and bug 1299310

@@ +6915,1 @@
>    if (value < 0.0f) value = 0.0f;

If we dont't do the clamping, we will have problem in
  uint8_t value = nsStyleUtil::FloatToColorComponent(floatOpacity);
in ParseColorOpacityAndCloseParen().

Comment 64

[Daniel Holbert [:dholbert]]

Comment on attachment 8787060 [details] [diff] [review]
P3. support percentage opacity value in CSS color functions. v3

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

r=me on this part, with percent/number swapped in the warning name & text.

::: dom/locales/en-US/chrome/layout/css.properties
@@ +97,5 @@
>  PEColorNotColor=Expected color but found ‘%1$S’.
>  PEColorComponentEOF=color component
>  PEExpectedPercent=Expected a percentage but found ‘%1$S’.
>  PEExpectedInt=Expected an integer but found ‘%1$S’.
> +PEExpectedPercentOrNumber=Expected a percentage or a number but found ‘%1$S’.

Please swap the ordering of "number" and "percentage" in the name as well as the prose of this warning. So:
  s/PercentOrNumber/NumberOrPercent/
  s/percentage or a number/number or a percentage/

That ordering is better, because:
 (1) then it'll match the ordering on the immediately subsequent line, in the prose of PEColorBadRGBContents. (which says "number or percentage").  We might as well use that same ordering for easier comparison/translation.)
 (2) then it'll match the ordering in the spec for <alpha-value> (the sole reason this warning exists, right now): https://drafts.csswg.org/css-color/#rgb-functions
 (3) then it'll match the comment/code where we actually check for whether we should post the warning (in nsCSSParser.cpp) - the comment/code at that usage-site mentions number before percent.

::: layout/style/nsCSSParser.cpp
@@ +6972,5 @@
>    }
>  
> +  // eCSSToken_Number or eCSSToken_Percentage.
> +  if (mToken.mType != eCSSToken_Number && mToken.mType != eCSSToken_Percentage) {
> +    REPORT_UNEXPECTED_TOKEN(PEExpectedPercentOrNumber);

(You'll need to use the new name for this warning here, too, with Number & Percent swapped.)

Comment 65

[Daniel Holbert [:dholbert]]

Comment on attachment 8787064 [details] [diff] [review]
P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v3

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

::: layout/style/nsCSSParser.cpp
@@ +1181,5 @@
>    // ParseColorOpacity will enforce that the color ends with a ')'
>    // after the opacity
>    bool ParseColorOpacity(uint8_t& aOpacity);
>    bool ParseColorOpacity(float& aOpacity);
> +  // The ParseColorOpacity methods below attempt to parse an optional

This comment isn't using the correct function-names.

s/ParseColorOpacity/ParseColorOpacityAndCloseParen/

@@ +1184,5 @@
>    bool ParseColorOpacity(float& aOpacity);
> +  // The ParseColorOpacity methods below attempt to parse an optional
> +  // [ separator <alpha-value> ] expression, followed by a close-parenthesis,
> +  // at the end of a css color function (e.g. "rgba()" or "hsla()"). If these
> +  // functions simply encounters a close-parenthesis (without any

s/encounters/encounter/

@@ +1186,5 @@
> +  // [ separator <alpha-value> ] expression, followed by a close-parenthesis,
> +  // at the end of a css color function (e.g. "rgba()" or "hsla()"). If these
> +  // functions simply encounters a close-parenthesis (without any
> +  // [separator <alpha-value>]), they will still succeed (i.e. return true),
> +  // with outparam 'aOpacity' set to an default opacity value. The default

s/an/a/
s/default opacity value/default (fully-opaque) opacity value/ -- otherwise it kinda sounds like this is saying one function might default to opaque and another might default to transparent or something like that.  (Your later comments do clarify the defaults, but we might as well just make it clearer what we mean here in this first sentence.)

@@ +6845,5 @@
> +  // Parse R.
> +  if (!ParseColorComponent(aR, Nothing())) {
> +    return false;
> +  }
> +  // Look for a comma separator after "r" component to determinate if the

Typo: s/determinate/determine/

@@ +6849,5 @@
> +  // Look for a comma separator after "r" component to determinate if the
> +  // expression is comma-less or not.
> +  bool hasComma = ExpectSymbol(commaSeparator, true);
> +
> +  // Start to parse G, B and A.

s/Start to parse/Parse/

("Start to" doesn't really makes sense here -- we don't start parsing G and then finish parsing G later on. We just parse it. And then we parse B. Etc.)

@@ +6870,5 @@
>      REPORT_UNEXPECTED_EOF(PEColorComponentEOF);
>      return false;
>    }
>  
> +  if (mToken.mType != eCSSToken_Number) {

When making this change (dropping a "mIntegerValid" check for the 0-255 version of the color-parsing function), you need to also change the error message on the next line. (currently "PEExpectedInt", which isn't the correct error message here, if we're not requiring valid integer values anymore)

I think it wants to be PEExpectedNumber.

@@ +7063,5 @@
> +                                              char aSeparator)
> +{
> +  if (ExpectSymbol(')', true)) {
> +    // The optional [separator <alpha-value>] was omitted, so set the opacity
> +    // to default value '1.0f' and return succeed.

"return succeed" isn't grammatically correct.

This should just say: "...and succeed."  Or alternately, "...and return success."

@@ +7094,5 @@
> +  if (mToken.mNumber < 0.0f) {
> +    mToken.mNumber = 0.0f;
> +  } else if (mToken.mNumber > 1.0f) {
> +    mToken.mNumber = 1.0f;
> +  }

So, about half of this function is copypasted directly from the old ParseColorOpacity function -- which is good, except that all the existing ParseColorOpacity code is about to get removed in a later patch.  So, someone who wants to trace the history of this code (e.g. to learn about this clamping or the error messages) would hit a *dead-end* when they reach this patch.  Really, we'd like them to be able to follow the history back through the old ParseColorOpacity version.

To avoid that, I'd like for these new ParseColorOpacityAndCloseParen functions be represented as *minor in-place rewritings* of the existing ParseColorOpacity functionality, with unmodified code left intact, rather than as a copypaste-with-edits.  I know the existing ParseColorOpacity functions can't entirely be replaced at this point in the series, BUT I think we can get pretty close.

SO: Specifically, please make the following minor change, which should address my concerns here I think: 
- In this patch, move the old soon-to-be-deleted ParseColorOpacity() implementatoins (with no changes) somewhere *far away* (perhaps to the very bottom of this file), and add a comment like "// XXX these will be removed by a later patch in this series" right above them.

Then, hg should represent this new functionality here as *incrementally modifying the existing ParseColorOpacity* (plus adding some duplicated soon-to-be-deleted code added at the bottom of the file).  And that's much more useful for review & hg-archeology purposes, I think.

This will require you to regenerate P6, but that should be easy enough since it's just deleting code.

Comment 66

[Daniel Holbert [:dholbert]]

Comment on attachment 8787067 [details] [diff] [review]
P5. change CSS color function hsl() to be an alias of hsla() and support css-color-4 comma-less expression. v3

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

r=me with nits addressed, though note that we'll need another patch here for the <angle>-valued hue thing, as noted in my first review comment below. (And this probably shouldn't land until we've got that patch as well)

::: layout/style/nsCSSParser.cpp
@@ +6926,5 @@
>      REPORT_UNEXPECTED_EOF(PEColorHueEOF);
>      return false;
>    }
>    if (mToken.mType != eCSSToken_Number) {
>      REPORT_UNEXPECTED_TOKEN(PEExpectedNumber);

This expectation (requiring a <number> for the <hue>) isn't correct anymore, per documentation that you're adding further up in this patch:
  <hue> = <number> | <angle>

Maybe drop a TODO/XXX comment here to clarify that that's missing, and add another patch in this stack to address the TODO & add support for <angle>-valued hues? (with units, e.g. "180deg" or "0.5turn" instead of just "180".

(If we had an angle here, I think mToken.mType would be "eCSSToken_Dimension", and then you'd need to check the unit, like we do elsewhere for angles; maybe sharing some other angle-parsing code if possible.)

@@ +6936,4 @@
>    // hue values are wraparound
> +  aHue = aHue - floor(aHue);
> +  // Look for a comma separator after "hue" component to determinate if the
> +  // expression is comma-less or not.

Typo: s/determinate/determine/ (as noted for the rgb version in comment 65)

@@ +6938,5 @@
> +  // Look for a comma separator after "hue" component to determinate if the
> +  // expression is comma-less or not.
> +  bool hasComma = ExpectSymbol(commaSeparator, true);
> +
> +  // Start to parse saturation, lightness and opacity. The saturation and

s/Start to parse/Parse/ (as noted for the rgb version in comment 65)

@@ +6939,5 @@
> +  // expression is comma-less or not.
> +  bool hasComma = ExpectSymbol(commaSeparator, true);
> +
> +  // Start to parse saturation, lightness and opacity. The saturation and
> +  // lightness are also <percentage>, so reuse ParseColorComponent function for

Two nits:
 (1) I think "also" is an extraneous word here -- let's drop that word.
(It doesn't really make sense to me, in this context -- it sounds like it's implying you were just talking about some other thing that took a percentage, but you weren't.)

 (2) s/ParseColorComponent/the float version of ParseColorComponent/
(since that's the version that parses specifically <percentage> values)

Comment 67

[Daniel Holbert [:dholbert]]

Comment on attachment 8787064 [details] [diff] [review]
P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v3

(P4 is almost r+, but I'll mark it r- for now since it's probably worth one final look-over after you've addressed comment 65, particularly the last part of comment 65 about ParseColorOpacity evolution.)

Comment 68

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P3: support percentage opacity value in CSS color functions. v4. r=dholbert

update for comment 64.
s/PercentOrNumber/NumberOrPercent/

Comment 69

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v4

update for comment 65 especially for ParseColorOpacity function.

Comment 70

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5. change CSS color function hsl() to be an alias of hsla() and support css-color-4 comma-less expression. v4. r=dholbert

update for comment 66.
<angle> value parsing will at next patch.

Comment 71

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5-1. support <angle> value for hue component in CSS hsl() color function. v1

support <angle> value for hue.

Comment 72

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P6. remove now-unused single-arg ParseColorOpacity functions from nsCSSParser. v4. r=dholbert

rebase.

Comment 73

[Daniel Holbert [:dholbert]]

Comment on attachment 8790084 [details] [diff] [review]
P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v4

> bool
>-CSSParserImpl::ParseColorOpacity(float& aOpacity)
>+CSSParserImpl::ParseColorOpacityAndCloseParen(float& aOpacity,
>+                                              char aSeparator)
> {
[...]
> 
>   // eCSSToken_Number or eCSSToken_Percentage.
>   if (mToken.mType != eCSSToken_Number && mToken.mType != eCSSToken_Percentage) {
>-    REPORT_UNEXPECTED_TOKEN(PEExpectedNumberOrPercent);
>+    REPORT_UNEXPECTED_TOKEN(PEExpectedPercentOrNumber);

I think this change (tweaking the label for this error message) is a mistake. It looks like PEExpectedNumberOrPercent (the version being removed here) is the correct name for this now, per the latest P3.

(I expect this line won't compile, as a result.)

Comment 74

[Daniel Holbert [:dholbert]]

Comment on attachment 8790084 [details] [diff] [review]
P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v4

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

r=me with comment 73 addressed (that one-liner PEExpectedNumberOrPercent mistaken tweak reverted) and the following:

::: layout/style/nsCSSParser.cpp
@@ +1188,5 @@
> +  // "hsla()"). If these functions simply encounter a close-parenthesis (without
> +  // any [separator <alpha-value>]), they will still succeed (i.e. return true),
> +  // with outparam 'aOpacity' set to a default opacity value (fully-opaque). The
> +  // default opacity value is different for each function and will be shown
> +  // below.

Nit: I think this would be clearer if you just dropped the final sentence here (about the default opacity being different for each function). 

The per-function comments (below this) are now sufficiently clear about what the defaults are for each function (and that they mean the same thing).

Comment 75

[Daniel Holbert [:dholbert]]

Comment on attachment 8790086 [details] [diff] [review]
P5-1. support <angle> value for hue component in CSS hsl() color function. v1

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

Thanks for adding this patch! This is nearly there.

Firstly: please add some tests for this new <angle>-in-hsl() functionality.  As noted in comment 55, we need some new values that exercise this syntax (with several angular units, e.g. "deg" and "turn", in property_database.js)  Those tests could come in P7 rather than in this patch, but they need to exist somewhere in this bug.

::: dom/locales/en-US/chrome/layout/css.properties
@@ +98,5 @@
>  PEColorComponentEOF=color component
>  PEExpectedPercent=Expected a percentage but found ‘%1$S’.
>  PEExpectedInt=Expected an integer but found ‘%1$S’.
>  PEExpectedNumberOrPercent=Expected a number or a percentage but found ‘%1$S’.
> +PEExpectedNumberOrAngle=Expected a number or a angle but found ‘%1$S’.

Nit: missing an "n":
s/a angle/an angle/

::: layout/style/nsCSSParser.cpp
@@ +1205,5 @@
>    // Similar to the previous one, but parse a <percentage> color component.
>    // The range of color component is [0.0f, 1.0f].
>    bool ParseColorComponent(float& aComponent, Maybe<char> aSeparator);
>  
> +  // Parse a <number> or a <angle> component. The unit of outparam 'aAngle' is

Replace first sentence here with:
// Parse a CSS <hue> = <number> | <angle>.

(since I think this function should be renamed, as noted below)

@@ +1207,5 @@
>    bool ParseColorComponent(float& aComponent, Maybe<char> aSeparator);
>  
> +  // Parse a <number> or a <angle> component. The unit of outparam 'aAngle' is
> +  // degree. Assume the unit to be degree if an unitless <number> is provided.
> +  bool ParseNumberOrAngle(float &aAngle);

Nits:
(1) shift the "&" left.
(2) Let's name this "ParseHue", since this function exactly parses a CSS <hue> value (according to how <hue> is defined, at https://drafts.csswg.org/css-color-4/#the-hsl-notation )  (This also will let us more justifiably shift an error message inside the function, as noted below)

@@ +6880,5 @@
>    return true;
>  }
>  
> +bool
> +CSSParserImpl::ParseNumberOrAngle(float &aAngle)

As above:
(1) shift the "&" left.
(2) Let's name this "ParseHue".

@@ +6885,5 @@
> +{
> +  // <number>
> +  if (!GetToken(true)) {
> +    return false;
> +  }

...and let's shift the unexpected EOF line (and REPORT_UNEXPECTED_EOF(PEColorHueEOF)) from the caller here.

We can more justifiably use "PEColorHueEOF" inside this function if it's renamed to ParseHue (per above).

(And right now, the repeated GetToken() calls (one in the caller & one here) are a bit redundant & awkward.)

@@ +6889,5 @@
> +  }
> +  if (mToken.mType == eCSSToken_Number) {
> +    aAngle = mToken.mNumber;
> +
> +    return true;

Drop the blank line before "return true" here.

@@ +6891,5 @@
> +    aAngle = mToken.mNumber;
> +
> +    return true;
> +  } else {
> +    UngetToken();

Don't use "else after return", per coding style guide.

Just drop the else{ ... } wrapper, and deindent UngetToken().

@@ +6901,5 @@
> +  // instead of VARIANT_ANGLE_OR_ZERO.
> +  if (ParseSingleTokenVariant(angleValue, VARIANT_ANGLE, nullptr)) {
> +    aAngle = angleValue.GetAngleValueInDegrees();
> +
> +    return true;

As above, drop the blank line before "return true" here.  It feels awkward to have the 2 lines of a 2-line "if" block separated by a blank line. (particularly when they're related -- setting the returned-by-value value & then the directly-returned value)

@@ +6916,5 @@
> +  if (!GetToken(true)) {
> +    REPORT_UNEXPECTED_EOF(PEColorHueEOF);
> +    return false;
> +  }
> +  UngetToken();

Let's drop this block entirely, and shift the REPORT_UNEXPECTED_EOF(PEColorHueEOF) into ParseHue (formerly ParseNumberOrAngle), as suggested above.

@@ +6927,5 @@
>    const char commaSeparator = ',';
>  
>    // Parse hue.
>    // <hue> = <number> | <angle>
> +  float degreeAngle = 0.0f;

Drop the default-initialization here -- this default value gets overwritten before we use this variable, in all cases.

::: layout/style/nsCSSValue.cpp
@@ +333,5 @@
>      return 0.0;
>    }
>  }
>  
> +double nsCSSValue::GetAngleValueInDegrees() const

newline after "double"

@@ +338,5 @@
> +{
> +  double angle = GetFloatValue();
> +
> +  switch (GetUnit()) {
> +  case eCSSUnit_Degree: return angle;

Please indent each "case:"/"default:" line by 2 spaces here. (should be indented further than the "switch")

(The contextual code doesn't do that, but the coding style guide requires it. Eventually we should fix up the surrounding code.)

Comment 76

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Daniel,

From comment 52, what's the expect output for getAuthoredPropertyValue() with the following color function?
a) rgb(10, 10, 10, 100%)
d) rgb(10, 10, 10, 0.5)
b) rgba(10, 10, 10)

Should all of them become rgb(r,g,b,a) and omit the 'a' component if 'a' is full opaque[1]?
hsl color function also has the same problem.

I got some mochitests failed as:
182 INFO TEST-UNEXPECTED-FAIL | layout/style/test/chrome/test_author_specified_style.html | specified rgb(10,20,30) - got "rgba(10, 20, 30, 1)", expected "rgb(10, 20, 30)"
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:270:5

[1] The css color-value serializer:
https://dxr.mozilla.org/mozilla-central/rev/bef797e6c129249a0ac331be21e04487f2719fc5/layout/style/nsCSSValue.cpp#1519

Comment 77

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [wip] P7. add tests for css-color-4 color function changes. v2

Comment 78

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P4. change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. v5. r=dholbert

update for comment 74

Comment 79

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5-1. support <angle> value for hue component in CSS hsl() color function. v2

rename ParseNumberOrAngle function to ParseHue.
move hue unexpected EOF line checking into ParseHue function.
update indent.

Comment 80

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8790200 [details] [diff] [review]
P5-1. support <angle> value for hue component in CSS hsl() color function. v2

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

::: layout/style/nsCSSValue.cpp
@@ +318,5 @@
>    return false;
>  }
>  
> +double
> +nsCSSValue::GetAngleValueInRadians() const

This function is not related to this bug. Just fix the indent.

Comment 81

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8790200 [details] [diff] [review]
P5-1. support <angle> value for hue component in CSS hsl() color function. v2

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

::: layout/style/nsCSSParser.cpp
@@ +6894,5 @@
> +    return true;
> +  }
> +  UngetToken();
> +
> +  // <angle>

The testing of <angle> value will at P7 patch.

Comment 82

[Daniel Holbert [:dholbert]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #76)
> From comment 52, what's the expect output for getAuthoredPropertyValue()
> with the following color function?
[...]
> Should all of them become rgb(r,g,b,a) and omit the 'a' component if 'a' is
> full opaque[1]?

Now that rgba() is officially a legacy alias of rgb(), that seems reasonable to me, yeah.

I wonder if we should just get rid of _RGBAColor entirely, then... I'm not sure it's valuable to care about whether the value was specified with rgb() vs rgba() now that they're aliases, particularly since _RGBAColor was added entirely so that we could distinguish between these two cases, back when rgb() and rgba() had different syntax.

(Side note: getAuthoredPropertyValue is a mozilla-internal thing, for use by devtools -- so I don't know that its behavior is specified anywhere beyond bug-comments in the bug that added it, e.g. bug 731271 comment 2 & bug 731271 comment 3.  heycam is perhaps the best authority for that, since he implemented that feature.)

Comment 83

[Daniel Holbert [:dholbert]]

Comment on attachment 8790200 [details] [diff] [review]
P5-1. support <angle> value for hue component in CSS hsl() color function. v2

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

r=me with the following:

::: dom/locales/en-US/chrome/layout/css.properties
@@ +98,5 @@
>  PEColorComponentEOF=color component
>  PEExpectedPercent=Expected a percentage but found ‘%1$S’.
>  PEExpectedInt=Expected an integer but found ‘%1$S’.
>  PEExpectedNumberOrPercent=Expected a number or a percentage but found ‘%1$S’.
> +PEExpectedNumberOrAngle=Expected a number or an angle but found ‘%1$S’.

This should be inserted 1 line further up, to be in correct alphabetical order.

(since "Angle" sorts before "Percent" alphabetically)

::: layout/style/nsCSSParser.cpp
@@ +1206,5 @@
>  
> +  // Parse a <hue> component.
> +  //   <hue> = <number> | <angle>
> +  // The unit of outparam 'aAngle' is degree. Assume the unit to be degree if an
> +  // unitless <number> is provided.

s/provided/parsed/

("provided" is a bit too vague -- it sounds like it might mean "passed into this function by the caller")

::: layout/style/nsCSSValue.cpp
@@ +332,5 @@
> +    default:
> +      MOZ_ASSERT(false, "unrecognized angular unit");
> +      return 0.0;
> +  }
> +}

Please don't mix substantial existing-code-reformatting in the same patch as actual-code-changes.  (A one-liner tweak in contextual code is OK here and there, but that's different from reindenting a whole function that's not otherwise being used/modified in the patch.)

I take it you want to reformat this (old/misindented) code before adding a nicer copy of it -- that's a noble goal, but it's still best to do the reformatting separately, because otherwise it obscures what's actually changing in this patch.

I'll push a one-off patch to do this reindenting-of-existing-code, and please revert these changes from this patch.

Comment 84

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #83)
> I'll push a one-off patch to do this reindenting-of-existing-code, and
> please revert these changes from this patch.

(For the record -- I pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b6ed05d041 as a "no bug" whitespace-only fixup for this, with you as the author (and with r=me), because I just copied the changes from your patch.

Comment 85

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Brian,
For css-color-4[1], rgb() and rgba(), and hsl() and hsla() are now aliases of each other (all of them have the same syntax).

What's the expect output of getAuthoredPropertyValue() with these following css rules?
1) rgb(1, 1, 1, 0.5)
2) rgb(1, 1, 1)
3) rgba(1, 1, 1, 0.5)
4) rgba(1, 1, 1)

From comment 76 and comment 82, What do you think about making all of outputs to become rgb(r,g,b,a) and omit the 'a' component if 'a' is full opaque?

[1]
https://drafts.csswg.org/css-color/#rgb-functions

Comment 86

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P5-1. support <angle> value for hue component in CSS hsl() color function. v3. r=dholbert

update for comment 83.

Comment 87

[Brian Grinstead [:bgrins]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #85)
> Hi Brian,
> For css-color-4[1], rgb() and rgba(), and hsl() and hsla() are now aliases
> of each other (all of them have the same syntax).
> 
> What's the expect output of getAuthoredPropertyValue() with these following
> css rules?
> 1) rgb(1, 1, 1, 0.5)
> 2) rgb(1, 1, 1)
> 3) rgba(1, 1, 1, 0.5)
> 4) rgba(1, 1, 1)
> 
> From comment 76 and comment 82, What do you think about making all of
> outputs to become rgb(r,g,b,a) and omit the 'a' component if 'a' is full
> opaque?
> 
> [1]
> https://drafts.csswg.org/css-color/#rgb-functions

I don't see any references to getAuthoredPropertyValue in devtools: https://dxr.mozilla.org/mozilla-central/search?q=getAuthoredPropertyValue.  I do see it used in firebug, though: https://dxr.mozilla.org/addons/search?q=getAuthoredPropertyValue.

Going to ni? Tom who might know if we are somehow using this in devtools and Honza about the usage within Firebug.

Comment 88

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #87)

> I don't see any references to getAuthoredPropertyValue in devtools:
> https://dxr.mozilla.org/mozilla-central/search?q=getAuthoredPropertyValue. 

> Going to ni? Tom who might know if we are somehow using this in devtools and
> Honza about the usage within Firebug.

We don't use it.  I wasn't aware of it when doing the as-authored
work, and anyway it wouldn't have helped, because it doesn't do anything
different from GetPropertyValue.

Comment 89

[Daniel Holbert [:dholbert]]

(In reply to Tom Tromey :tromey from comment #88)
> I wasn't aware of it when doing the as-authored
> work, and anyway it wouldn't have helped, because it doesn't do anything
> different from GetPropertyValue.

It does exactly one thing different, AFAICT -- it makes us serialize to "rgba(..., 1)" instead of "rgb(...)", if the author specified rgba(..., 1).

I'm not sure that's useful, though (particularly now that rgb/rgba will accept the same syntax and rgba is becoming a legacy thing).  So, I've filed bug 1302513 on deprecating and/or removing this API.

I'll move the needinfo=Jan Odvarko over to that bug, for his thoughts from a firebug perspective on the needs/usefulness of this API.

Comment 90

[Daniel Holbert [:dholbert]]

Comment on attachment 8790104 [details] [diff] [review]
[wip] P7. add tests for css-color-4 color function changes. v2

Looks like this test patch still needs finishing off.  (I notice it's still got WIP in the title)

A few nits for things I noticed when skimming it just now, which may or may not be things you're already aware of & planning on fixing:

>+++ b/layout/reftests/w3c-css/submitted/css-color-4/background-color-hsl-001-ref.html
>@@ -0,0 +1,33 @@
>+<!DOCTYPE html>
[...]
>+</html>
>\ No newline at end of file

Please make sure there's a newline character at the end of each file, so that you don't end up with that above-quoted "No newline" warning in your patch.  (There are a number of files with that warning in the patch right now.)

>+++ b/layout/reftests/w3c-css/submitted/css-color-4/background-color-hsl-003.html
[...]
>+    <link rel="match" href="background-color-hsl-002-ref.html" />

Copypaste typo -- this testcase, numbered 003, presumably should match 003-ref.html (not 002-ref.html)

>+++ b/layout/reftests/w3c-css/submitted/css-color-4/background-color-rgba-ref.html
>@@ -0,0 +1,23 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<style>
>+#p1 { background-color:rgb(255, 0, 0, 20%); }
>+#p2 { background-color:rgb(0 255 0 / 40%); }
>+#p3 { background-color:rgb(0/*test comment*/0/*test comment*/255/80%); }

This reference case and its corresponding testcase are missing the standard CSSWG title/author/etc. metadata tags (which you've got in the earlier testcases here).

Also, you still need to remove the CSS comments interspersed in the colors here (since reference cases aren't the place to exercise edge cases, as noted in comment 55)

>diff --git a/layout/reftests/w3c-css/submitted/css-color-4/reftest.list b/layout/reftests/w3c-css/submitted/css-color-4/reftest.list
>new file mode 100644
>index 0000000..96c8896
>--- /dev/null
>+++ b/layout/reftests/w3c-css/submitted/css-color-4/reftest.list
>@@ -0,0 +1 @@
>+#css-color-4 function

This reftest.list file shouldn't be empty. :) There are a bunch of tests in this directory (added in this patch) which should be listed here.

Comment 91

[Daniel Holbert [:dholbert]]

(Oh, also: the "rgba" reftest files still need a -001 suffix, too, to fit the CSSWG testsuite's requirements.)

Comment 92

[Tom Tromey :tromey]

We're going to need some devtools patches to account for the new syntax as well.
Could be a follow-up.  (I skimmed a couple of patches but didn't see anything -- my
apologies if I missed it.)

Comment 93

[Daniel Holbert [:dholbert]]

(Yeah, let's punt devtools patches to a follow-up bug -- would you mind filing?)

Comment 94

[Tom Tromey :tromey]

Filed.

Comment 95

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7. add tests for css-color-4 color function changes. v1

Comment 96

[Jerry Shih[:jerry] (UTC+8) (inactive)]

update attachment 8791946 [details] [diff] [review] for comment 90.

Comment 97

[Daniel Holbert [:dholbert]]

Comment on attachment 8791946 [details] [diff] [review]
P7. add tests for css-color-4 color function changes. v1

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

Sorry -- I just noticed the <title> and <meta name="assert"> don't really accurately describe what the testcase/reference-case pairs are testing.

(I think they did before comment 55, but they ended up less accurate after you updated the CSS to address that comment.)

I'm leaving comments below on rgb-001 and rgb-002, but I think these comments apply to the hsl tests as well.

::: layout/reftests/w3c-css/submitted/css-color-4/background-color-rgb-001.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <meta charset="utf-8">
> +    <title>CSS-Color-4: rgb() and rgba() are aliases of each other(test) </title>

This would be a valuable thing to test, but it's not actually what this test is testing.  (The reference case doesn't use "identical grammar" -- it uses the backgwards-compatible grammar -- which I think is appropriate, BTW.)

So:
 (1) maybe replace this title with "CSS-Color-4: rgb() syntax accepts alpha and non-integer components (and comments as separators)"

 (2) if you like, maybe create a second version of this test (so you'll have two testcases named -001a and -001b) which actually *does* use identical values and grammar, but with rgba() instead of rgb().  If these two testcases each match the (backwards-compatible) reference, then that would better-demonstrate that rgb() and rgba() are in fact aliases.

@@ +5,5 @@
> +    <title>CSS-Color-4: rgb() and rgba() are aliases of each other(test) </title>
> +    <link rel="author" title="Jerry Shih" href="mailto:[e-mail]" />
> +    <link rel="author" title="Mozilla" href="http://www.mozilla.org/" />
> +    <link rel="help" href="https://drafts.csswg.org/css-color/#rgb-functions" />
> +    <meta name="assert" content="rgb() should have the identical grammar and behavior to rgba()." />

As noted above, this isn't a good description of what's being tested here, because this test doesn't actually verify that rgb() & rgba() have "identical grammar". (The reference case uses different grammar -- as it should, to be a nice & simple & backwards-compatible reference case.)

So, this language needs a tweak. Maybe it should say:
  "rgb() accepts an alpha component and non-integer values (and treats CSS comments as token-separators)"

::: layout/reftests/w3c-css/submitted/css-color-4/background-color-rgb-002.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +  <head>
> +    <meta charset="utf-8">
> +    <title>CSS-Color-4: rgba() and rgb() are aliases of each other(test) </title>

My comments for -001 all apply here as well -- this testcase & its reference don't actually prove that rgba and rgb are aliases.

@@ +9,5 @@
> +    <meta name="assert" content="rgba() should have the identical grammar and behavior to rgb()." />
> +    <link rel="match" href="background-color-rgb-002-ref.html" />
> +    <style type="text/css">
> +      #p1 { background-color: rgba(10, 175, 10, 1.0); }
> +      #p2 { background-color: rgba(10, 175, 10, 100%); }

These don't seem like particularly useful values to test, for css-color-4 rgba().

(It looks like you're testing the same sorts of values as -001 here, but using rgba() and 1.0 opacity. As noted above, I think it'd be better to actually test the *same* values as your existing -001 test -- but with rgba() -- in a -001b test.)

If you want to add an 002 dedicated test for rgba() in css-color-4, I think it'd be much more useful to (primarily) test it with 3-component-values (which only start being valid in css-color-4), rather than testing it with values that always worked like rgba(10, 175, 10, 1.0).

(4-component colors are interesting, too, but they're even more interesting with non-1.0 alpha values -- and you might as well exercise that via -001b so you can actually verify that rgb() and rgba() are truly aliases, as a side-benefit)

Comment 98

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7. add tests for css-color-4 color function changes. v2

Comment 99

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8795359 [details] [diff] [review]
P7. add tests for css-color-4 color function changes. v2

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

background-color-rgb-001.html => test rgb() syntax in css-color-4
background-color-rgb-002.html => test rgba() syntax in css-color-4
background-color-rgb-003.html => show rgb() and rgba() are aliases

background-color-hsl-001.html => test hsla() syntax in css-color-4
background-color-hsl-002.html => test hsl() syntax in css-color-4
background-color-hsl-003.html => test the <angle> hue component
background-color-hsl-004.html => show hsl() and hsla() are aliases

Comment 100

[Daniel Holbert [:dholbert]]

Comment on attachment 8795359 [details] [diff] [review]
P7. add tests for css-color-4 color function changes. v2

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

::: layout/reftests/w3c-css/submitted/css-color-4/reftest-stylo.list
@@ +1,1 @@
> +# The stylo doesn't implement the css-color-4 yet. So, skip all tests here.

I sanity-checked with Manishearth (who added the reftest-stylo.list files) in #layout about what (if anything) we should do with these reftest-stylo.list files.

Upshot from that conversation:
 (0) Please remove this first line. (it won't be accurate anymore, per my other comments below)

 (1) Please copypaste the boilerplate line from other reftest-stylo.list files:
# DO NOT EDIT! This is a auto-generated temporary list for Stylo testing

 (2) After that line, please add a blank line (for separation) and then a comment like the following, which will help Manish on his next pass over stylo reftest failures:
  # This file was created in bug 1295456. Pretty sure they all
  # fail with stylo since it doesn't implement css-color-4 yet.

 (3) Instead of disabling these reftests with "#", let's just mark them as "fails", so they actually get run.  Given that css-color-4 isn't supported in Servo's CSS parser yet, it's pretty certain that these tests should all fail in styloized Gecko, I think -- so "fails" is appropriate.  (In case we happen to be wrong, that'd arguably be a bug in the test, since it shouldn't pass unless the feature is implemented.)

Comment 101

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P8. embrace css-color-4 rgb()/hsl() for nsCSSValue AppendToString() output. v1

From comment 82, it's time to use the new rgb()/hsl() in css-color-4.

Comment 102

[Daniel Holbert [:dholbert]]

Comment on attachment 8795359 [details] [diff] [review]
P7. add tests for css-color-4 color function changes. v2

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

Thanks for fixing this up!

Along with comment 100's stylo-manifest tweaks, here are a few more nits.  This will be r=me with the following & comment 100 addressed.

::: layout/reftests/w3c-css/submitted/css-color-4/background-color-hsl-004-ref.html
@@ +9,5 @@
> +      #p1 { background-color: hsl(120, 75%, 50%); }
> +      #p2 { background-color: hsl(120.0, 75%, 50%); }
> +      #p3 { background-color: hsl(1.2e2, 75%, 50%); }
> +      #p4 { background-color: hsl(1.2E2, 75%, 50%); }
> +      #p5 { background-color: hsl(0, 75%, 50%); }

One nit on #p5 here (which applies to this reference's corresponding testcase, background-color-hsl-004.html, as well):

Right now, this renders as bright red (because hue=0 is red).  And the presence of red in a CSS test normally means "something failed".  Could you you change this 0 to 50, perhaps? (here and in the testcase)  That seems to be a shade of yellow.

@@ +14,5 @@
> +      #p6 { background-color: hsla(120, 75%, 50%, 0.2); }
> +      #p7 { background-color: hsla(120.0, 75%, 50%, 0.4); }
> +      #p8 { background-color: hsla(1.2e2, 75%, 50%, 0.6); }
> +      #p9 { background-color: hsla(1.2E2, 75%, 50%, 0.8); }
> +      #p10 { background-color: hsla(0.0, 75%, 50%), 1.0; }

Two nits on #p10: (both of which apply to this reference's corresponding testcase, background-color-hsl-004.html)
 (1) This is invalid syntax -- the close-paren needs to be shifted to after the "1.0".
 (2) After that's fixed, this renders as bright red (like #p5 above), which makes a correct rendering look like a possible-test-failure (to a human viewer). Could you change the 0.0 hue to 50.0?

::: layout/reftests/w3c-css/submitted/reftest.list
@@ +70,5 @@
>  # CSS Writing Modes Level 3
>  include writing-modes-3/reftest.list
> +
> +# CSS-Color-4
> +include css-color-4/reftest.list

Looks like this file is alphabetically sorted -- please insert this new chunk in alphabetical order, rather than just at the end.

(Same goes for its sibling reftest-stylo.list file.)

::: layout/style/test/property_database.js
@@ +2783,5 @@
> +      /* hsl() and hsla() are aliases of each other. */
> +      "hsl(0, 0%, 0%)", "hsla(0, 0%, 0%)", "hsl(0, 0%, 0%, 1)", "hsla(0, 0%, 0%, 1)",
> +      /* rgb() and rgba() functions now accept <number> rather than <integer>. */
> +      "rgb(0.0, 0.0, 0.0)", "rgba(0.0, 0.0, 0.0)", "rgb(0.0, 0.0, 0.0, 1)", "rgba(0.0, 0.0, 0.0, 1)",
> +      /* <alpha-value> now accept <percentage> as well as <number> in rgba() and hsla(). */

s/accept/accepts/

(in "<alpha-value> now accept")

@@ +2802,5 @@
> +      "hsl(0, 0, 0%)", "hsla(0%, 0%, 0%, 0.1)",
> +      /* css-color-4: */
> +      /* comma and comma-less expressions should not mix together. */
> +      "rgb(0, 0, 0 / 1)", "rgb(0 0 0, 1)", "rgb(0, 0 0, 1)", "rgb(0 0, 0 / 1)",
> +      "hsl(0, 0%, 0% / 1)", "hsl(0 0% 0%, 1)", "hsl(0 0% 0%, 1)", "hsl(0 0%, 0% / 1)",

Please add four more "invalid" edge cases here:

 BEFORE THE CSS-COLOR-4 COMMENT:
  Trailing commas in otherwise-valid traditional rgb / rgba expressions:
  - rgb(0,0,0,)
  - rgba(0,0,0,0,)

 AFTER THE CSS-COLOR-4 COMMENT:
  Trailing slashes in otherwise-valid expressions:
    rgb(0 0 0 /)    (comma-less syntax, just missing alpha-value)
    rgb(0, 0, 0 /)  (comma syntax; we better reject at the "/" even without an alpha)

Comment 103

[Daniel Holbert [:dholbert]]

Comment on attachment 8795396 [details] [diff] [review]
P8. embrace css-color-4 rgb()/hsl() for nsCSSValue AppendToString() output. v1

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

This looks fine to me; I'll hold off on r+ pending your decision on how you want to handle removing the "A" units (my first comment review comment below) -- if that happens as part of this patch, I should take another look at this.

::: layout/style/nsCSSValue.cpp
@@ -1561,5 @@
> -        uint8_t a = NS_GET_A(color);
> -        bool showAlpha =
> -          (aSerialization == eNormalized && a < 255) ||
> -          (aSerialization == eAuthorSpecified &&
> -           unit == eCSSUnit_RGBAColor);

Now that you're removing this usage of the RGBAColor unit, I suspect that unit serves no purpose.  (Same goes for eCSSUnit_PercentageRGBAColor and eCSSUnit_HSLAColor, whose special-case usages you're removing below.)

We should probably just remove these "A" enum-values entirely, unless they're serving some other purpose that I'm not seeing.  That could either happen in this patch or in another patch; either is fine with me.

@@ +1566,5 @@
>          aResult.AppendInt(NS_GET_G(color), 10);
>          aResult.Append(comma);
>          aResult.AppendInt(NS_GET_B(color), 10);
> +        uint8_t a = NS_GET_A(color);
> +        if (a < 255) {

This should probably be != 255, for consistency with my suggestion for the nsCSSValueFloatColor version of this comparison below.

(Technically it doesn't matter here, because it's impossible for 'a' to be larger than 255 in a nscolor representation.  So, this is just from a consistency standpoint.)

@@ +2995,5 @@
>    aResult.AppendFloat(mComponent2 * 100.0f);
>    aResult.AppendLiteral("%, ");
>    aResult.AppendFloat(mComponent3 * 100.0f);
> +  aResult.AppendLiteral("%");
> +  if (mAlpha < 1.0f) {

This should be !=, not <.

(As of bug 1216843, we can represent float alpha values that are larger than 1, and we should honor those in serialization.)

Comment 104

[Daniel Holbert [:dholbert]]

Comment on attachment 8795396 [details] [diff] [review]
P8. embrace css-color-4 rgb()/hsl() for nsCSSValue AppendToString() output. v1

Actually, on further thought, I think the "A"-flavored CSS-unit-removal changes  will be large enough that they belong in their own patch.

So, r=me on P8 with the "!=" review comments addressed (and assuming an additional patch comes along with the now-obsolete-I-think-"A"-unit removal.)

Comment 105

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7. add tests for css-color-4 color function changes. v3

don't use red color for reftest.

update reftest-stylo.list
  update comments
  setup failed testing for css-color-4 items

add more test case in property_database.js
  trailing commas
  trailing slashes

Comment 106

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P8. embrace css-color-4 rgb()/hsl() for nsCSSValue AppendToString() output. v2. r=dholbert

update for comment 103.

the now-unused alpha component related enums will be removed in P9 patch.

Comment 107

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P9. remove now-unused alpha component related nsCSSUnit enum. v1

Comment 108

[Daniel Holbert [:dholbert]]

Comment on attachment 8796046 [details] [diff] [review]
P7. add tests for css-color-4 color function changes. v3

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

Nearly there!

::: layout/reftests/w3c-css/submitted/css-color-4/reftest-stylo.list
@@ +7,5 @@
> +#hsl
> +falis == background-color-hsl-001.html background-color-hsl-001-ref.html
> +fails == background-color-hsl-002.html background-color-hsl-002-ref.html
> +falis == background-color-hsl-003.html background-color-hsl-003-ref.html
> +falis == background-color-hsl-004.html background-color-hsl-004-ref.html

You have at least 3 instances of "falis" here (typo, "l" and "i" swapped).

Please make sure they all say "fails".

::: layout/reftests/w3c-css/submitted/reftest-stylo.list
@@ +20,5 @@
>  # Containment
>  include contain/reftest-stylo.list
>  
> +# CSS-Color-4
> +include css-color-4/reftest-stylo.list

Hmm, given that none of the other subdirectories are named "css-***", I suppose this one shouldn't either.

Please rename this folder to just "color4" to match the pattern used by the other subdirectories here. (And move it up above "conditional3" in the reftest[-stylo].list manifest files, to keep it sorted.)

And I suppose the comment should say "Color Level 4", too. (to match the pattern of the other nearby lines)

::: layout/style/test/property_database.js
@@ +2800,5 @@
> +    invalid_values: [ "#f", "#ff", "#fffff", "#fffffff", "#fffffffff",
> +      "rgb(100%, 0, 100%)", "rgba(100, 0, 100%, 30%)",
> +      "hsl(0, 0, 0%)", "hsla(0%, 0%, 0%, 0.1)",
> +      /* trailing commas */
> +      "rgb(0, 0, 0,)" , "rgba(0, 0, 0, 0,)",

nit: drop the stray space character between the close-quote and comma (after the first value in this line)

Comment 109

[Daniel Holbert [:dholbert]]

Comment on attachment 8796049 [details] [diff] [review]
P9. remove now-unused alpha component related nsCSSUnit enum. v1

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

Commit message nit:
>  remove now-unused alpha component related nsCSSUnit enum

This sentence is a bit hard to visually parse... I see "remove now-unused alpha component", and then some more words that trip me up and force me to re-read the first part.


How about this instead (this sounds clearer in my head, at least):
  Bug 1295456 - remove now-unnecessary nsCSSUnit enum values for rgba()/hsla() color functions, since they're are now just legacy aliases.

r=me with a clarification along those lines, and the following:

::: layout/style/nsCSSValue.h
@@ +474,5 @@
>    eCSSUnit_Enumerated   = 71,     // (int) value has enumerated meaning
>  
>    eCSSUnit_EnumColor           = 80,   // (int) enumerated color (kColorKTable)
> +  eCSSUnit_RGBColor            = 81,   // (nscolor) an RGBA value specified as
> +                                       // css-color-4 rgb() color function with

"specified as...rgb()" here is a bit misleading, because the author might *actually* have specified it as rgba().

SO: Let's clarify this to "...specified as css-color-4 rgb()/rgba()..." here.

@@ +481,5 @@
> +  eCSSUnit_ShortHexColor       = 83,   // (nscolor) an opaque RGBA value specified as #rgb
> +  eCSSUnit_HexColorAlpha       = 84,   // (nscolor) an opaque RGBA value specified as #rrggbbaa
> +  eCSSUnit_ShortHexColorAlpha  = 85,   // (nscolor) an opaque RGBA value specified as #rgba
> +  eCSSUnit_PercentageRGBColor  = 86,   // (nsCSSValueFloatColor*) an RGBA value
> +                                       // specified as css-color-4 rgb() color

Same here.

Comment 110

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P9. remove now-unnecessary nsCSSUnit enum values for rgba()/hsla() color functions. v2. r=dholbert

update for comment 109.

Since the commit title is too long, I break them into patch comment.
How about the title and comments in patch?

Comment 111

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7. add tests for css-color-4 color function changes. v4

update for comment 108.
change the tests' folder name.
update property_database.js.

Comment 112

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8796973 [details] [diff] [review]
P7. add tests for css-color-4 color function changes. v4

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

::: layout/reftests/w3c-css/submitted/color4/background-color-hsl-001-ref.html
@@ +1,1 @@
> +<!DOCTYPE html>

move test files into

layout/reftests/w3c-css/submitted/color4/

::: layout/reftests/w3c-css/submitted/color4/reftest-stylo.list
@@ +4,5 @@
> +# since it doesn't implement css-color-4 yet.
> +
> +#css-color-4 function
> +#hsl
> +fails == background-color-hsl-001.html background-color-hsl-001-ref.html

fix the typo

::: layout/style/test/property_database.js
@@ -814,5 @@
>      "-webkit-gradient(linear, 1 2, 3 4, from(00ff00))",
>      "-webkit-gradient(linear, 1 2, 3 4, from(##00ff00))",
>      "-webkit-gradient(linear, 1 2, 3 4, from(#00fff))", // wrong num hex digits
>      "-webkit-gradient(linear, 1 2, 3 4, from(xyz(0,0,0)))", // bogus color func
> -    "-webkit-gradient(linear, 1 2, 3 4, from(rgb(100, 100.5, 30)))", // fraction

rgb(100, 100.5, 30) is acceptable now.

@@ -2264,5 @@
>      inherited: false,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "transparent", "rgba(255, 127, 15, 0)", "hsla(240, 97%, 50%, 0.0)", "rgba(0, 0, 0, 0)", "rgba(255,255,255,-3.7)" ],
> -    other_values: [ "green", "rgb(255, 0, 128)", "#fc2", "#96ed2a", "black", "rgba(255,255,0,3)", "hsl(240, 50%, 50%)", "rgb(50%, 50%, 50%)", "-moz-default-background-color" ],
> -    invalid_values: [ "#0", "#00", "#00000", "#0000000", "#000000000", "rgb(100, 100.0, 100)" ],

move rgb(100, 100.0, 100) from invalid_values to initial_values section.

Comment 113

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8796973 [details] [diff] [review]
P7. add tests for css-color-4 color function changes. v4

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

::: layout/style/test/property_database.js
@@ -2264,5 @@
>      inherited: false,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "transparent", "rgba(255, 127, 15, 0)", "hsla(240, 97%, 50%, 0.0)", "rgba(0, 0, 0, 0)", "rgba(255,255,255,-3.7)" ],
> -    other_values: [ "green", "rgb(255, 0, 128)", "#fc2", "#96ed2a", "black", "rgba(255,255,0,3)", "hsl(240, 50%, 50%)", "rgb(50%, 50%, 50%)", "-moz-default-background-color" ],
> -    invalid_values: [ "#0", "#00", "#00000", "#0000000", "#000000000", "rgb(100, 100.0, 100)" ],

move rgb(100, 100.0, 100) from invalid_values to other_values section.

Comment 114

[Daniel Holbert [:dholbert]]

Comment on attachment 8796973 [details] [diff] [review]
P7. add tests for css-color-4 color function changes. v4

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

r=me with the following:

::: layout/style/test/property_database.js
@@ -814,5 @@
>      "-webkit-gradient(linear, 1 2, 3 4, from(00ff00))",
>      "-webkit-gradient(linear, 1 2, 3 4, from(##00ff00))",
>      "-webkit-gradient(linear, 1 2, 3 4, from(#00fff))", // wrong num hex digits
>      "-webkit-gradient(linear, 1 2, 3 4, from(xyz(0,0,0)))", // bogus color func
> -    "-webkit-gradient(linear, 1 2, 3 4, from(rgb(100, 100.5, 30)))", // fraction

Instead of just removing this line, please replace it with a different still-invalid rgb() expression.

(The point of the line wasn't that *this specific* rgb() function was invalid -- the point was to test an invalid rgb() expression.)

So: please:
 * bring back this line
 * replace 100.5 with 100% (mixing <number> and <percent> which is still invalid).
 * update the comment to say "// invalid rgb() func" or something like that.

@@ -2264,5 @@
>      inherited: false,
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "transparent", "rgba(255, 127, 15, 0)", "hsla(240, 97%, 50%, 0.0)", "rgba(0, 0, 0, 0)", "rgba(255,255,255,-3.7)" ],
> -    other_values: [ "green", "rgb(255, 0, 128)", "#fc2", "#96ed2a", "black", "rgba(255,255,0,3)", "hsl(240, 50%, 50%)", "rgb(50%, 50%, 50%)", "-moz-default-background-color" ],
> -    invalid_values: [ "#0", "#00", "#00000", "#0000000", "#000000000", "rgb(100, 100.0, 100)" ],

As above: please leave this value where it was, and just update it so that it's still invalid, e.g. by making it mix percent with non-percent.

This value isn't special/interesting in-and-of-itself -- it's just a placeholder for "some invalid rgb() expression". It lets us test...
 (1) ...that rgb() is accepted (that's tested via entries in other_values).
 (2) ...but that we do successfully reject *invalid* rgb() values.

If you remove this entry, we're not testing (2) anymore.

Comment 115

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #114)
> As above: please leave this value where it was, and just update it so that
> it's still invalid, e.g. by making it mix percent with non-percent.

(And by "this value" I mean the rgb(100, 100.0, 100) value that the patch is currently moving from invalid_values to other_values.)

Comment 116

[Daniel Holbert [:dholbert]]

(And if you like, feel free to add rgb(100, 100.0, 100) to the list of other_values for "color", further down in this file, if we don't yet have any mixed-integer-and-number-values there.  That's the appropriate place to test edge cases & interesting mixed syntax for color functions.  But up here for "background-color", we only really need to spot-test a few valid/invalid values.)

Comment 117

[Daniel Holbert [:dholbert]]

Comment on attachment 8796972 [details] [diff] [review]
P9. remove now-unnecessary nsCSSUnit enum values for rgba()/hsla() color functions. v2. r=dholbert

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

Latest P9 looks good to me; r+

Comment 118

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P7. add tests for css-color-4 color function changes. v5. r=dholbert

update for comment 114, 115 and 116.

Comment 119

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I can't catch the meaning of this testing table from:
https://github.com/w3c/web-platform-tests/blob/master/editing/data/forecolor.js
https://github.com/w3c/web-platform-tests/blob/master/editing/README

There is the wpt failed at:
https://treeherder.mozilla.org/logviewer.html#?job_id=28780169&repo=try#L3772

Comment 120

[Daniel Holbert [:dholbert]]

The test failure output there says:
> expected "foo<span style=\"color:rgba(0, 0, 255, 0.5)\">bar</span>baz"
>  but got "foo<span style=\"color:rgb(0, 0, 255, 0.5)\">bar</span>baz"

Note the rgba-->rgb difference there. (because we're deprecating rgba() as a serialization, in this bug)

You probably want to just find this input (and/or expected-output) value in the test logic there, and update it to use "rgb" instead of "rgba".

Comment 121

[Daniel Holbert [:dholbert]]

(Probably simplest to just do s/rgba/rgb/ across that whole forecolor.js file, if you like.)

Comment 122

[Daniel Holbert [:dholbert]]

(That change probably wants to happen *as part of* P8 or P9; wherever we stop serializing rgba() as rgba().)

Comment 123

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P10. use user specific css color function name for css value serialization. v1

From irc, we still try to use more backwards-compatible result. So, here is the patch for that and remove the P8 and P9 patches.

If the color is fully opaque, omit the 'a' suffix of color function name (e.g. just use 'rgb' or 'hsl').
Otherwise, show the original user specific function name for serialization.

Comment 124

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P11. update gecko tests for css-color-4 color function changes. v1.

This patch try to update the invalid/valid css color value in our tests for the new css-color-4 color function changes.

Comment 125

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P12. update web-platform tests for new css-color-4 color function changes. v1

I'm not sure the right process flow for wpt updating. And should we create the pull-request for wpt update at github?

--
update wpt for new css-color-4 color function changes.

1) update testing/web-platform/tests/2dcontext/tools/tests2d.yaml
2) execute testing/web-platform/tests/2dcontext/tools/gentest.py
3) ./mach web-platform-tests --manifest-update

Comment 126

[Daniel Holbert [:dholbert]]

Thanks, Jerry!

So, I've just discovered that CSSOM actually has explicit (though perhaps-in-need-of-an-update) spec-text on how to serialize rgb/rgba colors.  Right now, it just says to always use rgba() if alpha isn't one.
Source: https://drafts.csswg.org/cssom/#serializing-css-values

So, I'm not sure we need to respect the syntax that authors used -- we could perhaps still get rid of the RGB/RGBA distinction, and simply serialize based on the presence of non-1 alpha... Though maybe we should wait a few days to see if the CSSWG has strong feelings.

I filed this github issue, asking for clarification: https://github.com/w3c/csswg-drafts/issues/585

Comment 127

[Daniel Holbert [:dholbert]]

(Oh, I guess that spec-text is mostly about *computed* values -- and further down it says this about specified values (for nsCSSValue serialization, which is what we're mostly concerning ourselves with here):
> If <color> is a component of a specified value, then return the color as follows:
>   If the color was explicitly specified by the author, then return the original, author specified color value.

Your latest patches are in that spirit, which means they're probably good.  The spec still needs to stabilize a bit perhaps, but I think we're OK proceeding with what you outlined in comment 123. (I hope to take a closer look & r+ later this afternoon.)

Comment 128

[Daniel Holbert [:dholbert]]

Comment on attachment 8799312 [details] [diff] [review]
P10. use user specific css color function name for css value serialization. v1

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

r=me with the following.

Firstly, commit message nit:
>Bug 1295456 - use user specific css color function name for css value serialization.
> [...]
> Otherwise, show the original user specific function name for serialization.

s/user specific/author-specified/ (on the first line as well as the last line)


("user specific" would imply "a specific/customized one for each user" -- you really mean "specified", not "specific".  And this syntax is specified by the *author*, not the user.)

::: layout/style/nsCSSValue.cpp
@@ +1558,5 @@
>          // round-tripping of all other rgba() values.
>          aResult.AppendLiteral("transparent");
>        } else {
>          uint8_t a = NS_GET_A(color);
> +        bool showAlpha = a != 255;

A few things:
 - add parens to the boolean condition, to make this easier to visually parse:
  bool showAlpha = (a != 255);

Also, please add a comment to explain what we're aiming to do here (since it's non-obvious and not-really-specced) -- something like this:
        // For brevity, we omit the alpha component if it's equal to 1.  Also,
        // we try to preserve the author-specified function name, unless it's
        // rgba() and we're omitting the alpha component - then we use rgb().

@@ +2984,5 @@
>  nsCSSValueFloatColor::AppendToString(nsCSSUnit aUnit, nsAString& aResult) const
>  {
>    MOZ_ASSERT(nsCSSValue::IsFloatColorUnit(aUnit), "unexpected unit");
>  
> +  bool showAlpha = mAlpha != 1.0f;

As above:
 - add parens around "mAlpha != 1.0f"
 - add a comment to explain the intent of our somewhat-quirky logic here.  (In this case, we might have rgb/rgba/hsl/hsla, so the comment needs a little more nuance.)

@@ +2985,5 @@
>  {
>    MOZ_ASSERT(nsCSSValue::IsFloatColorUnit(aUnit), "unexpected unit");
>  
> +  bool showAlpha = mAlpha != 1.0f;
> +  bool isHSL = aUnit == eCSSUnit_HSLColor || aUnit == eCSSUnit_HSLAColor;

It looks like you're just rewrapping this isHSL expression from 2 lines to 1 line. I'd prefer we don't do that, since it's more readable in its old formulation, with each comparison on its own line & nicely-aligned:

  bool isHSL = aUnit == eCSSUnit_HSLColor ||	
               aUnit == eCSSUnit_HSLAColor;

Though this could perhaps use some parens for improved-visual-parsability, too -- if you like:
  bool isHSL = (aUnit == eCSSUnit_HSLColor ||	
                aUnit == eCSSUnit_HSLAColor);

Comment 129

[Daniel Holbert [:dholbert]]

Comment on attachment 8799315 [details] [diff] [review]
P11. update gecko tests for css-color-4 color function changes. v1.

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

r=me

Comment 130

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P10. use author-specified css color function name for css value serialization. v2. r=dholbert

update for comment 128.

---
If the color is fully opaque, omit the 'a' suffix of color function name (e.g. just use 'rgb' or 'hsl').
Otherwise, show the original author-specified function name for serialization.

Comment 131

[Daniel Holbert [:dholbert]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #125)
> Created attachment 8799316 [details] [diff] [review]
> P12. update web-platform tests for new css-color-4 color function changes. v1
> 
> I'm not sure the right process flow for wpt updating

Nor am I.

> And should we create
> the pull-request for wpt update at github?

Probably not -- at least, I don't think that needs to block this bug. I'd defer to the documentation here:
https://dxr.mozilla.org/mozilla-central/rev/7ae377917236b7e6111146aa9fb4c073c0efc7f4/testing/web-platform/README.md#28
...which says "You can commit the tests directly to the Mozilla repository under `testing/web-platform/tests` and they will be upstreamed next time the test is imported."  (I think "The Mozilla repository" means mozilla-central; and your changes are within the subdirectory mentioned there, so I think we're good. I expect our changes will be merged with upstream the next time we import -- which could be problematic if upstream has conflicting changes, but hopefully that won't happen (and we could merge sooner/proactively if that's a real possibility).

I pinged jgraham as an extra sanity-check, but he seems to be away at the moment. So, I think we're probably OK to proceed with your WPT patch here -- and if we're mistaken about WPT process, we can address any needed changes in a followup bug.

Comment 132

[Daniel Holbert [:dholbert]]

Comment on attachment 8799316 [details] [diff] [review]
P12. update web-platform tests for new css-color-4 color function changes. v1

r=me with the following addressed:

>Bug 1295456 - update web-platform tests for new css-color-4 color function changes.
>
>update wpt for new css-color-4 color function changes.
>
>1) update testing/web-platform/tests/2dcontext/tools/tests2d.yaml
>2) execute testing/web-platform/tests/2dcontext/tools/gentest.py
>3) ./mach web-platform-tests --manifest-update

Your second line of the commit message ("update wpt [...]") doesn't add any information -- it's just repeating what you said in the first line.

So: please drop that line.

>+++ b/testing/web-platform/tests/2dcontext/tools/tests2d.yaml
>@@ -1367,16 +1367,50 @@
>+        # css-color-4 rgb() color function
>+        #   https://drafts.csswg.org/css-color/#numeric-rgb
[...]
>+        ('css-color-4-rgb-2', 'rgb(0, 255, 0, 0.5)', 0,255,0,128, ""),

This is not quite valid! It looks like you're asserting that 0.5 alpha is exactly equivalent to an 8-bit value of 128 -- but that's not quite right!  Technically, the 50% point of that range would be 0+.5*255 = 127.5, which we can't exactly represent.  We *will* round that up to 128 under the hood, but it seems fragile to hardcode that expectation in a wpt test; maybe someday we'll have better precision, and maybe some other browser already does have better precision (or different rounding choices).  So, unless there's solid spec text to *require* this exact rounding, we should just choose a fraction/percent that produces a whole-number color channel value here.

.2 happens to be a nice fraction to apply to 255, mathematically: .2 * 255 = 51 (nice whole number).

So, in your new rgb() lines here, let's just replace all instances of .5 & 50% with .2 & 20%. (and then in the results, replace 128 with 51)

>+        # css-color-4 rgb() color function
>+        #   https://drafts.csswg.org/css-color/#the-hsl-notation
>+        ('css-color-4-hsl-1', 'hsl(120 100.0% 50.0%)', 0,255,0,255, ""),
>+        ('css-color-4-hsl-2', 'hsl(120 100.0% 50.0% / 0.5)', 0,255,0,128, ""),

Comment typo -- you meant "hsl() color function" here, not "rgb()"

ALSO: as above, let's replace the 0.5 alpha-channel values throughout this section with 0.2, and its results with 51 instead of 128.  (Only for the "A" value here -- not for the "L" value, I guess, since that's not directly converted to an 8-bit value.)

Comment 133

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P12. update web-platform tests for new css-color-4 color function changes. v2 r=dholbert

Comment 134

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P12. update web-platform tests for new css-color-4 color function changes. v3. r=dholbert

update for comment 132.

Comment 135

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land these patches to m-c after bug 1302787 patch.

https://bugzilla.mozilla.org/attachment.cgi?id=8790083
https://bugzilla.mozilla.org/attachment.cgi?id=8790199
https://bugzilla.mozilla.org/attachment.cgi?id=8790085
https://bugzilla.mozilla.org/attachment.cgi?id=8790557
https://bugzilla.mozilla.org/attachment.cgi?id=8790087
https://bugzilla.mozilla.org/attachment.cgi?id=8798761
https://bugzilla.mozilla.org/attachment.cgi?id=8799667
https://bugzilla.mozilla.org/attachment.cgi?id=8799315
https://bugzilla.mozilla.org/attachment.cgi?id=8800219

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40aa99af1f3d&selectedJob=29218925&group_state=expanded

Comment 136

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbae54e61d06
Support percentage opacity value in CSS color functions. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/20bce2bfa3aa
Change CSS color function rgb() to be an alias of rgba() and support css-color-4 comma-less expression. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3d443e78452
Change CSS color function hsl() to be an alias of hsla() and support css-color-4 comma-less expression. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/daa2a1f8326a
Support <angle> value for hue component in CSS hsl() color function. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb1c914ee90
Remove now-unused single-arg ParseColorOpacity functions from nsCSSParser. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa15b95bb73
Add tests for css-color-4 color function changes. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a29d5cb24af4
Use author-specified css color function name for css value serialization. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bfcbf898f11
Update Gecko tests for css-color-4 color function changes. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fde1a9df579
Update web-platform tests for new css-color-4 color function changes. r=dholbert

Comment 137

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbae54e61d06
https://hg.mozilla.org/mozilla-central/rev/20bce2bfa3aa
https://hg.mozilla.org/mozilla-central/rev/b3d443e78452
https://hg.mozilla.org/mozilla-central/rev/daa2a1f8326a
https://hg.mozilla.org/mozilla-central/rev/0fb1c914ee90
https://hg.mozilla.org/mozilla-central/rev/dfa15b95bb73
https://hg.mozilla.org/mozilla-central/rev/a29d5cb24af4
https://hg.mozilla.org/mozilla-central/rev/4bfcbf898f11
https://hg.mozilla.org/mozilla-central/rev/3fde1a9df579

Comment 138

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

The CSS WG test system didn't associate the new tests with the color 4 spec because:

remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-hsl-001-ref.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-hsl-001.html status changed to 'Needs Work' due to error:
remote: Linked to unversioned specification URL https://drafts.csswg.org/css-color/#the-hsl-notation. Use https://www.w3.org/TR/css-color-4/... or https://drafts.csswg.org/css-color-4/... instead.
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-hsl-002-ref.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-hsl-002.html status changed to 'Needs Work' due to error:
remote: Linked to unversioned specification URL https://drafts.csswg.org/css-color/#the-hsl-notation. Use https://www.w3.org/TR/css-color-4/... or https://drafts.csswg.org/css-color-4/... instead.
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-hsl-003-ref.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-hsl-003.html status changed to 'Needs Work' due to error:
remote: Linked to unversioned specification URL https://drafts.csswg.org/css-color/#the-hsl-notation. Use https://www.w3.org/TR/css-color-4/... or https://drafts.csswg.org/css-color-4/... instead.
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-hsl-004-ref.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-hsl-004.html status changed to 'Needs Work' due to error:
remote: Linked to unversioned specification URL https://drafts.csswg.org/css-color/#the-hsl-notation. Use https://www.w3.org/TR/css-color-4/... or https://drafts.csswg.org/css-color-4/... instead.
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-rgb-001-ref.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-rgb-001.html status changed to 'Needs Work' due to error:
remote: Linked to unversioned specification URL https://drafts.csswg.org/css-color/#rgb-functions. Use https://www.w3.org/TR/css-color-4/... or https://drafts.csswg.org/css-color-4/... instead.
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-rgb-002-ref.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-rgb-002.html status changed to 'Needs Work' due to error:
remote: Linked to unversioned specification URL https://drafts.csswg.org/css-color/#rgb-functions. Use https://www.w3.org/TR/css-color-4/... or https://drafts.csswg.org/css-color-4/... instead.
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-rgb-003-ref.html status changed to 'Submitted for Review'
remote: vendor-imports/mozilla/mozilla-central-reftests/color4/background-color-rgb-003.html status changed to 'Needs Work' due to error:
remote: Linked to unversioned specification URL https://drafts.csswg.org/css-color/#rgb-functions. Use https://www.w3.org/TR/css-color-4/... or https://drafts.csswg.org/css-color-4/... instead.

(Also, in the future (although it's not worth redoing these), it's probably better for this sort of test to be a scripted test (i.e., web-platform-test) rather than a reftest, since that's less computationally expensive.)

Comment 139

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: P13. add the versioned spec link for css-color-4 reftest. v1

update the spec link for comment 138.

Comment 140

[Daniel Holbert [:dholbert]]

Comment on attachment 8801848 [details] [diff] [review]
P13. add the versioned spec link for css-color-4 reftest. v1

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

r=me

Comment 141

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4abaf0bda1fe
add the versioned spec link for css-color-4 reftest. r=dholbert

Comment 142

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4abaf0bda1fe

Comment 143

[Xidorn Quan [:xidorn] UTC+11]

Sent a belated intent-to-ship for this: https://groups.google.com/d/msg/mozilla.dev.platform/kXX7TrKhIRw/4eS85MEwAAAJ

Comment 144

[Daniel Holbert [:dholbert]]

Thanks, Xidorn! Sorry, I should've requested that back when reviewing this. Your description in that post looks good to me.

Comment 145

[Chris Mills [:cmills]]

I've documented these changes, adding descriptions and examples where appropriate to the <color> reference page, as well as updating the browser support and spec information:
https://developer.mozilla.org/en-US/docs/Web/CSS/color_value

I've updated the data for the macros that generate the CSS Formal syntax, for example:
https://developer.mozilla.org/en-US/docs/Web/CSS/background-color
https://developer.mozilla.org/en-US/docs/Web/CSS/color

I've also added a big note to the Fx52 release notes, describing the changes:
https://developer.mozilla.org/en-US/Firefox/Releases/52#CSS

Can you check these and let me know if they look OK? Particularly the formal syntax, as I've never updated that stuff before ;-) Thanks!