Bug 204772 – Web Inspector: Add text/background contrast ratio information to color picker

• Status: ASSIGNED
• Component: Web Inspector

[Nikita Vasilyev]

Web Content Accessibility Guidelines define levels of acceptable text color to background color contrast ratios and provide formulas for calculating contrast (https://www.w3.org/TR/WCAG20/#contrast-ratiodef).

[Radar WebKit Bug Importer]

<rdar://problem/57573998>

[Nikita Vasilyev]

Created attachment 390328 [details]
Patch

[Nikita Vasilyev]

Created attachment 390329 [details]
[Image] With patch applied

[Timothy Hatcher]

Comment on attachment 390328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390328&action=review

Neat!

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:541
> +                this.getBackgroundColorOf(this._property.ownerStyle.nodeStyles, function(backgroundColor) {

Nit: Use arrow function syntax.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:574
> +            let parentNode = nodeStyles.node.parentNode;
> +            let parentNodeStyles = WI.cssManager.stylesForNode(parentNode);
> +            parentNodeStyles.refresh().then(() => {
> +                this.getBackgroundColorOf(parentNodeStyles, callback);
> +            });

What if parentNode is null?

[Nikita Vasilyev]

Created attachment 390817 [details]
Patch

[Timothy Hatcher]

How does this interact with the variables list?

Maybe we should have more explanation on what the number means. Is 1.9 good or bad? A ratio or range of 1 to 21 is mentioned in the spec. How is this number related to that?

[Nikita Vasilyev]

Comment on attachment 390817 [details]
Patch

(In reply to Timothy Hatcher from comment #6)
> How does this interact with the variables list?

I think it should be above the variable list.

> 
> Maybe we should have more explanation on what the number means. Is 1.9 good
> or bad? A ratio or range of 1 to 21 is mentioned in the spec. How is this
> number related to that?

Yes, absolutely.

The idea is to show a green checkmark icon when the contrast it acceptable, and a warning icon when it isn't.

For large text (>120% larger than body text) the minimum ratio is 3:1.
For body text, it's 4.5:1. To display appropriate warning or checkmark, ColorPicker wound need to know about the font size.

I'm not going to lend this just yet because, I think, the number alone is not very descriptive.

[Devin Rousso]

Comment on attachment 390817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=390817&action=review

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:351
> +    getPropertyByName(name)

Why not use `propertyForName`?

> Source/WebInspectorUI/UserInterface/Models/Color.js:590
> +        return 0.2126 * r + 0.7152 * g + 0.0722 * b;

Style: please add parenthesis to clarify order of operations
```
    return (0.2126 * r) + (0.7152 * g) + (0.0722 * b);
```

> Source/WebInspectorUI/UserInterface/Models/Color.js:667
> +        return (Math.max(l1, l2) + 0.05) / (Math.min(l1, l2) + 0.05);

Style: extra parenthesis around the entire expression

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:106
> +    border: 0.5px solid var(--text-color-quaternary);

Should we be using a variable for "text" in a "border"?

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:112
> +    margin-left: 8px;

What about RTL?

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:116
> +    margin-left: -16px;

Ditto (112)

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:117
> +    top: -2px;

Style: please put this above so the two `margin-*` are next to each other.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:118
> +    margin-right: 10px;

Ditto (112)

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:28
> +    constructor(showContrast = false)

NIT: I prefer using a object for optional arguments instead of individually specified arguments with default values, as it makes it _much_ easier for new arguments to be added in the future.
```
    constructor({showContrast} = {})
```
For optional parameters, the naming should imply a default falsy value as well, so no need to have a fallback for it.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:63
> +            this._contrastLabelText = WI.UIString("Contrast: ", "Contrast @ Color Picker", "Color contrast between foreground text color and background color");

Rather than save this value to a member variable, why not have `_calculateContrast` always do it and move the early-return of `!this._backgroundColor` to after `this._contrastElement.textContent` is set?

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:162
> +        previewBackground.style.backgroundColor = this._backgroundColor;

NIT: I prefer using `setProperty` as it makes searching for CSS inside JavaScript _much_ easier.
```
    previewBackground.style.setProperty("background-color", this._backgroundColor);
```

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:165
> +        previewForeground.style.backgroundColor = this._color.toString();

Ditto (162)

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:219
> +            this._valueEditor = new WI.ColorPicker(this._valueEditorOptions?.showContrast || false);

Following my comment on ColorPicker.js:28, the `|| false` can be dropped.
```
    this._valueEditor = new WI.ColorPicker({showContrast: this._valueEditorOptions?.showContrast});
```

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:539
> +            swatch.addEventListener(WI.InlineSwatch.Event.Activated, (event) => {

We should avoid adding two listeners for the same event to the same object.  Please merge this with the listener below.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:540
> +                let colorPicker = event.target.valueEditor;

Should we assert that `colorPicker instanceof WI.ColorPicker`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:564
> +    getBackgroundColorOf(nodeStyles, callback)

Why isn't this a function on `WI.DOMNodeStyles`?  It definitely should have tests.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:568
> +        let color = WI.Color.fromString(backgroundColorProperty.value);

Should we assert that `backgroundColorProperty` is not null?  I know it is always set for CSS, but that may not be true for ITML (please check).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:572
> +                const DefaultBackgroundColor = "white";

Style: don't capitalize a variable name

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:576
> +            }
> +            let parentNodeStyles = WI.cssManager.stylesForNode(parentNode);

Style: add a return after an `if` that `return`s.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:577
> +            parentNodeStyles.refresh().then(() => {

Why not `refreshIfNeeded`?  Otherwise, we could trigger a second `refresh` while one is already being processed.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:581
> +            callback(backgroundColorProperty.value);

Rather than have a full `if-else`, how about we have an early return with this code if `color.alpha !== 0`?
```
    if (color.alpha) {
        callback(backgroundColorProperty.value);
        return;
    }

    let parentNode = nodeStyles.node.parentNode;
    ...
```