Bug 1295456 is implementing css-color-4. This adds new color function syntax to CSS. devtools will need some updates for this; at least css-color.js, but perhaps some other things as well.

Inspector bug triage (filter on CLIMBING SHOES).

There are some devtool testings failed with the bug 1295456 css-color-4 implementation. I try to update our devtool to pass the testings.

This is the js version of css-color-4 color function implementation from bug 1295456.

fix the wrong hsl() to rgba value.

The "rgb(24, 25, 45, 1)" is valid now. Turn to use "rgb(24, 25%, 45, 1)".

Comment on attachment 8798800 [details] [diff] [review] P1. Implement css-color-4 color function changes in devtool parser. v2 Review of attachment 8798800 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for doing this. I have a number of comments, but they're generally nit-picking; I think this patch looks good overall. However, I have one bigger concern. This patch unconditionally changes devtools to support the new color style. That's correct when debugging a current firefox - but not when debugging an older version (or when using valence to debug some other browser). I think we're going to need to add an attribute on the css properties actor (perhaps in the db) and then have the inspector adapt. This means that the parsing functions are going to need a mode where they reject css-4 colors. There's also the question of whether the color-cycling code (in CssColor) should try to preserve the comma-less state of its input. I'm sure you didn't think you were signing up to completely fix devtools for css-4 colors :) So I would suggest fixing up your current patches and adding the new parsing mode. If you make the mode default to old-style but change the test to use the new style, then maybe that will let all the inspector tests continue to work? And then I can fix up the remaining issues in a follow-up bug. If you're feeling motivated to fix everything of course I won't stop you :) I can provide some pointers in this case. I'm going to "blank" this review considering it needs some more work. ::: devtools/shared/css/color.js @@ +5,5 @@ > "use strict"; > > const Services = require("Services"); > > +const {angleUtils} = require("devtools/client/shared/css-angle"); Files in devtools/shared can't require files from devtools/client, so this will need to be done some other way. I tend to think this require isn't needed, see below. @@ +650,5 @@ > * @return {CSSToken} The next non-whitespace, non-comment token; or > * null at EOF. > */ > function getToken(lexer) { > + if (lexer.mHasPushBackToken) { devtools doesn't generally use the mMumble style. Please rename this property. I think "_hasPushBackToken" would be fine. @@ +651,5 @@ > * null at EOF. > */ > function getToken(lexer) { > + if (lexer.mHasPushBackToken) { > + if (lexer.mCurrentToken && lexer.mCurrentToken.tokenType !== "comment" && This logic looks suspect to me. First, can the current token here ever be "comment" or "whitespace"? I couldn't see how. Second, if there is a pushback token, and it is one of these (or null, meaning EOF), then this code will fall through to the ordinary parsing loop without resetting mHasPushbackToken. I think it's probably better to just remove this "if". @@ +676,5 @@ > + * @param {CSSLexer} lexer The lexer > + */ > +function unGetToken(lexer) { > + if (lexer.mHasPushBackToken) { > + alert('double pushback'); Throw an exception here, I don't think we use alert in devtools. Also, we don't use single quotes; this is enforced by eslint. @@ +684,5 @@ > + > +/** > + * A helper function to exame a symbol is exist or not. > + * > + * @param {CSSLexer} lexer The lexer This should have an @param line for "symbol" as well. @@ +687,5 @@ > + * > + * @param {CSSLexer} lexer The lexer > + * @return {Boolean} The expect symbol is parsed or not. > + */ > +function ExpectSymbol(lexer, symbol) { Since this isn't a constructor the name should start with a lower-case letter. @@ +808,5 @@ > + if (token.tokenType === "number") { > + val = token.number; > + } else if (token.tokenType === "dimension") { > + let angleValidator = new angleUtils.CssAngle(token.number + token.text); > + if (!angleValidator.valid) { CssAngle.valid boils down to a simple check: return (token.tokenType === "dimension" && token.text.toLowerCase() in CssAngle.ANGLEUNIT); ... but also instantiates a new lexer, which seems excessive here, considering that this function already has a token. I appreciate the desire for DRY; if you want to preserve that then perhaps moving ANGLEUNIT to devtools/shared/css/properties-db would be appropriate. @@ +855,2 @@ > > + let commaSeparator = ","; I think "const" here would be clearer. Also for hasComma and separatorBeforeAlpha. @@ +891,5 @@ > + // comma-less expression: > + // rgb() = rgb( component{3} [ / <alpha-value> ]? ) > + // the expression with comma: > + // rgb() = rgb( component#{3} , <alpha-value>? ) > + let commaSeparator = ","; The same "const" treatment in this function too.

Comment on attachment 8798747 [details] [diff] [review] P2. remove the now-unused requireComma() function in devtool colorUtils. v1 Review of attachment 8798747 [details] [diff] [review]: ----------------------------------------------------------------- Thank you. This is good.

Comment on attachment 8798801 [details] [diff] [review] P3. set a new invalid rgb color value for devtool test case. Review of attachment 8798801 [details] [diff] [review]: ----------------------------------------------------------------- Thank you. This looks good.

(In reply to Tom Tromey :tromey from comment #7) > Comment on attachment 8798800 [details] [diff] [review] > P1. Implement css-color-4 color function changes in devtool parser. v2 > > Review of attachment 8798800 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you for doing this. I have a number of comments, but they're > generally nit-picking; I think this patch looks good overall. > > However, I have one bigger concern. This patch unconditionally changes > devtools to support the new color style. > That's correct when debugging a current firefox - but not when debugging an > older version (or when using valence > to debug some other browser). Can we use firefox to debug other browser > > I think we're going to need to add an attribute on the css properties actor > (perhaps in the db) and then have the inspector > adapt. This means that the parsing functions are going to need a mode where > they reject css-4 colors. > Could you tell more about this? Do you mean that have a pref for the new mode parser? > There's also the question of whether the color-cycling code (in CssColor) > should try to preserve the comma-less > state of its input. > What's color-cycling code? Is it related to animation? https://en.wikipedia.org/wiki/Color_cycling > I'm sure you didn't think you were signing up to completely fix devtools for > css-4 colors :) > So I would suggest fixing up your current patches and adding the new parsing > mode. > If you make the mode default to old-style but change the test to use the new > style, then maybe > that will let all the inspector tests continue to work? And then I can fix > up the remaining issues > in a follow-up bug. > > If you're feeling motivated to fix everything of course I won't stop you :) > I can provide some pointers in this case. > > I'm going to "blank" this review considering it needs some more work. > > ::: devtools/shared/css/color.js > @@ +5,5 @@ > > "use strict"; > > > > const Services = require("Services"); > > > > +const {angleUtils} = require("devtools/client/shared/css-angle"); > > Files in devtools/shared can't require files from devtools/client, so this > will need to be done some other way. > > I tend to think this require isn't needed, see below. > I still try to share the css-angle parsing code for both client side and the color parser in devtools/shared/css/color.js . How about move devtools/client/shared/css-angle.js to devtools/shared/css/ folder? > @@ +808,5 @@ > > + if (token.tokenType === "number") { > > + val = token.number; > > + } else if (token.tokenType === "dimension") { > > + let angleValidator = new angleUtils.CssAngle(token.number + token.text); > > + if (!angleValidator.valid) { > > CssAngle.valid boils down to a simple check: > > return (token.tokenType === "dimension" > && token.text.toLowerCase() in CssAngle.ANGLEUNIT); > > ... but also instantiates a new lexer, which seems excessive here, > considering that this function already has a token. > > I appreciate the desire for DRY; if you want to preserve that then perhaps > moving ANGLEUNIT to devtools/shared/css/properties-db would be appropriate. > Yes, I think we could to that. But, how about the conversion of various angle units to degree? That code is already existed in css-angle.js. We might still have the duplicate code here.

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #10) > Can we use firefox to debug other browser Yes, see https://developer.mozilla.org/en-US/docs/Tools/Valence > Could you tell more about this? Do you mean that have a pref for the new > mode parser? No, not a pref, just a parameter to the parsing functions. It might be sufficient here to have the tests parse both ways (correctly rejecting css-4 colors when appropriate); and then not worry about the bigger devtools changes. > > There's also the question of whether the color-cycling code (in CssColor) > > should try to preserve the comma-less > > state of its input. > > > > What's color-cycling code? Is it related to animation? > https://en.wikipedia.org/wiki/Color_cycling In the inspector if you shift-click on a color swatch, it cycles through different color representations, like name->hex->rgb->hsl. The question here is whether this should try to preserve the spelling. > I still try to share the css-angle parsing code for both client side and the > color parser in devtools/shared/css/color.js . > How about move devtools/client/shared/css-angle.js to devtools/shared/css/ > folder? That's fine provided the resulting patch isn't instantiating a new lexer just for this one use. > Yes, I think we could to that. But, how about the conversion of various > angle units to degree? > That code is already existed in css-angle.js. We might still have the > duplicate code here. Yeah, if that's needed, it's fine to move the file. Or you could put helper functions somewhere, like css/parsing-utils.js.

update for comment 7. I will add an argument for the color parser to accept the old style syntax only in next patch.

update for comment 7. update for lint checking failed. I will add an argument for the color parser to accept the old style syntax only in next patch.

update hsl/rgb color function syntax in comment.

fix some typo.

update for lint failed.

update for lint failed

Comment on attachment 8800503 [details] [diff] [review] P1. Implement css-color-4 color function changes in devtool parser. v6 Review of attachment 8800503 [details] [diff] [review]: ----------------------------------------------------------------- Thank you. This looks good -- some minor nits, nothing serious. ::: devtools/shared/css/color.js @@ +6,5 @@ > > const Services = require("Services"); > > +const {CSS_ANGLEUNIT} = require("devtools/shared/css/properties-db"); > +const {getAngleValueInDegrees} = require("devtools/shared/css/parsing-utils.js"); Remove the ".js" here. @@ +681,5 @@ > + lexer._hasPushBackToken = true; > +} > + > +/** > + * A helper function to exame a symbol is exist or not. Typo in "examine". But I think this should probably read more like: A helper function that checks if the next token matches symbol. If so, reads the token and returns true. If not, pushes the token back and returns false. ::: devtools/shared/css/parsing-utils.js @@ +1134,5 @@ > priority: declaration ? declaration.priority : "" > }; > } > > +function getAngleValueInDegrees(angleValue, angleUnit) { Please add a jsdoc comment describing the function, arguments, and return value.

Comment on attachment 8800504 [details] [diff] [review] P4. add a mode for css-color-4 color parser to accept the old style syntax only. v3 Review of attachment 8800504 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for doing this. This looks basically good, though I found one spot I think is in error. Also I wonder what the test plan is for the "old-style" mode. ::: devtools/shared/css/color.js @@ +1017,5 @@ > + > + // Parse G, B and A. > + // No need to check the separator after 'B'. It will be checked in 'A' values > + // parsing. > + let separatorBeforeAlpha = hasComma ? commaSeparator : "/"; This doesn't look like old-style rgb. @@ +1040,5 @@ > * @param {String} name the color > * @return {Object} an object of the form {r, g, b, a}; or null if the > * name was not a valid color > */ > +function colorToRGBA(name, oldColorFunctionSyntax = true) { This needs a new @param comment explaining what the parameter means.

fix the bug for the invalid tailing token.

Update for comment 20. Rewrite parseOldStyleHsl and parseOldStyleRgb. there is a logic bug for the main if condition. And we can't use parseColorComponent() for the old style hue component parsing. The function output is [0,255]. Update parseColorComponent() comment

the test case for P4.

Comment on attachment 8801058 [details] [diff] [review] P1. implement css-color-4 color function changes in devtool parser. v7. r=ttromey Review of attachment 8801058 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 8801062 [details] [diff] [review] P4. add a mode for css-color-4 color parser to accept the old style syntax only. v4 Review of attachment 8801062 [details] [diff] [review]: ----------------------------------------------------------------- Thank you. This looks good to me.

Comment on attachment 8801064 [details] [diff] [review] P5. add a test for old-style and css-color-4 color function syntax in devtool. v1 Review of attachment 8801064 [details] [diff] [review]: ----------------------------------------------------------------- Thank you. This is very nice.

update for lint checking failed

The previous test case is failed. I'm trying to test with another value. It might be the rgb->hsl rounding problem. The devtool's output rgb value is only 1 unit different from gecko's rgb value (e.g. rgb(12,13,14) vs rgb(13,13,14)). If this is a big problem, I think we could fix that in another bug.

Hi Tom, In attachment 8801530 [details] [diff] [review], the default mode is setting for old-style. In comment 11: "No, not a pref, just a parameter to the parsing functions. It might be sufficient here to have the tests parse both ways (correctly rejecting css-4 colors when appropriate); and then not worry about the bigger devtools changes." Do you have any idea that how to turn on the new css-color-4 in devtool? What do you think about creating a option in devtool setting page? I'm not familiar with devtool, could you please fire the bug for these works? And could you please also check comment 28?

update for lint checking failed.

Please land these patch to m-c. https://bugzilla.mozilla.org/attachment.cgi?id=8801553 https://bugzilla.mozilla.org/attachment.cgi?id=8798747 https://bugzilla.mozilla.org/attachment.cgi?id=8798801 https://bugzilla.mozilla.org/attachment.cgi?id=8801530 https://bugzilla.mozilla.org/attachment.cgi?id=8801531 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/02c03ca774ee Implement css-color-4 color function changes in devtool parser. r=tromey https://hg.mozilla.org/integration/mozilla-inbound/rev/0b3435dc45f8 Remove the now-unused requireComma() function in devtool colorUtils. r=tromey https://hg.mozilla.org/integration/mozilla-inbound/rev/8597a7e99ac8 Set a new invalid rgb color value for devtool test case. r=tromey https://hg.mozilla.org/integration/mozilla-inbound/rev/281ed6b1d3b4 Add a mode for css-color-4 color parser to accept the old style syntax only. r=tromey https://hg.mozilla.org/integration/mozilla-inbound/rev/554359eddb93 Add a test for old-style and css-color-4 color function syntax in devtool. r=tromey

Sorry had to backed out this for lint failure, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=37754266&repo=mozilla-inbound#L4041

Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c9d1de3534 Backed out changeset 554359eddb93 for lint failure https://hg.mozilla.org/integration/mozilla-inbound/rev/b10b8e054d9f Backed out changeset 281ed6b1d3b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6fa8b34a0f Backed out changeset 8597a7e99ac8 https://hg.mozilla.org/integration/mozilla-inbound/rev/1f45b0e01383 Backed out changeset 0b3435dc45f8 https://hg.mozilla.org/integration/mozilla-inbound/rev/9707f0972299 Backed out changeset 02c03ca774ee

Pushed by hshih@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc049fc9f8c implement css-color-4 color function changes in devtool parser. r=ttromey https://hg.mozilla.org/integration/mozilla-inbound/rev/2b1aa4e34f14 remove the now-unused requireComma() function in devtool colorUtils. r=ttromey https://hg.mozilla.org/integration/mozilla-inbound/rev/c3994652649a set a new invalid rgb color value for devtool test case. r=ttromey https://hg.mozilla.org/integration/mozilla-inbound/rev/acf3d3d8766d add a mode for css-color-4 color parser to accept the old style syntax only. r=ttromey https://hg.mozilla.org/integration/mozilla-inbound/rev/5820cd95d563 add a test for old-style and css-color-4 color function syntax in devtool. r=ttromey

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #29) > Do you have any idea that how to turn on the new css-color-4 in devtool? > What do you think about creating a option in devtool setting page? > I'm not familiar with devtool, could you please fire the bug for these works? > > And could you please also check comment 28? I've filed follow-up bugs for both of these.

https://hg.mozilla.org/mozilla-central/rev/4bc049fc9f8c https://hg.mozilla.org/mozilla-central/rev/2b1aa4e34f14 https://hg.mozilla.org/mozilla-central/rev/c3994652649a https://hg.mozilla.org/mozilla-central/rev/acf3d3d8766d https://hg.mozilla.org/mozilla-central/rev/5820cd95d563