finish devtools support for css-color-4

RESOLVED FIXED in Firefox 53

Status

P2
enhancement
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: tromey, Assigned: jerry)

Tracking

unspecified
Firefox 53

Firefox Tracking Flags

(firefox53 fixed, firefox55 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
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
Flags: needinfo?(ttromey)
(Reporter)

Comment 2

3 years ago
(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.
Flags: needinfo?(ttromey)
(Reporter)

Updated

3 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

3 years ago
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.
Assignee: ttromey → hshih
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?
Triaging like parent bug 1302787 as an enhancement P2.
(filter on CLIMBING SHOES).
Severity: normal → enhancement
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

3 years ago
Attachment #8802287 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8802286 - Attachment is obsolete: true
(Reporter)

Comment 12

3 years ago
mozreview-review
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.
Attachment #8803516 - Flags: review?(ttromey)
(Reporter)

Updated

3 years ago
Attachment #8803516 - Flags: review?(kmaglione+bmo)

Comment 13

3 years ago
mozreview-review
Comment on attachment 8803516 [details]
Bug 1310681 - do not use devtools colorUtils in browser.

https://reviewboard.mozilla.org/r/87752/#review86734
Attachment #8803516 - Flags: review?(kmaglione+bmo) → review+
(Reporter)

Comment 14

3 years ago
mozreview-review
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.
Attachment #8803517 - Flags: review?(ttromey) → review+
(Reporter)

Comment 15

3 years ago
mozreview-review
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.
Attachment #8803518 - Flags: review?(ttromey) → review-
(Reporter)

Comment 16

3 years ago
mozreview-review
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.
Attachment #8803519 - Flags: review?(ttromey) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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.
Flags: needinfo?(ttromey)
(Reporter)

Comment 21

2 years ago
mozreview-review
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
Attachment #8803518 - Flags: review?(ttromey)
(Reporter)

Comment 22

2 years ago
mozreview-review
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.
(Reporter)

Comment 23

2 years ago
(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.
Flags: needinfo?(ttromey)
(Reporter)

Comment 24

2 years ago
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.
Flags: needinfo?(hshih)
Thanks for your reminding. I will switch to do this today.
Flags: needinfo?(hshih)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
new review request!
Flags: needinfo?(ttromey)
(Assignee)

Comment 31

2 years ago
mozreview-review
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)
(Assignee)

Comment 33

2 years ago
mozreview-review
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.
(Reporter)

Comment 34

2 years ago
mozreview-review
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.
Attachment #8803518 - Flags: review?(ttromey) → review+
(Reporter)

Comment 35

2 years ago
Clearing the NI.
Flags: needinfo?(ttromey)
(Reporter)

Comment 36

2 years ago
mozreview-review
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+.
Attachment #8803516 - Flags: review?(ttromey) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 41

2 years ago
Pushed by hshih@mozilla.com:
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

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Comment 43

2 years ago
mozreview-review
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?
MozReview-Commit-ID: Crsog4XJhW0
Thanks, Gordon.
I send a new try for the missed closing parenthesis.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 49

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1697080a4726
fix the missed closing parenthesis. r=me
Keywords: checkin-needed

Comment 50

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1697080a4726
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.