1332090 - Add Contrast Ratio component to the Color Widget

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P3)

Description

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

Add a contrast ratio component to the color widget. This bug is reserved for UCOSP students.

Comment 1

[Rahul Chaudhary [:rahulc]]

Attachment: Bug1332090.patch

Initial working version for contrast ratio

Comment 2

[Rahul Chaudhary [:rahulc]]

Attachment: Bug1332090.patch

Functional version of color widget contrast ratio calculator

Comment 3

[Rahul Chaudhary [:rahulc]]

Attachment: Bug1332090.patch

Comment 4

[Rahul Chaudhary [:rahulc]]

Attachment: Bug1332090.patch

Added unit tests, please apply patch and test if the interface is working.

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8833092 [details] [diff] [review]
Bug1332090.patch

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

::: devtools/client/shared/widgets/ColorWidget.js
@@ +24,5 @@
> +function calculateLuminance(rgba) {
> +  /* based on https://www.w3.org/TR/2008/REC-WCAG20-20081211/#relativeluminancedef */
> +  for (let i = 0; i < 3; i++) {
> +    rgba[i] /= 255;
> +    rgba[i] = (rgba[i] < 0.03928) ? (rgba[i] / 12.92) : 

nit: trailing whitespace

@@ +38,5 @@
> +    ratio = (textLuminance + 0.05) / (backgroundLuminance + 0.05);
> +
> +  return (ratio > 1.0) ? ratio : (1 / ratio);
> +}
> +

I feel like calculateLuminance and calculateContrastRatio should be a colorUtils.

Not sure if @gl agrees.

@@ +92,5 @@
>        </div>
>      </div>
> +    <div class="colorwidget-contrast">
> +      <span class="colorwidget-contrast-message">
> +        Input background color to calculate contrast:

Not sure we need this message.

@@ +351,5 @@
> +          roundedRatio = roundNumber(ratio, 2),
> +          gradeAndTitle = {
> +            grade: "",
> +            title: ""
> +          };

That's quite an usual code style in moz-central

Can you use let ... = ...; for each var ?

@@ +364,5 @@
> +        gradeAndTitle = ColorWidget.contrastGrading.AAA;
> +      }
> +
> +      this.colorwidgetRatio.innerHTML = roundedRatio.toString() + ":1";
> +      this.colorwidgetGrade.innerHTML = gradeAndTitle.grade;

We tend to prefer using textContent when possible

