Closed Bug 1310681 Opened 8 years ago Closed 7 years ago

finish devtools support for css-color-4

Categories

(DevTools :: Inspector, enhancement, P2)

enhancement

Tracking

(firefox53 fixed, firefox55 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed
firefox55 --- fixed

People

(Reporter: tromey, Assigned: jerry)

References

Details

Attachments

(5 files, 2 obsolete files)

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)
(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)
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
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
Attachment #8802287 - Attachment is obsolete: true
Attachment #8802286 - Attachment is obsolete: true
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)
Attachment #8803516 - Flags: review?(kmaglione+bmo)
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+
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+
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-
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+
(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)
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)
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.
(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)
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)
new review request!
Flags: needinfo?(ttromey)
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 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 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+
Clearing the NI.
Flags: needinfo?(ttromey)
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+
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 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 → ---
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1697080a4726
fix the missed closing parenthesis. r=me
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1697080a4726
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: