1310681 - finish devtools support for css-color-4

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Tom Tromey :tromey]

css-color-4 is landing in platform in bug 1295456.
The minimal devtools support for this -- just enough changes to the color parsing
code to unbreak the tests -- landed in bug 1302787.

However, I think more work is needed.  Probably we should modify the
css-properties actor to determine if the target understands css-color-4
(or more precisely the subset implemented here...).  Then output-parser
and maybe other spots can be updated to parse colors correctly based on
this information.

Comment 1

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

How about check the particular css-color-4 color value (e.g. "rgb(255 255 255 / 100%)") using DOMUtils::IsValidCSSColor()[1] function in output-parser initial function? If the DOMUtils says yes, pass "false" to the second argument of colorUtils.colorToRGBA() in output-parse.

[1]
https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/css-properties.js#10

Comment 2

[Tom Tromey :tromey]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #1)
> How about check the particular css-color-4 color value (e.g. "rgb(255 255
> 255 / 100%)") using DOMUtils::IsValidCSSColor()[1] function in output-parser
> initial function? If the DOMUtils says yes, pass "false" to the second
> argument of colorUtils.colorToRGBA() in output-parse.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/css-
> properties.js#10

Yes, as long as it's done on the server and sent back.
Currently the inspector has a couple of cases where properties of the target
(what you're inspecting) are determined on the client (the inspector itself);
but these are errors and we don't want to add more of them.

Comment 3

[Tom Tromey :tromey]

Attachment: Bug 1310681 - do not use devtools colorUtils in browser

Review commit: https://reviewboard.mozilla.org/r/86712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86712/

Comment 4

[Tom Tromey :tromey]

Attachment: Bug 1310681 - start devtools UI work for css-color-4

Review commit: https://reviewboard.mozilla.org/r/86714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86714/

Comment 5

[Tom Tromey :tromey]

Here are my WIP patches.

The first one insulates the browser code from any devtools changes.

The second one is the main part of this bug.
It still needs some work:

* Add a parameter to setAlpha in color.js, and audit other code as well
* Try it out (I haven't run any of it yet)
* Update some tests.
* Decide how to handle shift-clicking on the color swatches.
  One valid choice would be to just leave the as-is, since using css 3
  in this case can't hurt.

Comment 6

[Patrick Brosset <:pbro>]

When this bug is done, we'll want to create a demo for it. This will help blogging/tweeting about it and write documentation on MDN.
I've opened an issue on our demo repo for this: https://github.com/mozdevs/devtools-demos/issues/2
Could someone comment on the issue to list which format exactly are going to be supported in devtools with this bug?

Comment 7

[Patrick Brosset <:pbro>]

Triaging like parent bug 1302787 as an enhancement P2.
(filter on CLIMBING SHOES).

Comment 8

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

Attachment: Bug 1310681 - do not use devtools colorUtils in browser.

Review commit: https://reviewboard.mozilla.org/r/87752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87752/

Comment 9

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

Attachment: Bug 1310681 - put css-color-4 color function supporting information into css property db.

Review commit: https://reviewboard.mozilla.org/r/87754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87754/

Comment 10

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

Attachment: Bug 1310681 - pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip.

Review commit: https://reviewboard.mozilla.org/r/87756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87756/

Comment 11

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

Attachment: Bug 1310681 - update devtool color function test for colorUtils.colorToRGBA default argument changing.

Turn to use colorUtils.setCssColor4Mode() api.

Review commit: https://reviewboard.mozilla.org/r/87758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87758/

Comment 12

[Tom Tromey :tromey]

Comment on attachment 8803516 [details]
Bug 1310681 - do not use devtools colorUtils in browser.

https://reviewboard.mozilla.org/r/87752/#review86730

I don't think I can review this one.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8803516 [details]
Bug 1310681 - do not use devtools colorUtils in browser.

https://reviewboard.mozilla.org/r/87752/#review86734

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8803517 [details]
Bug 1310681 - put css-color-4 color function supporting information into css property db.

https://reviewboard.mozilla.org/r/87754/#review86740

Thank you.  I really like your treatment of this patch.

Comment 15

[Tom Tromey :tromey]

Comment on attachment 8803518 [details]
Bug 1310681 - pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip.

https://reviewboard.mozilla.org/r/87756/#review86742

Thank you for doing this.  I think this one needs to be done differently.

::: devtools/shared/css/color.js:1062
(Diff revision 1)
> - * @param {Boolean} oldColorFunctionSyntax use old color function syntax or the
> - *        css-color-4 syntax
>   * @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) {
> +function colorToRGBA(name) {

I'd prefer passing a flag to the various functions that need it, rather than having a global that affects the entire module.  A global variable is more difficult to reason about; and in this case there aren't very many functions that need to be updated.

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8803519 [details]
Bug 1310681 - update devtool color function test for colorUtils.colorToRGBA default argument changing.

https://reviewboard.mozilla.org/r/87758/#review86746

Thank you.  I'm going to r+ this, but I think if the third patch is changed then perhaps this one won't need to be.

For the overall series, I'd like it if you could add an integration test of a css-color-4 color.  There is probably an existing rule view test where you can just add one more line.

Comment 17

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

Comment on attachment 8803517 [details]
Bug 1310681 - put css-color-4 color function supporting information into css property db.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87754/diff/1-2/

Comment 18

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

Comment on attachment 8803518 [details]
Bug 1310681 - pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87756/diff/1-2/

Comment 19

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

Comment on attachment 8803519 [details]
Bug 1310681 - update devtool color function test for colorUtils.colorToRGBA default argument changing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87758/diff/1-2/

Comment 20

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

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #19)
> Comment on attachment 8803519 [details]
> Bug 1310681 - update devtool color function test for colorUtils.colorToRGBA
> default argument changing.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/87758/diff/1-2/

I don't know why this carries the previous r+. This is for the updating of attachment 8803518 [details] and it's the new reviewing quest.


> For the overall series, I'd like it if you could add an integration test of
> a css-color-4 color.  There is probably an existing rule view test where you
> can just add one more line.

I'm not sure whether we have this kind of test or not. I will try to find out.

Comment 21

[Tom Tromey :tromey]

Comment on attachment 8803518 [details]
Bug 1310681 - pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip.

https://reviewboard.mozilla.org/r/87756/#review88924

Thanks.  This looks good, but I think a couple of spots in the color swatch will need to be updated:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js#162

Comment 22

[Tom Tromey :tromey]

Comment on attachment 8803519 [details]
Bug 1310681 - update devtool color function test for colorUtils.colorToRGBA default argument changing.

https://reviewboard.mozilla.org/r/87758/#review88926

Thanks, this still looks good, with one minor nit.

::: devtools/client/shared/test/unit/test_cssColor-03.js:53
(Diff revisions 1 - 2)
>  
> -  // turn off css-color-4 color function
> -  colorUtils.setCssColor4Mode(false);
>    for (let test of CSS_COLOR_4_TESTS) {
> -    let oursOld = colorUtils.colorToRGBA(test);
> +    let oursOld = colorUtils.colorToRGBA(test, false);
> +    let oursNew= colorUtils.colorToRGBA(test, true);

This should have a space before the "=".  I'd imagine eslint should note this.

Comment 23

[Tom Tromey :tromey]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #20)

> I don't know why this carries the previous r+. 

They carry over by default if the patch identity hasn't changed.
I think you can re-request review in reviewboard by clicking on the
little pencil icon.

Comment 24

[Tom Tromey :tromey]

Hi, what's going on with this bug?  It is very close to landing!  But it's
been a while since the last update... if you've stopped working on it, please
let me know, and I will fix up the remaining nit and run it through try.

Comment 25

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

Thanks for your reminding. I will switch to do this today.

Comment 26

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

Comment on attachment 8803516 [details]
Bug 1310681 - do not use devtools colorUtils in browser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87752/diff/1-2/

Comment 27

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

Comment on attachment 8803517 [details]
Bug 1310681 - put css-color-4 color function supporting information into css property db.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87754/diff/2-3/

Comment 28

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

Comment on attachment 8803518 [details]
Bug 1310681 - pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87756/diff/2-3/

Comment 29

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

Comment on attachment 8803519 [details]
Bug 1310681 - update devtool color function test for colorUtils.colorToRGBA default argument changing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87758/diff/2-3/

Comment 30

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

new review request!

Comment 31

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

Comment on attachment 8803518 [details]
Bug 1310681 - pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip.

https://reviewboard.mozilla.org/r/87756/#review100552

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:36
(Diff revision 3)
> + * @param {Function} supportsCssColor4ColorFunction
> + *        A function for checking the supporting of css-color-4 color function.
>   */
> -function SwatchColorPickerTooltip(document, inspector) {
> +function SwatchColorPickerTooltip(document,
> +                                  inspector,
> +                                  {supportsCssColor4ColorFunction} ) {

I will update this format problem later.
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:36:68 | There should be no spaces inside this paren. (space-in-parens)

Comment 32

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

trt link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3963bd9998be176fce220d951b684a7b21ac8e34&selectedJob=33188758

Comment 33

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

Comment on attachment 8803518 [details]
Bug 1310681 - pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip.

https://reviewboard.mozilla.org/r/87756/#review100610

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:34
(Diff revision 3)
>   * @param {InspectorPanel} inspector
>   *        The inspector panel, needed for the eyedropper.
> + * @param {Function} supportsCssColor4ColorFunction
> + *        A function for checking the supporting of css-color-4 color function.
>   */
> -function SwatchColorPickerTooltip(document, inspector) {
> +function SwatchColorPickerTooltip(document,

Pass the css-color-4 supporting status to this colorPicker.

Comment 34

[Tom Tromey :tromey]

Comment on attachment 8803518 [details]
Bug 1310681 - pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip.

https://reviewboard.mozilla.org/r/87756/#review101088

Thank you very much.

Comment 35

[Tom Tromey :tromey]

Clearing the NI.

Comment 36

[Tom Tromey :tromey]

Comment on attachment 8803516 [details]
Bug 1310681 - do not use devtools colorUtils in browser.

https://reviewboard.mozilla.org/r/87752/#review101090

Thank you.  I think this one was already reviewed by :kmag, but bugzilla seems to have flagged me as well, and I'm happy to give my r+.

Comment 37

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

Comment on attachment 8803516 [details]
Bug 1310681 - do not use devtools colorUtils in browser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87752/diff/2-3/

Comment 38

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

Comment on attachment 8803517 [details]
Bug 1310681 - put css-color-4 color function supporting information into css property db.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87754/diff/3-4/

Comment 39

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

Comment on attachment 8803518 [details]
Bug 1310681 - pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87756/diff/3-4/

Comment 40

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

Comment on attachment 8803519 [details]
Bug 1310681 - update devtool color function test for colorUtils.colorToRGBA default argument changing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87758/diff/3-4/

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4acb07675f61
do not use devtools colorUtils in browser. r=kmag,tromey
https://hg.mozilla.org/integration/autoland/rev/9964687bc8fb
put css-color-4 color function supporting information into css property db. r=tromey
https://hg.mozilla.org/integration/autoland/rev/a6e4c1a7cc2b
pass css-color-4 color function supporting info to devtool css OutputParser and SwatchColorPickerTooltip. r=tromey
https://hg.mozilla.org/integration/autoland/rev/b91e1c0f1960
update devtool color function test for colorUtils.colorToRGBA default argument changing. r=tromey

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4acb07675f61
https://hg.mozilla.org/mozilla-central/rev/9964687bc8fb
https://hg.mozilla.org/mozilla-central/rev/a6e4c1a7cc2b
https://hg.mozilla.org/mozilla-central/rev/b91e1c0f1960

Comment 43

[Gordon P. Hemsley [:GPHemsley]]

Comment on attachment 8803517 [details]
Bug 1310681 - put css-color-4 color function supporting information into css property db.

https://reviewboard.mozilla.org/r/87754/#review121298

::: devtools/server/actors/css-properties.js:35
(Diff revision 4)
>    getCSSDatabase() {
>      const properties = generateCssProperties();
>      const pseudoElements = DOMUtils.getCSSPseudoElementNames();
> +    const supportedFeature = {
> +      // checking for css-color-4 color function support.
> +      "css-color-4-color-function": DOMUtils.isValidCSSColor("rgb(1 1 1 / 100%"),

Apologies for the drive-by comment, but... is this missing a closing parenthesis?

Comment 44

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

Attachment: fix the missed closing parenthesis. r=me

MozReview-Commit-ID: Crsog4XJhW0

Comment 45

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

Thanks, Gordon.
I send a new try for the missed closing parenthesis.

Comment 46

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

The try for the missed closing parenthesis:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46684ea7aa23057622feb8460d2e38bac9549153

Comment 47

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

The new try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed5afeccc04d7c8614c881b01af45f60d68a240a

Comment 48

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

Please land the attachment 8846753 [details] [diff] [review] to m-c.
It's just a trivial fix.
try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed5afeccc04d7c8614c881b01af45f60d68a240a

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1697080a4726
fix the missed closing parenthesis. r=me

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1697080a4726