@@ +373,5 @@
> +      this.colorwidgetSwatch.style.borderColor = this.rgbCssString;
> +
> +    } catch(err) {
> +      this.colorwidgetRatio.innerHTML = "Invalid color";
> +      this.colorwidgetGrade.innerHTML = "";

Same here.

@@ +435,5 @@
>      this.alphaSliderInner.style.background = alphaGradient;
> +    this.closestBackgroundColor = "rgba(0, 0, 0, 0)";
> +
> +    if (this.inspector && this.inspector.selection.nodeFront) {
> +        this.closestBackgroundColor = yield (this.inspector.selection

nit: use 2 space indent

@@ +438,5 @@
> +    if (this.inspector && this.inspector.selection.nodeFront) {
> +        this.closestBackgroundColor = yield (this.inspector.selection
> +                                  .nodeFront.getClosestBackgroundColor());
> +    }
> +    

nit: whitespace

Comment 6

[Rahul Chaudhary [:rahulc]]

Attachment: Bug1332090.patch

Made changes suggested by :ntim and eslint.

Comment 7

[Tim Nguyen :ntim]

Attachment: contrast-ratio-invisible.png

The patch doesn't seem to work locally for me. There's a huge blank space in the popup. Do I need any dependency ?

Comment 8

[Tim Nguyen :ntim]

Attachment: Screenshot of patch

Ah, looks like I needed devtools.inspector.colorWidget.enabled to be set to true.

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8833695 [details] [diff] [review]
Bug1332090.patch

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

About the UX, I do think we should just follow: https://projects.invisionapp.com/share/3T87OWLYW#/screens/143217180_Color_Picker

::: devtools/client/shared/widgets/ColorWidget.js
@@ +85,5 @@
>  
>    this.parentEl.appendChild(this.element);
> +  this.updateContrast = this.updateContrast.bind(this);
> +  this.colorwidgetContrastInput = this.element.querySelector(
> +    ".colorwidget-contrast-input");

I find this name confusing, at first read, it seems like an input that contains the contrast in it (which is not the case, since it contains the background).

@@ +90,5 @@
> +  this.colorwidgetContrastInput.addEventListener("input", this.updateContrast);
> +  this.colorwidgetSwatch = this.element.querySelector(".colorwidget-colorswatch");
> +  this.colorwidgetRatio = this.element.querySelector(".colorwidget-contrast-ratio");
> +  this.colorwidgetGrade = this.element.querySelector(".colorwidget-contrast-grade");
> +  this.colorwidgetInner = this.element.querySelector(".colorwidget-contrast-inner");

I would name them contrastSwatch, constrastRatio, constrastGrade, contrastInner for clarity.

@@ +116,5 @@
>  
> +ColorWidget.contrastGrading = {
> +  fail: {
> +    title: "This contrast ratio fails for all text sizes.",
> +    grade: "Fail"

Strings need to be localized (see devtools/client/locales/).

@@ +323,5 @@
> +      let rgba = rgbaColor._getRGBATuple();
> +      let backgroundColor = [rgba.r, rgba.g, rgba.b, rgba.a];
> +      let ratio = colorUtils.calculateContrastRatio(backgroundColor, textColor);
> +      let roundedRatio = roundNumber(ratio, 2);
> +      let gradeAndTitle = {};

I'd prefer contrastGrade as variable name.

@@ +336,5 @@
> +      } else if (roundedRatio >= 4.5 && roundedRatio < 7.0) {
> +        gradeAndTitle = ColorWidget.contrastGrading.AAABig;
> +      } else if (roundedRatio >= 7.0 && roundedRatio < 22) {
> +        gradeAndTitle = ColorWidget.contrastGrading.AAA;
> +      }

Hmm, it might be cleaner to do it like this:


let grade;
if (roundedRatio < 3.0) {
  grade = "fail";
} else if (...) {
  grade = "AABig";
} else if (...) {
  grade = "AAABig";
} else if (...) {
  grade = "AAA";
}

let strings = ColorWidget.contrastGrading[grade];

I would also split the part that gets the grading into a different function, something like getContrastGrade(background, text);

@@ +338,5 @@
> +      } else if (roundedRatio >= 7.0 && roundedRatio < 22) {
> +        gradeAndTitle = ColorWidget.contrastGrading.AAA;
> +      }
> +
> +      this.colorwidgetRatio.textContent = roundedRatio.toString() + ":1";

You don't need toString() since roundedRatio is a number (which JS automatically converts to a string with the + operator)

So:

roundedRatio + ":1";

or fancier:

`${roundedRatio}:1`;

@@ +347,5 @@
> +      this.colorwidgetSwatch.style.color = this.rgbCssString;
> +      this.colorwidgetSwatch.style.borderColor = this.rgbCssString;
> +    } catch (err) {
> +      this.colorwidgetRatio.textContent = "Invalid color";
> +      this.colorwidgetGrade.textContent = "";

Are we ever going to come across this case?

::: devtools/client/shared/widgets/color-widget.css
@@ +40,5 @@
> +}
> +
> +.colorwidget-colorswatch {
> +  background-color: rgba(0, 0, 0, 0);
> +  color: rgba(0, 0, 0, 0);

nit: Can you use transparent instead of rgba(...) ?

@@ +41,5 @@
> +
> +.colorwidget-colorswatch {
> +  background-color: rgba(0, 0, 0, 0);
> +  color: rgba(0, 0, 0, 0);
> +  border: 1px solid black;

You can use border: 1px solid currentColor; (or just transparent), to avoid updating the border color with JS.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js
@@ +73,5 @@
>       */
>      eyedropper.style.pointerEvents = "auto";
>      container.appendChild(eyedropper);
>  
> +    this.tooltip.setContent(container, { width: 218, height: 300 });

Seems like the new dimensions should only apply to the new widget (when devtools.inspector.colorWidget.enabled is enabled)

::: devtools/server/actors/inspector.js
@@ +773,5 @@
> +        // do nothing
> +      }
> +      current = current.walker.parentNode(current);
> +    }
> +    return "rgb(255, 255, 255)";

nit: Your function doc says it should return a rgba tuple, so this should be "rgba(255, 255, 255, 1)"

Comment 10

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

Comment on attachment 8833695 [details] [diff] [review]
Bug1332090.patch

I am gonna add pbro to get more feedback on functionality. I don't think we should be looking too deeply into the code since this is mainly about prototyping a feature. 

There are a couple of reasons we had to derivate from the original UX design. Some of the problems we wanted to address was how would the user know what the foreground color is being used to compare with the font color to calculate the contrast ratio. This is not obvious in every case because text can be positioned absolutely or an image could behind the text. We would only surface the Contrast Ratio bit for 'color' properties. We added the extra input fields to not only display the background color that is being used to calculate the contrast ratio, but to also allow for a more useful contrast ratio tool.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8833695 [details] [diff] [review]
Bug1332090.patch

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

I was also confused at first when devtools.inspector.colorWidget.enabled was false and the color picker was taller and showed plenty of whitespace at the bottom. But I see :ntim has already commented about that.

Below is a list of other problems I found. Sorry, I know this is a prototype for now, so probably not what you care about too much right now, but at least they're here and you can refer to them later:

- Text selection is invisible in the text color textfield. The caret is invisible, and if I select text with the mouse, the selection is invisible too.
- The "Background color:" and "Contrast: ..." labels are always black, even in the dark theme, which makes them hard to see.
- The size of the text is too big compared to the rest of DevTools.
- Even if I change the background color to another value, as soon as I change again the value in the color spectrum, the background color defaults back to white.

Now, in terms of the actual feature:

- It makes kind of sense to only show the color contrast widget for 'color' properties. You're usually concerned about checking this when setting the color of the text in an element. So I'm fine with doing this.
- I also think it is useful to show the background color that was used to calculate the contrast ratio.
- I would default this background color to the computed style.backgroundColor value of the current element. This will be the most common case that people want. In case its transparent/unset, then I would walk up the DOM until a parent element with a defined background color is found.
- In some cases, people will want to change this background color. But I really feel like this will be a minority of cases. So having a textfield just for this seems too much to me. People might wonder what this textfield is about and it might distract them from what they were trying to do in the first place: just pick a color.
Here's an idea: make the preview box clickable. When clicked, the eyedropper appears on the page, and the user can click on the color they want to use as the reference background color.
- Finally I would make the text preview box smaller, with just one letter, like this:
+---+
| A | 4.88:1 AAA'
+---+
This way the tooltip can be made to be the same size as it was before, the preview box and contrast value can simply be displayed next to the existing eyedropper button, along the bottom side.

I hope this was the kind of feedback you were looking for. I haven't reviewed any of the code changes yet. Let me know if you want me to.

Comment 12

[Tim Nguyen :ntim]

I agree with pbro's remarks. In addition to those, I would probably make the text spacing consistent, the main reason I preferred Helen's UX was that the text kept moving and it was hard to keep in track of the information. A monospace font and displaying a fixed number of digits (eg. always displaying 4.80 instead of 4.8) would greatly help.

I would also add a help icon next to AAA linking to MDN or to some kind of helpful link. Not everyone knows what AAA, or what 4.88:1 means.

Perhaps a simplified explanation of https://www.w3.org/TR/WCAG/#visual-audio-contrast would be enough.


Something like:

+---+
| A | 4.88:1  AAA'          (?)
+---+

Comment 13

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #12)
> I agree with pbro's remarks. In addition to those, I would probably make the
> text spacing consistent, the main reason I preferred Helen's UX was that the
> text kept moving and it was hard to keep in track of the information. A
I realize I wasn't clear here, the reason I preferred Helen's UX was that the text wasn't moving, and it was easier to see the information.

Comment 14

[Rahul Chaudhary [:rahulc]]

Attachment: Bug 1332090 - Add Contrast Ratio component to the Color Widget.

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

Comment 15

[Rahul Chaudhary [:rahulc]]

Attachment: Bug 1332090 - Add Contrast Ratio component to the Color Widget.

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

Comment 16

[Rahul Chaudhary [:rahulc]]

So based on the previous comments from ntim and pbro, gl and I have come up with a MVP of sorts for the contrast ratio component. 

The following items from the previous comments have been implemented and fixed in this MVP:
> - Text selection is invisible in the text color textfield. The caret is invisible, and if I select text with the mouse, the selection is invisible too.
> - The "Background color:" and "Contrast: ..." labels are always black, even in the dark theme, which makes them hard to see.
> - The size of the text is too big compared to the rest of DevTools.
> - Even if I change the background color to another value, as soon as I change again the value in the color spectrum, the background color defaults back to white.
> - It makes kind of sense to only show the color contrast widget for 'color' properties. You're usually concerned about checking this when setting the color of the text in an element. So I'm fine with doing this.
> - I also think it is useful to show the background color that was used to calculate the contrast ratio.
> - I would default this background color to the computed style.backgroundColor value of the current element. This will be the most common case that people want. In case its transparent/unset, then I would walk up the DOM until a parent element with a defined background color is found.
> - In some cases, people will want to change this background color. But I really feel like this will be a minority of cases. So having a textfield just for this seems too much to me. People might wonder what this textfield is about and it might distract them from what they were trying to do in the first place: just pick a color.

The code changes from ntim's feedback have been added in, along with fixed width for the grade and ratio so they don't move around.

The next steps are to add the following from ntim's and pbro's previous comments:
> Here's an idea: make the preview box clickable. When clicked, the eyedropper appears on the page, and the user can click on the color they want to use as the reference background color.
> A monospace font and displaying a fixed number of digits (eg. always displaying 4.80 instead of 4.8) would greatly help.
> A (?) icon next to the contrast ratio that tells explains the contrast grading system and tells you that the preview box is clickable.

For now an explanation of the grading is available in the title when you hover over the contrast area, so move the explanation from the title it to a tool-tip that shows up when the (?) icon is clicked on/hovered over.

Comment 17

[Tim Nguyen :ntim]

I've noticed a small issue when testing: the text isn't visible with the dark theme. 

Looks great otherwise!

Comment 18

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #17)
> I've noticed a small issue when testing: the text isn't visible with the
> dark theme.
Hmm, I was missing the other review request that fixed this.

Comment 19

[Patrick Brosset <:pbro>]

Attachment: colorpicker-design-comments.png

I tested the last version of the patches locally. I like what I'm seeing!
I'm also looking forward to testing again when the next steps described in comment 16 have been implemented.

I also have a comments about the general design of the color picker tooltip: I think it could use some adjustments and fine-tuning. I don't know if you intend to do them here or in a separate bug, but I do think they should be done before turning this new version of the tooltip ON by default. Please see the screenshot. Here are a few specific comments:
- the select box is wider than the textfield below it, but it seems like they should have the same size
- the fontsize isn't consistent through the tooltip
- the style if the textfield and the select box isn't consistent with the rest of devtools
- the eyedropper icon is too far down, it touches the bottom border of the tooltip without respecting the padding.

Anyway, those are just little adjustments we should make at some point to make sure using the tooltip feels great.

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8839311 [details]
Bug 1332090 - Add Contrast Ratio component to the Color Widget.

https://reviewboard.mozilla.org/r/113980/#review118284

Good work! Thanks for the patch.
Please see my comments below.
Also, see comment 19. I do think there's a bunch of work to be done to make sure this new part of the color picker looks a bit nicer.
What is our strategy there? Do it later in a separate bug? Or now before landing?

::: devtools/client/locales/en-US/inspector.properties:351
(Diff revision 1)
>  # LOCALIZATION NOTE (inspector.eyedropper.label): A string displayed as the tooltip of
>  # a button in the inspector which toggles the Eyedropper tool
>  inspector.eyedropper.label=Grab a color from the page
>  
> +# LOCALIZATION NOTE (inspector.colorwidget.contrastRatioInfo):
> +# This string is used as a header to indicate the contrast section of the 

nit: There's a trailing space at the end of this line.

::: devtools/client/locales/en-US/inspector.properties:353
(Diff revision 1)
>  inspector.eyedropper.label=Grab a color from the page
>  
> +# LOCALIZATION NOTE (inspector.colorwidget.contrastRatioInfo):
> +# This string is used as a header to indicate the contrast section of the 
> +# color widget.
> +inspector.colorwidget.contrastRatioInfo=Contrast Ratio

All other strings your added have the following format:
`inspector.colorwidget.contrastRatio.<something>`
so I suggest using the same format for this string too. In this case `<something>` could be `header`

::: devtools/client/locales/en-US/inspector.properties:356
(Diff revision 1)
> +# This string is used as a header to indicate the contrast section of the 
> +# color widget.
> +inspector.colorwidget.contrastRatioInfo=Contrast Ratio
> +
> +# LOCALIZATION NOTE (inspector.colorwidget.contrastRatio.info):
> +# This string is used to explain the contrast ratio grading system.

Can you explain where is the string going to be displayed in the UI? Is it in a tooltip?

::: devtools/client/locales/en-US/inspector.properties:357
(Diff revision 1)
> +# color widget.
> +inspector.colorwidget.contrastRatioInfo=Contrast Ratio
> +
> +# LOCALIZATION NOTE (inspector.colorwidget.contrastRatio.info):
> +# This string is used to explain the contrast ratio grading system.
> +inspector.colorwidget.contrastRatio.info=The contrast ratio grading system for text has the following grading: Fail, AA*, AAA* AAA from lowest to highest readability.\nIt was calculated based on the computed background color: 

nit: There's a trailing space at the end of this line.

::: devtools/client/shared/widgets/ColorWidget.js:96
(Diff revision 1)
>        </div>
>      </div>
> +    <div class="colorwidget-contrast">
> +      <div class="colorwidget-contrast-info"></div>
> +      <div class="colorwidget-contrast-inner">
> +        <span class="colorwidget-colorswatch">Abc</span>

Let's move the sample test (`Abc`) at least to a constant at the top of this file.
This way it makes it easier to change it.
In fact, I'm wondering if we should even localize this string. Abc might not be the best in some languages.

::: devtools/client/shared/widgets/ColorWidget.js:172
(Diff revision 1)
> +  let contrastGrade = "";
> +  let contrastTitle = "";

I think you could use simpler variable names here: `grade` and `title`.

::: devtools/client/shared/widgets/ColorWidget.js:192
(Diff revision 1)
> +  return {
> +    grade: contrastGrade,
> +    title: contrastTitle
> +  };

And if you do, you can then simply return like this:
`return { grade, title };`

::: devtools/client/shared/widgets/ColorWidget.js:509
(Diff revision 1)
> +  updateContrast: function () {
> +    if (!this.contrastEnabled) {
> +      this.contrast.style.display = "none";
> +      return;
> +    }
> +    try {

What is this try/catch block supposed to prevent exactly? It looks like there is a lot of code in the try part. It would be good to try and reduce that a bit so we don't blindly catch things that should be fixed and would instead silently fail here.

::: devtools/client/shared/widgets/ColorWidget.js:513
(Diff revision 1)
> +    }
> +    try {
> +      this.contrast.style.display = "initial";
> +
> +      let rgbaColor = new colorUtils.CssColor(this.closestBackgroundColor);
> +      let rgba = rgbaColor._getRGBATuple();

Let's make `_getRGBATuple` a public method. It is very useful, and should not only be used internally inside the CssColor class.
So, there is no reason it should have a `_` in its name.

::: devtools/client/shared/widgets/ColorWidget.js:531
(Diff revision 1)
> +      this.contrastInner.setAttribute("title", contrastDetails.title);
> +
> +      this.contrastSwatch.style.backgroundColor = rgbaColor.toString();
> +      this.contrastSwatch.style.color = this.rgbCssString;
> +    } catch (err) {
> +      this.contrastRatio.textContent = "Invalid color";

This string needs to be localized instead of being hardcoded here. So you should add a new key in the properties file and retrieve its value here with the L10N helper.

::: devtools/client/shared/widgets/ColorWidget.js:611
(Diff revision 1)
> +      this.closestBackgroundColor = yield (this.inspector.selection
> +                                  .nodeFront.getClosestBackgroundColor());

The formatting makes it a bit hard to read:

```
this.closestBackgroundColor = 
  yield this.inspector.selection.nodeFront.getClosestBackgroundColor();
```

Or, if this is too long to fit on one line:

```
let node = this.inspector.selection.nodeFront;
this.closestBackgroundColor = yield node.getClosestBackgroundColor();
```

::: devtools/client/shared/widgets/ColorWidget.js:612
(Diff revision 1)
>      this.alphaSliderInner.style.background = alphaGradient;
> -  },
>  
> +    if (this.inspector && this.inspector.selection.nodeFront && this.contrastEnabled) {
> +      this.closestBackgroundColor = yield (this.inspector.selection
> +                                  .nodeFront.getClosestBackgroundColor());

getClosestBackgroundColor is a new NodeActor method that did not exist before. However, DevTools can be connected to any browser or device, including targets that run an older version of the NodeActor.
So, you need to handle some form of backward compatibility here, to avoid this from failing if the method does not exist.

I think the right thing to do here is to make `this.contrastEnabled` be false if the backend doesn't have the method. This way we just hide the whole thing.

To check if an actor has a given method, you can do something similar to what is done here:
http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/devtools/client/inspector/inspector.js#633-634

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:110
(Diff revision 1)
>      if (this.activeSwatch) {
>        this.currentSwatchColor = this.activeSwatch.nextSibling;
>        this._originalColor = this.currentSwatchColor.textContent;
>        let color = this.activeSwatch.style.backgroundColor;
>        this.spectrum.off("changed", this._onSpectrumColorChange);
> +      this.spectrum.contrastEnabled = this.activeSwatch.name === "color";

This is where we could use the `actorHasMethod` thing I talked about before to set the value of `contractEnabled` to false if the NodeActor doesn't have the new method you added.

::: devtools/server/actors/inspector.js:769
(Diff revision 1)
> +        let currentColor = currentCssColor._getRGBATuple();
> +        if (currentColor.a > 0) {
> +          return currentCssColor.rgba;
> +        }

It might be useful to move this logic into the CssColor class instead of have it here, inside a new method:

```
let currentCssColor = new colorUtils.CssColor(currentStyle);
if (!currentCssColor.isTransparent()) {
  return currentCssColor.rgba;
}
```

A bit easier to read.

::: devtools/server/actors/inspector.js:774
(Diff revision 1)
> +        let currentColor = currentCssColor._getRGBATuple();
> +        if (currentColor.a > 0) {
> +          return currentCssColor.rgba;
> +        }
> +      } catch (err) {
> +        // do nothing

Either try to make the try contain less code so it's obvious what we expect to be failing or, if not possible, explain in this comment when we think this fails and why.

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8839321 [details]
Bug 1332090 - Add Contrast Ratio component to the Color Widget.

https://reviewboard.mozilla.org/r/113994/#review118362

Please merge this commit with the previous one. I don't think they need to be separated.
Having said this, no problem with the code changes here. So R+ on this one, but please do merge it.

Comment 22

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8839311 [details]
Bug 1332090 - Add Contrast Ratio component to the Color Widget.

https://reviewboard.mozilla.org/r/113980/#review118284

As part of comment 19 the following points were mentioned:
- the select box is wider than the textfield below it, but it seems like they should have the same size
- the fontsize isn't consistent through the tooltip
- the style if the textfield and the select box isn't consistent with the rest of devtools
- the eyedropper icon is too far down, it touches the bottom border of the tooltip without respecting the padding.

I believe the select box and the text field are both part of a different bug on the color widget, and in general the ColorWidget syling is inconsistent.
I spoke to Gabriel about this, and a bug is currently WIP where the plan is to rearrange the color widget to make it look better - and we plan to create a new bug to make the styling of the widget consistent throughout.

Comment 23

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8839311 [details]
Bug 1332090 - Add Contrast Ratio component to the Color Widget.

https://reviewboard.mozilla.org/r/113980/#review118284

> Can you explain where is the string going to be displayed in the UI? Is it in a tooltip?

Currently it shows up when you hover over the contrast ratio, I plan to move it to the (?) icon mentioned in comment 16. I shall add this information to the comment.

> nit: There's a trailing space at the end of this line.

The trailing space is currently intentional, so there is a space between the string and the computed background color.
If this is bad coding style, and I should just do `L10N.getStr(....) + " " + computedBackgroundColor`, please let me know. If not I will just add the fact that the space at the end is intentional onto the comment.

> Let's move the sample test (`Abc`) at least to a constant at the top of this file.
> This way it makes it easier to change it.
> In fact, I'm wondering if we should even localize this string. Abc might not be the best in some languages.

That is true. I am wondering if string localization converting Abc to a different language will be a problem, since we just want to display some sample text.

Comment 24

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8839311 [details]
Bug 1332090 - Add Contrast Ratio component to the Color Widget.

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

Comment 25

[Rahul Chaudhary [:rahulc]]

Attachment: Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

Added details in comments about next steps and discussion with :gl.

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

Comment 26

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8846869 [details]
Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

https://reviewboard.mozilla.org/r/119874/#review121750

::: commit-message-3d341:4
(Diff revision 1)
> +Bug 1332090 - Add Contrast Ratio component to the Color Widget. r?pbro
> +
> +Added details in comments about next steps and discussion with :gl.
> +

Added:
> A monospace font and displaying a fixed number of digits for contrast ratio and grade.
> A (?) icon next to the contrast ratio that explains the contrast grading system briefly on hover (in it's title).
> Addresed last review of code.

Next Steps:
> Finish code review on MVP of the contrast ratio.
> Fix non-uniform styling of color widget.
> Fix eyedropper bug mentioned below and an eyedropper to the color swatch.

Discussion:
Gabriel and I discussed
> the non-uniform styling of the color widget and we decided to do it in a separate bug, since some of the other features in the new color widget themselves aren't uniform between themselves either, and we plan to move around the items color widget in bug 1332086.

https://bugzilla.mozilla.org/show_bug.cgi?id=1332086

> the feature to add an eyedropper on the swatch to select background color, which is blocked by the colorwidget closing when the eyepicker is used (currently being addressed in bug 1332872).

https://bugzilla.mozilla.org/show_bug.cgi?id=1332872

Sorry about the earlier review being discarded (https://reviewboard.mozilla.org/r/113980/), it was due to some confusion with the reviewboard usage.

Comment 27

[Tim Nguyen :ntim]

Comment on attachment 8846869 [details]
Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

https://reviewboard.mozilla.org/r/119874/#review121802

::: devtools/client/jar.mn:315
(Diff revision 1)
>      skin/images/firebug/command-scratchpad.svg (themes/images/firebug/command-scratchpad.svg)
>      skin/images/firebug/command-screenshot.svg (themes/images/firebug/command-screenshot.svg)
>      skin/images/firebug/command-measure.svg (themes/images/firebug/command-measure.svg)
>      skin/images/firebug/command-rulers.svg (themes/images/firebug/command-rulers.svg)
>      skin/images/firebug/command-noautohide.svg (themes/images/firebug/command-noautohide.svg)
> +    skin/images/firebug/help.svg (themes/images/firebug/help.svg)

The firebug image is exactly the same as the normal theme image, can you remove it from the patch ?

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8846869 [details]
Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

https://reviewboard.mozilla.org/r/119874/#review122038

Looks good overall. I made a few comments about some minor optimizations.
The decision to work on the UI polish as a separate bug sounds good to me. This piece of UI is currently behind 2 prefs! So we have room for experimentation and incrementally making this better. Let'ship this soon.

::: commit-message-3d341:3
(Diff revision 1)
> +Bug 1332090 - Add Contrast Ratio component to the Color Widget. r?pbro
> +
> +Added details in comments about next steps and discussion with :gl.

Multi-line comments after the initial commit message are very useful, but this one seems to reference a comment that exists in mozreview.
You should use this space to detail what the commit is about, what got implemented, etc.

::: devtools/client/inspector/rules/views/text-property-editor.js:386
(Diff revision 1)
>            onRevert: this._onSwatchRevert
>          });
>          span.on("unit-change", this._onSwatchCommit);
>          let title = l10n("rule.colorSwatch.tooltip");
>          span.setAttribute("title", title);
> +        span.name = this.nameSpan.textContent;

It might be better to use a data attribute here instead of adding properties to the object directly:

`span.dataset.propertyName = this.nameSpan.textContent`

And you can later retrieve the name with
`span.dataset.propertyName`

::: devtools/client/shared/widgets/ColorWidget.js:179
(Diff revision 1)
>    }
>  }
>  
>  module.exports.ColorWidget = ColorWidget;
>  
> +ColorWidget.calculateGradeAndTitle = function (contrastRatio, backgroundColor) {

Please add some jsdoc comment for this function

::: devtools/client/shared/widgets/ColorWidget.js:184
(Diff revision 1)
> +ColorWidget.calculateGradeAndTitle = function (contrastRatio, backgroundColor) {
> +  let grade = "";
> +  let title = "";
> +
> +  if (contrastRatio < 3.0) {
> +    grade = "Fail";

So `Fail` isn't really a grade like `AAA` is, right? It's just an English word, and I think it might make sense to also localize it.
`AAA` (and others) are levels that are specified by WCAG, so it's safe to assume users would either know them or read more in the tooltip, or find more info on the internet. But I think `Fail` makes more sense as a localize string.

::: devtools/client/shared/widgets/ColorWidget.js:203
(Diff revision 1)
> +  title += backgroundColor;
> +
> +  return { grade, title };
> +};
> +
> +ColorWidget.ratioToString = function (contrastRatio) {

Please add some jsdoc comment for this function.

::: devtools/client/shared/widgets/ColorWidget.js:533
(Diff revision 1)
> +      this.contrastGrade.textContent = "";
> +      this.contrastHelp.removeAttribute("title");
> +      return;
> +    }
> +
> +    let rgbaColor = new colorUtils.CssColor(this.closestBackgroundColor);

Color objects can be reused if you use the `newColor` method, you do not need to instantiate a new one here every time.
You could do something like this:

```
if (!this.rgbaColor) {
  this.rgbaColor = new colorUtils.CssColor(this.closestBackgroundColor);
}
this.rgbaColor.newColor(this.closestBackgroundColor);
```

::: devtools/client/shared/widgets/ColorWidget.js:626
(Diff revision 1)
> +    if (this.inspector && this.inspector.selection.nodeFront && this.contrastEnabled) {
> +      let node = this.inspector.selection.nodeFront;
> +      this.closestBackgroundColor = yield (node.getClosestBackgroundColor());
> +    }

Could you move this to `show` instead? `updateUI` is called a lot, especially when moving sliders and cursors in the colorpicker. I don't think we need to get the background color every time.

I think we can get it once when the picker gets opened and then just assume that that value doesn't change. 
Of course, in practice, the color could change dynamically if there is an animation or JavaScript running while the color picker is opened. But I'm willing to accept this as an edge case we don't fully support.
I'm afraid that going to the server each time here is going to slow down using the tooltip too much.

So, if we just get the value when the tooltip opens up, I think we should be fine.

::: devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js:113
(Diff revision 1)
> +      let target = this.inspector.target;
> +      let isContrastCompatible = yield target.actorHasMethod(
> +        "domnode",
> +        "getClosestBackgroundColor"
> +      );

Running this code every time the tooltip opens seems like something we could improve.
I think `actorHasMethod` is actually cached, so maybe this wouldn't make a big difference, but I still think we could cache this to avoid having to `yield` every time we open the tooltip. Something like:

```
if (this.isContrastCompatible === undefined) {
  let target = this.inspector.target;
  this.isContrastCompatible = yield target.actorHasMethod(
    "domnode",
    "getClosestBackgroundColor");
}
```

So, next time the tooltip is opened, we can just skip right over this part of the code.

::: devtools/server/actors/inspector.js:773
(Diff revision 1)
> +        let currentCssColor = new colorUtils.CssColor(currentStyle);
> +        if (!currentCssColor.isTransparent()) {
> +          return currentCssColor.rgba;
> +        }
> +      }
> +      current = current.walker.parentNode(current);

Using the walker here doesn't seem necessary to me, and might actually be a little slower than just using the `.parentNode` DOM property, because it actually instantiates a new `DocumentWalker` each time.

Instead of setting `current` to `this` in the first line, you could set it to `this.rawNode`, and then just iterate on DOM nodes with `current = current.parentNode`.

Also, the walker can walk through frames, which we don't necessarily want here, right? I might be wrong here.
But what that means is that using the walker, we would be walking all the way back up to the root document, even if the node was nested in some iframe.
Instead, if you just use `.parentNode`, you'd stop at the closest parent document.

::: devtools/server/actors/inspector.js:773
(Diff revision 1)
> +        let currentCssColor = new colorUtils.CssColor(currentStyle);
> +        if (!currentCssColor.isTransparent()) {
> +          return currentCssColor.rgba;
> +        }
> +      }
> +      current = current.walker.parentNode(current);

Using the walker here doesn't seem necessary to me, and might actually be a little slower than just using the `.parentNode` DOM property, because it actually instantiates a new `DocumentWalker` each time.

Instead of setting `current` to `this` in the first line, you could set it to `this.rawNode`, and then just iterate on DOM nodes with `current = current.parentNode`.

Also, the walker can walk through frames, which we don't necessarily want here, right? I might be wrong here.
But what that means is that using the walker, we would be walking all the way back up to the root document, even if the node was nested in some iframe.
Instead, if you just use `.parentNode`, you'd stop at the closest parent document.

::: devtools/server/actors/inspector.js:775
(Diff revision 1)
> +          return currentCssColor.rgba;
> +        }
> +      }
> +      current = current.walker.parentNode(current);
> +    }
> +    return "rgba(0, 0, 0, 0)";

