Currently, we use "magenta" each grid element. We should use a qualitative color palette by default instead. <rdar://problem/73505119>
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).
Created attachment 420934 [details] [Image] With patch applied Screenshot of https://stripe.com.
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 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.
Created attachment 421002 [details] Patch
Created attachment 421008 [details] Patch
Committed r273164: <https://commits.webkit.org/r273164> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421008 [details].