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

I would like to do this.

test page for percentage opacity

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.)

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

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

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?

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.

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

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 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 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 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 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 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 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 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 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.

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

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 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 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;

(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. :))

(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.)

Ah, thanks! Disregard comment 35 then.

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.

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

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 on attachment 8783294 [details] [diff] [review] P1. accept two unexpected characters report After refining the css parser patch, this change could be skipped.

Comment on attachment 8783295 [details] [diff] [review] P2. update css parser error messages After refining the css parser patch, this change could be skipped.

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.

rebase and update the comment.

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 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 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.

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

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 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 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 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 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.

(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. :)

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/

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.

add PEExpectedPercentOrNumber message in css.properties file.

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

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

rebase.

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 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 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 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 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.)

update for comment 64. s/PercentOrNumber/NumberOrPercent/

update for comment 65 especially for ParseColorOpacity function.

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

support <angle> value for hue.

rebase.

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 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 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.)

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

update for comment 74

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

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 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.

(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 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.

(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.

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

update for comment 83.

(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.

(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.

(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 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.

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

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.)

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

Filed.

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

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:bignose1007@gmail.com" /> > + <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 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 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.)

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

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 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 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.)

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

update for comment 103. the now-unused alpha component related enums will be removed in P9 patch.

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 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.

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?

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

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 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 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.

(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.)

(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 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+

update for comment 114, 115 and 116.

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

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".

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

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

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.

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

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

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

(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 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 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

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.

(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 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.)

update for comment 132.

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

Pushed by ryanvm@gmail.com: 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

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

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.)

update the spec link for comment 138.

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

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

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

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

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!