911748 - Add default color dropdown to devtools options panel

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Default should be hex colors

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

When this lands we can also close bug 706427 & bug 775135

Comment 2

[Joe Walker [:jwalker]]

Comment on attachment 804409 [details] [diff] [review]
Patch

Review of attachment 804409 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/commandline/test/browser_cmd_addon.js
@@ +102,5 @@
>            markup: 'VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV',
>            status: 'VALID'
>          },
>          exec: {
> +          output: 'Test Plug-in 1.0.0.0 is already disabled.'

This doesn't make sense. Why has this changed?

::: browser/devtools/shared/css-color.js
@@ +285,5 @@
> +    }
> +
> +    r /= 255;
> +    g /= 255;
> +    b /= 255;

I'm not sure the character saving is worth the readability loss is it?

@@ +289,5 @@
> +    b /= 255;
> +
> +    let max = Math.max(r, g, b);
> +    let min = Math.min(r, g, b);
> +    let h, s, l = (max + min) / 2;

Ditto, especially given destructuring, I think we should split this up

@@ +365,5 @@
> +function processCSSString(value) {
> +  if (value && /^""$/.test(value)) {
> +    return value;
> +  }
> +  let colorPattern = /#[0-9a-fA-F]{3}\b|#[0-9a-fA-F]{6}\b|hsl\(.*?\)|hsl\(.*?\)|rgba?\(.*?\)|\b[a-zA-Z-]+\b/gi;

[a-fA-F] and /i ?
Also could you document this?

::: browser/devtools/styleinspector/rule-view.js
@@ +1001,5 @@
>  
>    remove: function TextProperty_remove()
>    {
>      this.rule.removeProperty(this);
> +  },

I think it's good to do these types of tweaks, but could you keep them in a separate patch please?

::: browser/devtools/styleinspector/test/browser_ruleview_bug_902966_revert_value_on_ESC.js
@@ +58,2 @@
>  
> +    info("Current object: " + testData[index].toSource());

Did you mean to add this, or did it slip by?

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://tbpl.mozilla.org/?tree=Try&rev=0a64ebffbb5f

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We need to wait on bug 916544 here as sometimes setting an rgba value on a node and then reading it's computed style can return different values.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: color-options-911748.patch

Disabled test on Linux until bug 916544 is fixed:

Try:
https://tbpl.mozilla.org/?tree=Try&rev=bb8891ff75a9

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e4e9ca3868d3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4e9ca3868d3

Comment 8

[Paul Rouget [:paul]]

You can't define something in /browser/ and use it in /toolkit/ (which you do with css-color.js).

This broke the inspector on B2G.

Comment 9

[Paul Rouget [:paul]]

bug 918305

Comment 10

[Tim Nguyen :ntim]

There should be an as authored option

Comment 11

[Jan Willem Oostendorp]

(In reply to ntim007 from comment #10)
> There should be an as authored option

This bug is about the dropdown option itself.
The color formats and such are in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=775135
Not sure if it was discussed there, but that is the place to note that