Bug 222161 - Web Inspector: CSS Grid Inspector: use a color palette for default grid overlay colors
Summary: Web Inspector: CSS Grid Inspector: use a color palette for default grid overl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-19 00:23 PST by Nikita Vasilyev
Modified: 2021-02-19 13:35 PST (History)
5 users (show)

See Also:


Attachments
Patch (5.49 KB, patch)
2021-02-19 00:36 PST, Nikita Vasilyev
drousso: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
[Image] With patch applied (914.01 KB, image/png)
2021-02-19 00:38 PST, Nikita Vasilyev
no flags Details
Patch (5.56 KB, patch)
2021-02-19 11:57 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Patch (5.73 KB, patch)
2021-02-19 12:16 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2021-02-19 00:23:18 PST
Currently, we use "magenta" each grid element. We should use a qualitative color palette by default instead.

<rdar://problem/73505119>
Comment 1 Nikita Vasilyev 2021-02-19 00:36:26 PST
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).
Comment 2 Nikita Vasilyev 2021-02-19 00:38:31 PST
Created attachment 420934 [details]
[Image] With patch applied

Screenshot of https://stripe.com.
Comment 3 Devin Rousso 2021-02-19 09:12:59 PST
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
Comment 4 Nikita Vasilyev 2021-02-19 11:52:05 PST
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.
Comment 5 Nikita Vasilyev 2021-02-19 11:57:08 PST
Created attachment 421002 [details]
Patch
Comment 6 Nikita Vasilyev 2021-02-19 12:16:55 PST
Created attachment 421008 [details]
Patch
Comment 7 EWS 2021-02-19 13:35:11 PST
Committed r273164: <https://commits.webkit.org/r273164>

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