Can you explain why you're returning transparent here? Isn't the default background color of a page white in all browsers? White would seem like a better default to me.

::: devtools/shared/css/color.js:1159
(Diff revision 1)
>  }
> +
> +/**
> + * Calculates the luminance of a rgba tuple based on the formula given here
> + * https://www.w3.org/TR/2008/REC-WCAG20-20081211/#relativeluminancedef
> + * Return the luminance of the rgba tuple.

Please add jsdoc-style comments for parameters and return values.
Something like

```
@param {Array} rgba An array with [r,g,b,a] values
@return {Number} The calculated luminance
```

::: devtools/shared/css/color.js:1173
(Diff revision 1)
> +}
> +
> +/**
> + * Calculates the contrast ratio of 2 rgba tuples based on the formula given
> + * https://www.w3.org/TR/2008/REC-WCAG20-20081211/#visual-audio-contrast7
> + * Return the contrast ratio between the 2 rgba tuples.

Same comment here. Please describe the 2 @param and the @return value.

Comment 29

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8846869 [details]
Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

https://reviewboard.mozilla.org/r/119874/#review122038

> Could you move this to `show` instead? `updateUI` is called a lot, especially when moving sliders and cursors in the colorpicker. I don't think we need to get the background color every time.
> 
> I think we can get it once when the picker gets opened and then just assume that that value doesn't change. 
> Of course, in practice, the color could change dynamically if there is an animation or JavaScript running while the color picker is opened. But I'm willing to accept this as an edge case we don't fully support.
> I'm afraid that going to the server each time here is going to slow down using the tooltip too much.
> 
> So, if we just get the value when the tooltip opens up, I think we should be fine.

I made all the other changes, and will commit it to reviewboard once I receive your advice on my following confusion:
Moving the 4 lines above to `show` makes it so contrastEnabled isn't set (`SwatchColorPickerTooltip.js` line 114) by the time it runs the `if` statement thus `this.closestBackgroundColor = yield (node.getClosestBackgroundColor());` is never actually called.

Comment 30

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8846869 [details]
Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

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

Comment 31

[Patrick Brosset <:pbro>]

(In reply to Rahul Chaudhary [:rahulc] from comment #29)
> Moving the 4 lines above to `show` makes it so contrastEnabled isn't set
> (`SwatchColorPickerTooltip.js` line 114) by the time it runs the `if`
> statement thus `this.closestBackgroundColor = yield
> (node.getClosestBackgroundColor());` is never actually called.
I see what you mean. That's because `show` is called before `contrastEnabled` is being set in SwatchColorPickerTooltip.js.
I still think that we should move these lines into the `show` function, so for this to work, we need to move the setting of the `contrastEnabled` property to before we call `show`.

So you could move this part:

if (this.activeSwatch) {
  let name = this.activeSwatch.dataset.propertyName;
  if (this.isContrastCompatible === undefined) {
    let target = this.inspector.target;
    this.isContrastCompatible = yield target.actorHasMethod(
      "domnode",
      "getClosestBackgroundColor"
    );
  }

  // only enable contrast if it is compatible and if the type of property is color.
  this.spectrum.contrastEnabled = (name === "color") && this.isContrastCompatible;
}

Before this is called:

yield SwatchBasedEditorTooltip.prototype.show.call(this);

Or something like that anyway. I'll let you test if this works. This way you can move the call to `this.updateContrast();` to the `show` function.

Comment 32

[Rahul Chaudhary [:rahulc]]

(In reply to Patrick Brosset <:pbro> from comment #31)
> Or something like that anyway. I'll let you test if this works. This way you
> can move the call to `this.updateContrast();` to the `show` function.

Moving that up did work as you said it would!
But I believe the `updateContrast` would also need to be called every time the input changes (so inside `onChange`).

Comment 33

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8846869 [details]
Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

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

Comment 34

[Patrick Brosset <:pbro>]

Hey Cameron, I remember having a conversation somewhat related to this with you some time ago. So maybe you can help.
Here's the use case we have:

we're trying to display a little widget that shows a piece of text, with a given text color, over a given background color. This widget will show the calculated contrast based on these colors. This will be available to users inside the inspector, when they pick a new color for text in the Rules sidebar.

To do this, we know what the current element is, we know which text color the user is picking, but we need to get the background color. Currently, we use something like this:

function getClosestBackgroundColor(node) {
    while (node) {
      let computedStyle = getComputedStyle(node);
      let backgroundColor = computedStyle.getPropertyValue("background-color");
      if (isValidCSSColor(backgroundColor)) {
        return backgroundColor;
      }
      node = node.parentNode;
    }
    return "rgba(255, 255, 255, 1)";
}

Although this will work in simple cases, it will fall short in other cases:
- what if the element sits in a transparent iframe and the background of the parent frame shows through it?
- what if the element is absolutely positioned on top of an element that is not in its chain of ancestors?
- what if the element is above an image or gradient?

I recall you suggesting something where, given X/Y coordinates, we could query the platform for information about that location, like which elements participated in rendering this pixel, or which color is it.
Does that ring a bell? Would something like this be possible to add as a new chrome-only platform API we could use? Should I file a bug?

Rahul: I'm not suggesting we block this bug on this potential other solution, but let's keep it in mind for a follow-up bug to make this widget better.

Comment 35

[Patrick Brosset <:pbro>]

Comment on attachment 8846869 [details]
Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

https://reviewboard.mozilla.org/r/119874/#review125410

Thanks for addressing my comments. The code looks good now.
I just have 3 final comments and we should be good to go for this.

I'm just going to list the follow-ups that will be required:
- as discussed, we'll need to polish the new tooltip (which I think we agreed to do in a later bug),
- we need new tests for this new functionality to avoid future regressions (I would like this to be done in this bug though, can be a different commit),
- we should continue the discussion initiated in comment 34 and find a better long term solution to provide a decent background color for contrast calculations. If we manage to do this, then we can drop the idea I described in comment 11 (letting users choose their own background colors by clicking on the widget), this is complex and won't be used often. And if we do get a much more sure way to get the background color, we won't need this.

::: devtools/client/shared/widgets/ColorWidget.js:395
(Diff revisions 2 - 3)
>      this.alphaSliderWidth = this.alphaSliderInner.offsetWidth;
>      this.alphaSliderHelperWidth = this.alphaSliderHelper.offsetWidth;
>  
> +    if (this.inspector && this.inspector.selection.nodeFront && this.contrastEnabled) {
> +      let node = this.inspector.selection.nodeFront;
> +      this.closestBackgroundColor = yield (node.getClosestBackgroundColor());

nit: There's no need for parens `()` around `node.getClosestBackgroundColor()`

::: commit-message-3d341:3
(Diff revision 3)
> +Added:
> +
> +> A monospace font and displaying a fixed number of digits for contrast ratio and grade.
> +
> +> A (?) icon next to the contrast ratio that explains the contrast grading system briefly on hover (in it's title).
> +
> +> Addresed last review of code.
> +
> +Next Steps:
> +
> +> Finish code review on MVP of the contrast ratio.
> +
> +> Fix non-uniform styling of color widget.
> +
> +> Fix eyedropper bug mentioned below and an eyedropper to the color swatch.

As I said earlier, these comments are more your own TODO list than something useful for someone looking at the commit later.
These extra lines in a commit message should be used to describe exactly what the commit does, not really say that you addressed review comments or still need to fix some code styling, etc.

::: devtools/server/actors/inspector.js:764
(Diff revision 3)
> +   * Returns a string with the background color of the form
> +   * rgba(r, g, b, a). Defaults to rgba(255, 255, 255, 1) if no
> +   * background color is found.
> +   */
> +  getClosestBackgroundColor: function () {
> +    console.log(this.toString());

Please remove this console.log

Comment 36

[Rahul Chaudhary [:rahulc]]

Comment on attachment 8846869 [details]
Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

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

Comment 37

[Cameron McCormack (:heycam)]

Hi Patrick, yes I remember the conversation. :-)

