add devtools support for css-color-4

RESOLVED FIXED in Firefox 52

Status

enhancement
P2
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: tromey, Assigned: jerry)

Tracking

unspecified
Firefox 52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

()

Attachments

(5 attachments, 12 obsolete attachments)

1.28 KB, patch
tromey
: review+
Details | Diff | Splinter Review
1.13 KB, patch
tromey
: review+
Details | Diff | Splinter Review
10.57 KB, patch
Details | Diff | Splinter Review
3.03 KB, patch
Details | Diff | Splinter Review
16.29 KB, patch
Details | Diff | Splinter Review
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).
Severity: normal → enhancement
Priority: -- → P2
There are some devtool testings failed with the bug 1295456 css-color-4 implementation. I try to update our devtool to pass the testings.
Assignee: nobody → hshih
Status: NEW → ASSIGNED
This is the js version of css-color-4 color function implementation from bug 1295456.
Attachment #8798746 - Flags: review?(ttromey)
fix the wrong hsl() to rgba value.
Attachment #8798800 - Flags: review?(ttromey)
Attachment #8798746 - Attachment is obsolete: true
Attachment #8798746 - Flags: review?(ttromey)
The "rgb(24, 25, 45, 1)" is valid now. Turn to use "rgb(24, 25%, 45, 1)".
Attachment #8798801 - Flags: review?(ttromey)
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.
Attachment #8798800 - Flags: review?(ttromey)
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.
Attachment #8798747 - Flags: review?(ttromey) → review+
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.
Attachment #8798801 - Flags: review?(ttromey) → review+
(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.
Flags: needinfo?(ttromey)
(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.
Flags: needinfo?(ttromey)
update for comment 7.
I will add an argument for the color parser to accept the old style syntax only in next patch.
Attachment #8800068 - Flags: review?(ttromey)
Attachment #8798800 - Attachment is obsolete: true
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.
Attachment #8800115 - Flags: review?(ttromey)
Attachment #8800068 - Attachment is obsolete: true
Attachment #8800068 - Flags: review?(ttromey)
update hsl/rgb color function syntax in comment.
Attachment #8800246 - Flags: review?(ttromey)
Attachment #8800115 - Attachment is obsolete: true
Attachment #8800115 - Flags: review?(ttromey)
Attachment #8800353 - Attachment is obsolete: true
Attachment #8800353 - Flags: review?(ttromey)
update for lint failed.
Attachment #8800503 - Flags: review?(ttromey)
Attachment #8800246 - Attachment is obsolete: true
Attachment #8800246 - Flags: review?(ttromey)
Attachment #8800493 - Attachment is obsolete: true
Attachment #8800493 - Flags: review?(ttromey)
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.
Attachment #8800503 - Flags: review?(ttromey) → review+
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.
Attachment #8800504 - Flags: review?(ttromey)
fix the bug for the invalid tailing token.
Attachment #8800503 - Attachment is obsolete: true
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
Attachment #8801062 - Flags: review?(ttromey)
Attachment #8800504 - Attachment is obsolete: true
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!
Attachment #8801058 - Flags: review+
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.
Attachment #8801062 - Flags: review?(ttromey) → review+
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.
Attachment #8801064 - Flags: review?(ttromey) → review+
Attachment #8801062 - Attachment is obsolete: true
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.
Attachment #8801064 - Attachment is obsolete: true
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?
Flags: needinfo?(ttromey)
Attachment #8801058 - Attachment is obsolete: true
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
Keywords: checkin-needed
Sorry had to backed out this for lint failure, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=37754266&repo=mozilla-inbound#L4041
Flags: needinfo?(hshih)
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
Flags: needinfo?(hshih)
Blocks: 1310676
Blocks: 1310681
(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.
Flags: needinfo?(ttromey)
Depends on: 1347164
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.