1478152 - Style current color picker to match new design

Status: VERIFIED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Micah [:mtigley] (she/her)]

Attachment: ColorPicker.png

This task will take the existing color picker to match the new design by Victoria: https://mozilla.invisionapp.com/share/CGMV7JMRV36#/screens 

Specifically, we only want to show the picker, alpha/hue sliders, color preview, and the eyedropper tool components of the design. Please see attached screenshot.

Comment 1

[Micah [:mtigley] (she/her)]

Attachment: Bug 1478152 - Style current color picker to match new design.

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

Comment 2

[Micah [:mtigley] (she/her)]

Attachment: old_vs_new_design.png

The changes for this bug are as follows:

- Both the alpha and hue sliders are both placed horizontally.
- A preview component for the selected color has been added. When a new color is selected and/or its hue/alpha values are updated, the color preview should reflect this.

Please see attached image that compares the old design of the color picker to the new one for expected changes.

Comment 3

[Micah [:mtigley] (she/her)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

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

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

https://reviewboard.mozilla.org/r/259246/#review268172

Clearing review based on what we talked about today.

Comment 5

[Micah [:mtigley] (she/her)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

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

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

https://reviewboard.mozilla.org/r/259246/#review268676

::: devtools/client/shared/test/browser_spectrum.js:115
(Diff revision 3)
>  
>    s.rgb = [240, 32, 124, 0];
>    s.updateUI();
>    is(s.alphaSliderHelper.style.left, -(s.alphaSliderHelper.offsetWidth / 2) + "px",
>      "Alpha range UI has been updated again");
> +  ok(s.hueHelperOriginalPos !== s.hueSliderHelper.style.left,

This should be hueHelperOriginalPos

::: devtools/client/shared/widgets/Spectrum.js:42
(Diff revision 3)
>    this.element = parentEl.ownerDocument.createElementNS(XHTML_NS, "div");
>    this.parentEl = parentEl;
>  
>    this.element.className = "spectrum-container";
>    this.element.innerHTML = `
> -    <div class="spectrum-top">
> +    <div class="spectrum-color-container">

We actually don't need an extra container here and its styles can actually replace spectrum-container. I would even go as far to remove the spectrum-show styles as well.

::: devtools/client/shared/widgets/Spectrum.js:44
(Diff revision 3)
>  
>    this.element.className = "spectrum-container";
>    this.element.innerHTML = `
> -    <div class="spectrum-top">
> -      <div class="spectrum-fill"></div>
> -      <div class="spectrum-top-inner">
> +    <div class="spectrum-color-container">
> +      <section class="spectrum-color-picker">
> +          <div class="spectrum-color-picker-inner">

NIT: There is a double indent everything inside of this <section> element.

::: devtools/client/shared/widgets/Spectrum.js:44
(Diff revision 3)
>  
>    this.element.className = "spectrum-container";
>    this.element.innerHTML = `
> -    <div class="spectrum-top">
> -      <div class="spectrum-fill"></div>
> -      <div class="spectrum-top-inner">
> +    <div class="spectrum-color-container">
> +      <section class="spectrum-color-picker">
> +          <div class="spectrum-color-picker-inner">

I think you can also remove this inner container since it's just an absolutely positioned container and so as its outer container.

::: devtools/client/shared/widgets/Spectrum.js:55
(Diff revision 3)
> -        <div class="spectrum-hue spectrum-box">
> -          <div class="spectrum-slider spectrum-slider-control"></div>
> -        </div>
> +          </div>
> +      </section>
> +      <section class="spectrum-controls">
> +        <div class="spectrum-color-preview"></div>

We should also do something similar to spectrum-checker here so that when we change the alpha values and make it more transparent. We can still see a checkered circle pattern around the color preview. See Chrome's color picker as an example.

::: devtools/client/shared/widgets/Spectrum.js:56
(Diff revision 3)
> -          <div class="spectrum-slider spectrum-slider-control"></div>
> -        </div>
> +          </div>
> +      </section>
> +      <section class="spectrum-controls">
> +        <div class="spectrum-color-preview"></div>
> +          <div class="spectrum-slider-container">

This container doesn't need to be indented since the element above is its sibling. Everything below up to the last </div> actually has an extra tab.

::: devtools/client/shared/widgets/Spectrum.js:79
(Diff revision 3)
>  
> -  this.slider = this.element.querySelector(".spectrum-hue");
> +  // Define dragger for the spectrum color.
> -  this.slideHelper = this.element.querySelector(".spectrum-slider");
> -  Spectrum.draggable(this.slider, this.onSliderMove.bind(this));
> -
>    this.dragger = this.element.querySelector(".spectrum-color");

I am tempted to say we should convert all these querySelector to getElementById - there is a small perf win for this, but I think it would also help us parse the elements in the innerHTML definition since everything is inside of class names right now. This can be a follow up.

Comment 7

[Micah [:mtigley] (she/her)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

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

Comment 8

[Micah [:mtigley] (she/her)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

https://reviewboard.mozilla.org/r/259246/#review268676

> I am tempted to say we should convert all these querySelector to getElementById - there is a small perf win for this, but I think it would also help us parse the elements in the innerHTML definition since everything is inside of class names right now. This can be a follow up.

I can create a follow-up bug for this.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

https://reviewboard.mozilla.org/r/259246/#review269462

The padding

::: devtools/client/shared/widgets/Spectrum.js:74
(Diff revision 4)
>    this.onElementClick = this.onElementClick.bind(this);
>    this.element.addEventListener("click", this.onElementClick);
>  
>    this.parentEl.appendChild(this.element);
>  
> -  this.slider = this.element.querySelector(".spectrum-hue");
> +  // Define dragger for the spectrum color.

Maybe just simplify this to be

// Color spectrum dragger

::: devtools/client/shared/widgets/Spectrum.js:311
(Diff revision 4)
> +    // Create a border around the color preview if the luminance of the current rgba
> +    // tuple is great.
> +    const colorLuminance = colorUtils.calculateLuminance(this.rgb);
> +
> +    if (colorLuminance > 0.95) {
> +      this.colorPreview.style.border = `1px solid ${checkeredRgb}`;

Let's move this into the CSS stylesheet, and use a CSS variable that we can set here.

::: devtools/client/shared/widgets/Spectrum.js:313
(Diff revision 4)
> +    const colorLuminance = colorUtils.calculateLuminance(this.rgb);
> +
> +    if (colorLuminance > 0.95) {
> +      this.colorPreview.style.border = `1px solid ${checkeredRgb}`;
> +    } else {
> +      this.colorPreview.style.border = "0px";

We should default this to 1px solid transparent otherwise you can see the controls shift when the color preview is updated and the 1px solid border appears.

However, I actually think we should just default this to be 1px solid ${this.rgbCssString) and only have the grey checkeredRgb border when it's colorLuminance > 0.95 similar to chrome's.

::: devtools/client/shared/widgets/Spectrum.js:318
(Diff revision 4)
> +      this.colorPreview.style.border = "0px";
>      }
>  
> +    // Overlay the rgba color over a checkered image background.
> +    this.colorPreview.style.backgroundImage = `
> +      linear-gradient(

Let's move all of this into the CSS stylesheet and use a CSS variable that can be set from the JS.

::: devtools/client/shared/widgets/Spectrum.js:323
(Diff revision 4)
> +      linear-gradient(
> +        ${this.rgbCssString},
> +        ${this.rgbCssString}),
> +      linear-gradient(
> +        45deg,
> +        ${checkeredRgb} 26%,

Probably no need to make checkeredRgb a variable since it's a constant.

::: devtools/client/shared/widgets/Spectrum.js:335
(Diff revision 4)
> +        transparent 25%,
> +        transparent 75%,
> +        ${checkeredRgb} 75%)`;
> +  },
> +
> +  updateDraggerColor: function() {

s/updateDraggerColor/updateDraggerBackgroundColor

::: devtools/client/shared/widgets/Spectrum.js:337
(Diff revision 4)
> +        ${checkeredRgb} 75%)`;
> +  },
> +
> +  updateDraggerColor: function() {
> +    const flatColor = "rgb(" + this.rgbNoSatVal[0] + ", " + this.rgbNoSatVal[1] + ", " +
> +    this.rgbNoSatVal[2] + ")";

Add a single indent here.

::: devtools/client/shared/widgets/spectrum.css:6
(Diff revision 4)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #eyedropper-button {
>    margin-inline-start: 5px;

This is a bit of a gray area, but I think it looks better with margin-inline-end: 5px

Comment 10

[Micah [:mtigley] (she/her)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

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

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

https://reviewboard.mozilla.org/r/259246/#review269584

We are pretty close, but one thing we forgot about is the dark theme colors. I think we might need to do something about the rgb(204, 204, 204) color for the different themes and the border color for the dark theme should maybe just be transparent.

::: devtools/client/shared/test/browser_spectrum.js:76
(Diff revisions 4 - 5)
>    const colorPreviewValue = extractRgbaOverlayString(linearGradientStr);
>  
>    is(colorPreviewValue, rgbCssString, `Color preview should be ${rgbCssString}`);
>  
>    // Test if color preview has a border or not.
> -  const border = spectrum.colorPreview.style.border;
> +  const border = getComputedStyle(colorPreview, "--preview-border");

I think we should do getComputedStyle(colorPreview, "border"); to test the computed style of the border property.

::: devtools/client/shared/test/browser_spectrum.js:77
(Diff revisions 4 - 5)
>  
>    is(colorPreviewValue, rgbCssString, `Color preview should be ${rgbCssString}`);
>  
>    // Test if color preview has a border or not.
> -  const border = spectrum.colorPreview.style.border;
> -  const checkeredRgb = "rgb(204, 204, 204)";
> +  const border = getComputedStyle(colorPreview, "--preview-border");
> +  const borderColor = getComputedStyle(colorPreview, "--preview-border-color");

We should specify the expected borderColor instead of getting the computed style of the variable since we know for certain tests it will be a light grey border and elsewhere that will not be the case. So, we know the expected borderColor is either transparent or rgb(204, 204, 204)

::: devtools/client/shared/widgets/spectrum.css:71
(Diff revisions 4 - 5)
>  .spectrum-color-preview {
> +  --overlay-color: transparent;
> +  --preview-border-color: transparent;
> +  --checkered-background: rgb(204, 204, 204);
> +  --preview-border: 1px solid var(--preview-border-color);
> +  border: var(--preview-border);

I think border can be simplified to be

border: 1px solid var(--preview-border-color);

since --preview-border isn't being overwritten and is not reused more than once.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Please check with Victoria about the color picker should look in the dark theme.

Comment 13

[Micah [:mtigley] (she/her)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

https://reviewboard.mozilla.org/r/259246/#review269584

> I think we should do getComputedStyle(colorPreview, "border"); to test the computed style of the border property.

getComputerStyle won't work for shorthand CSS properties. I think we should check for just the color of the preview border, which is already stored as a CSS variable that we can access with getComputedStyle.

Comment 14

[Micah [:mtigley] (she/her)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259246/diff/5-6/

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

https://reviewboard.mozilla.org/r/259246/#review269610

::: devtools/client/shared/test/browser_spectrum.js:78
(Diff revisions 5 - 6)
>    const colorPreviewValue = extractRgbaOverlayString(linearGradientStr);
>  
> -  is(colorPreviewValue, rgbCssString, `Color preview should be ${rgbCssString}`);
> +  is(colorPreviewValue, expectedRgbCssString,
> +    `Color preview should be ${expectedRgbCssString}`);
>  
> -  // Test if color preview has a border or not.
> +  // Test if color preview's border color is correct.

This can be an info("Check the color preview's border color is as expected"). Comments where you start with test X can usually be in an info().

::: devtools/client/themes/variables.css:111
(Diff revision 6)
>    --theme-codemirror-gutter-background: #f4f4f4;
>    --theme-messageCloseButtonFilter: invert(0);
> -}
>  
> +  /* Color Widget */
> +  --theme-slider-control-background: #ffffff;

The variables in variables.css are for generic widgets and the general theming of the toolbox. It would make sense to move these colors here and prefix them with a --theme-X if it was reused in multiple places and affects the same attribute such as box-shadow-color, background.

Since color widget is very specific, we should move these styles into tooltips.css.

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Can you also comment on where the dark theme colors are coming from? Just so we have on the record the confirmed values and source.

Comment 17

[Micah [:mtigley] (she/her)]

I don't have any official designs for the dark theme colors, especially for the slider-background-color and box-shadow-color. The color for slider-background-color specifically comes from  --theme-tooltip-background, however.

Victoria and I also discussed having the tooltip match the background and foreground colors of the doorhanger panel. What do you think?

Comment 18

[Micah [:mtigley] (she/her)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259246/diff/6-7/

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8994723 [details]
Bug 1478152 - Style current color picker to match new design.

https://reviewboard.mozilla.org/r/259246/#review269700

::: devtools/client/shared/test/browser_spectrum.js:57
(Diff revision 7)
> +function getComputedStyle(element, property) {
> +  return window.getComputedStyle(element).getPropertyValue(property).trim();
> +}
> +
> +/**
> + * Helper method for testing color preview display.

Let's remove this comment.

::: devtools/client/shared/test/browser_spectrum.js:72
(Diff revision 7)
> +  const { colorPreview } = spectrum;
> +
> +  spectrum.updateUI();
> +
> +  // Extract the first rgba value from the linear gradient
> +  const linearGradientStr = getComputedStyle(colorPreview, "background-image");

We can store window.getComputedStyle(element) in a variable so we can avoid fetching the computed style again, and then remove the getComputedStyle helper.

::: devtools/client/shared/widgets/Spectrum.js:8
(Diff revision 7)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const EventEmitter = require("devtools/shared/event-emitter");
> +const { colorUtils } = require("devtools/shared/css/color.js");

Add a new line between the require and constants.

::: devtools/client/shared/widgets/spectrum.css:62
(Diff revision 7)
> +  flex-direction: column;
> +  margin: -1px;
> +}
> +
> +/**
> + * Spectrum color preview styles the color overlay over the checkered background image.

I am thinking we should reword this comment to be:

This styles the color preview and adds a checkered background overlay inside of it that can be manipulated using the --overlay-color variable.

::: devtools/client/shared/widgets/spectrum.css:68
(Diff revision 7)
> + * It initializes the overlay and preview border colors for manipulation inside the
> + * Spectrum.prototype.updateColorPreview method.
> + */
> +.spectrum-color-preview {
> +  --overlay-color: transparent;
> +  --preview-border-color: transparent;

Let's remove this variable too and use an extra CSS class to toggle the grey border-color.

.spectrum-color-preview.low-lumninace(sp?) that you can toggle in the JS.

::: devtools/client/shared/widgets/spectrum.css:69
(Diff revision 7)
> + * Spectrum.prototype.updateColorPreview method.
> + */
> +.spectrum-color-preview {
> +  --overlay-color: transparent;
> +  --preview-border-color: transparent;
> +  --checkered-background: rgb(204, 204, 204);

Change it to #ccc

::: devtools/client/shared/widgets/spectrum.css:69
(Diff revision 7)
> + * Spectrum.prototype.updateColorPreview method.
> + */
> +.spectrum-color-preview {
> +  --overlay-color: transparent;
> +  --preview-border-color: transparent;
> +  --checkered-background: rgb(204, 204, 204);

Let's also remove this variable as discussed.

::: devtools/client/shared/widgets/spectrum.css:75
(Diff revision 7)
> +  border: 1px solid var(--preview-border-color);
> +  border-radius: 50%;
> +  width: 27px;
> +  height: 27px;
> +  background-color: #fff;
> +  background-image: linear-gradient(

Can we can keep the same formatting of the background-image as .spectrum-checker so we can tell it's the same background-image style.

background-image: linear-gradient(45deg, #ccc 25%, transparent 25%, transparent 75%, #ccc 75%, #ccc),
    linear-gradient(45deg, #ccc 25%, transparent 25%, transparent 75%, #ccc 75%, #ccc);

::: devtools/client/themes/tooltips.css:19
(Diff revision 7)
>    --bezier-diagonal-color: #eee;
>    --bezier-grid-color: rgba(0, 0, 0, 0.2);
>    --onboarding-link-color: var(--theme-highlight-blue);
>    --onboarding-link-active-color: var(--blue-40);
> +
> +  /* Tooltip: Color Widget variables */

/* Color widget variables */

::: devtools/client/themes/variables.css:112
(Diff revision 7)
>  
>    --theme-codemirror-gutter-background: #f4f4f4;
>    --theme-messageCloseButtonFilter: invert(0);
> -}
>  
> +  /* Color Widget */

Remove the changes added in variables.css

Comment 20

[Micah [:mtigley] (she/her)]

Attachment: Bug1478152.patch

Uploading rebased patch for styling the color picker.

Comment 21

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Comment 22

[Micah [:mtigley] (she/her)]

Attachment: Bug1478152.patch

Rebasing old redesign patch.

Comment 23

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Maliha, assigning this to you, as it is already reviewed and is a blocker for the contrast work. I believe we want to rebase the patch on top of the latest master and submit phabricator review to Micah to confirm it looks good. Also we would want to make sure that devtools tests are passing on Try so it should be a good opportunity to go through that workflow.

Comment 24

[Maliha Islam [:mislam]]

Attachment: Bug 1478152 - Style current color picker to match new design, r=mtigley

Comment 25

[Maliha Islam [:mislam]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e28696fe4e9e89db302eeac29606cd166d0251b

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67b8391890d0
Style current color picker to match new design, r=mtigley

Comment 27

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/67b8391890d0

Comment 28

[Patrick Brosset <:pbro>]

Attachment: Screenshot 2019-05-21 at 14.55.58.png

Thanks for fixing this bug. The new design looks great.
Attaching this here for info.

Comment 29

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 69.0a1 (2019-05-21) and on 68.0b3 on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.