(In reply to Patrick Brosset <:pbro> from comment #34)
> Although this will work in simple cases, it will fall short in other cases:
> - what if the element sits in a transparent iframe and the background of the
> parent frame shows through it?
> - what if the element is absolutely positioned on top of an element that is
> not in its chain of ancestors?
> - what if the element is above an image or gradient?

Also, if you are really considering what the final rendered result looks like: what if there are filters, masks, clip-paths, opacity, mix-blend-mode, etc. on the element or any of its ancestors?

What if there are scrollable elements (either the target element itself, or some ancestor) such that the background the text appears on would changed depending on the scroll position?

What if there are some animated background images or videos? :)

> I recall you suggesting something where, given X/Y coordinates, we could
> query the platform for information about that location, like which elements
> participated in rendering this pixel, or which color is it.
> Does that ring a bell? Would something like this be possible to add as a new
> chrome-only platform API we could use? Should I file a bug?

I think the API I suggested was one that took an X/Y coordinate pair as input and returned a list of elements that contributed to the pixel value at that position.  I was imagining that we could build the display list for the document, and then do some sort of query on that list.  I'm not very familiar with the display list code so I'm not sure how much work it would be to adapt it to return this information.

But I'm not sure this API is what we want here.  One thing is that I don't think you want to query this pixel by pixel, making a judgment about the contrast the text would have given the background pixels around it.  But also you would need to exclude elements that are painted on top of the element you're querying.  So maybe you want to paint the region covered by the element's padding box, but exclude items painted on top of it, then compute the average pixel value that was painted?  Maybe excluding filters, masking, etc. on the element itself?  And then do something similar with the whole element filled with the text color, and average that?  This is probably too complex, but would get you closer to a kind of general question like "what is the contrast between text and its background in this element"?  And it sounds like a lot of work, but as I say I'm not that familiar with the display list code to know how easy it would be to render parts of the tree like this.

