Bug 222161 – Web Inspector: CSS Grid Inspector: use a color palette for default grid overlay colors

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Currently, we use "magenta" each grid element. We should use a qualitative color palette by default instead.

<rdar://problem/73505119>

[Nikita Vasilyev]

Created attachment 420932 [details]
Patch

The palette I'm using is inspired by https://colorbrewer2.org/#type=qualitative&scheme=Set1&n=5
but it's heavily modified to fit our requirements. The overlay lines are 1px wide.
The Colorbrewer2 palettes are made to fill regions on the map, not to draw 1px borders.
I increased lightness and brightness to better serve our needs.

I also made it slightly more colorblind safe (tested with Sim Daltonism https://apps.apple.com/us/app/sim-daltonism/id693112260?mt=12).

[Nikita Vasilyev]

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

Screenshot of https://stripe.com.

[Devin Rousso]

Comment on attachment 420932 [details]
Patch

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

r=me with some comments

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:35
> +        this._colorForNodeMap = new Map;

I'd use a `WeakMap` so that we don't accidentally keep things alive

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:122
> +            [40, 97, 57]

Style: missing trailing comma

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:124
> +        let colorIndex = this._colorForNodeMap.size % hslColors.length;

This means that if a developer enables the grid overlay for a node and then that node (or any other node) is removed, the next node they show an overlay for will have the same default color.  Do we want that?  I feel like we should keep a separate count instead (e.g. `_nextDOMNodeColorIndex` that we `this._nextDOMNodeColorIndex = (this._nextDOMNodeColorIndex + 1) % hslColors.length;` and then reset to `0` inside `_handleMainResourceDidChange`).

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:125
> +        let hslComponents = hslColors[colorIndex];

NIT: I'd just inline this

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:142
> +    _mainResourceDidChange(event)

NIT: I recommend prefixing event listener callbacks with `_handle*` so they're immediately identifiable

[Nikita Vasilyev]

Comment on attachment 420932 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:124
>> +        let colorIndex = this._colorForNodeMap.size % hslColors.length;
> 
> This means that if a developer enables the grid overlay for a node and then that node (or any other node) is removed, the next node they show an overlay for will have the same default color.  Do we want that?  I feel like we should keep a separate count instead (e.g. `_nextDOMNodeColorIndex` that we `this._nextDOMNodeColorIndex = (this._nextDOMNodeColorIndex + 1) % hslColors.length;` and then reset to `0` inside `_handleMainResourceDidChange`).

Currently, I'm not removing nodes from `this._colorForNodeMap` when a DOM element is removed. So the next node a developer shows an overlay will have a different color.
However, I would have to add _nextDOMNodeColorIndex if I use a WeakMap because it doesn't have the `size` getter.

[Nikita Vasilyev]

Created attachment 421002 [details]
Patch

[Nikita Vasilyev]

Created attachment 421008 [details]
Patch

[EWS]

Committed r273164: <https://commits.webkit.org/r273164>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421008 [details].