Closed Bug 911748 Opened 7 years ago Closed 7 years ago

Add default color dropdown to devtools options panel

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file, 1 obsolete file)

Default should be hex colors
Attached patch Patch (obsolete) — Splinter Review
When this lands we can also close bug 706427 & bug 775135
Attachment #804409 - Flags: review?(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?
Attachment #804409 - Flags: review?(jwalker) → review+
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.
Disabled test on Linux until bug 916544 is fixed:

Try:
https://tbpl.mozilla.org/?tree=Try&rev=bb8891ff75a9
Attachment #804409 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/e4e9ca3868d3
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e4e9ca3868d3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
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.
Depends on: 923884
There should be an as authored option
(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
Depends on: 926283
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.