Sorry, my comment includes a lot of question marks, and might not be that helpful.  But it seems like a complicated thing to do right.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a843c3e25db
Added a contrast ratio component to the Color Widget. r=pbro

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a843c3e25db

Comment 40

[Patrick Brosset <:pbro>]

I see this bug is marked as FIXED, but please let's not forget about this:

(from comment #35)
> - we need new tests for this new functionality to avoid future regressions
> (I would like this to be done in this bug though, can be a different commit),

Could you please file a bug to get them done before we ship the new color widget?

Comment 41

[Francesco Lodolo [:flod]]

Comment on attachment 8846869 [details]
Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;

https://reviewboard.mozilla.org/r/119874/#review127962

::: devtools/client/locales/en-US/inspector.properties:369
(Diff revision 4)
> +# to the color widget.
> +inspector.colorwidget.contrastRatio.invalidColor=Invalid color
> +
> +# LOCALIZATION NOTE (inspector.colorwidget.contrastRatio.info):
> +# This string is used to explain the contrast ratio grading system when you hover over the help icon in the contrast info.
> +inspector.colorwidget.contrastRatio.info=The contrast ratio grading system for text has the following grading: Fail, AA*, AAA* AAA from lowest to highest readability.\nIt was calculated based on the computed background color:

Did you miss a command before AAA?

Comment 42

[YF (Yang)]

(In reply to Francesco Lodolo [:flod] from comment #41)
> Comment on attachment 8846869 [details]
> Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;
> 
> https://reviewboard.mozilla.org/r/119874/#review127962
> 
> ::: devtools/client/locales/en-US/inspector.properties:369
> (Diff revision 4)
> > +# to the color widget.
> > +inspector.colorwidget.contrastRatio.invalidColor=Invalid color
> > +
> > +# LOCALIZATION NOTE (inspector.colorwidget.contrastRatio.info):
> > +# This string is used to explain the contrast ratio grading system when you hover over the help icon in the contrast info.
> > +inspector.colorwidget.contrastRatio.info=The contrast ratio grading system for text has the following grading: Fail, AA*, AAA* AAA from lowest to highest readability.\nIt was calculated based on the computed background color:
> 
> Did you miss a command before AAA?

Should be "following grading: Fail, AA*, AAA* and AAA" or "following grading: Fail, AA*, AAA*, AAA"

Comment 43

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #41)
> Comment on attachment 8846869 [details]
> Bug 1332090 - Added a contrast ratio component to the Color Widget - Part 1;
> 
> https://reviewboard.mozilla.org/r/119874/#review127962
> 
> ::: devtools/client/locales/en-US/inspector.properties:369
> (Diff revision 4)
> > +# to the color widget.
> > +inspector.colorwidget.contrastRatio.invalidColor=Invalid color
> > +
> > +# LOCALIZATION NOTE (inspector.colorwidget.contrastRatio.info):
> > +# This string is used to explain the contrast ratio grading system when you hover over the help icon in the contrast info.
> > +inspector.colorwidget.contrastRatio.info=The contrast ratio grading system for text has the following grading: Fail, AA*, AAA* AAA from lowest to highest readability.\nIt was calculated based on the computed background color:
> 
> Did you miss a command before AAA?

s/command/comma :-\