Bug 222319 – Web Inspector: Persist CSS Grid overlay colors

• Component: Web Inspector
• Status: RESOLVED FIXED

[Razvan Caliman]

When changing the color for a CSS grid overlay, the value should be saved and restored the next time the overlay is shown (i.e. after page reload, browser restart, etc).

[Radar WebKit Bug Importer]

<rdar://problem/74647242>

[Razvan Caliman]

Created attachment 421324 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 421324 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:37
> +        // Can't rely on value matching in `this._gridColors` alone because colors may be edited to become duplicates of existing values.
> +        this._colorIndexForNodeMap = new Map;

I don't understand this. Could you write down steps for me to take to make colors become duplicates of the existing values?

[Razvan Caliman]

(In reply to Nikita Vasilyev from comment #3)
> Comment on attachment 421324 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421324&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:37
> > +        // Can't rely on value matching in `this._gridColors` alone because colors may be edited to become duplicates of existing values.
> > +        this._colorIndexForNodeMap = new Map;
> 
> I don't understand this. Could you write down steps for me to take to make
> colors become duplicates of the existing values?

Sure!

- Open a page with more than one CSS grid container
- Switch to Layout panel
- Edit the color for the first overlay to pure black
- Edit the color for the second overlay to pure black
- Then, edit the color for the second overlay to something else

If we'd try to match the color by looking at values in the colors array, we'd match the first back encountered. 

If using a reference to the index in the array, we'll always point to the color the user actually wants to overwrite.

[Patrick Angle]

Comment on attachment 421324 [details]
Patch

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

What is the behavior if a node from the middle of the list becomes something other than a grid and later becomes a grid again? Do all the colors shift each time the number of grids change?

>>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:37
>>> +        this._colorIndexForNodeMap = new Map;
>> 
>> I don't understand this. Could you write down steps for me to take to make colors become duplicates of the existing values?
> 
> Sure!
> 
> - Open a page with more than one CSS grid container
> - Switch to Layout panel
> - Edit the color for the first overlay to pure black
> - Edit the color for the second overlay to pure black
> - Then, edit the color for the second overlay to something else
> 
> If we'd try to match the color by looking at values in the colors array, we'd match the first back encountered. 
> 
> If using a reference to the index in the array, we'll always point to the color the user actually wants to overwrite.

But shouldn't two different Color objects, even with the same values within, still `colorA !== colorB`?

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:39
> +        // List of preset colors (HSL) for grid overlays with defaults provided. Will be mutated and persisted after editing overlay colors.

I think this comment is redundancy with the code right below it, as WI.Setting pretty much always has a default value(s) and are assumed to be mutated and persisted. At most, it might be worth noting that these are HSL colors, since that's not entirely obvious without the comment.
```
// Default colors are notated in HSL
```
or similar.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:88
> +        // The method to show the overlay will be called repeatedly while updating the grid overlay color.

I'm not sure what this comment has to do with the code below? Can you elaborate?

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:162
> +        // Update references for all nodes using the color at the same index.

This comment feels redundant.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:170
> +            // Update any visible grid overlays to reflect the new color.

dito :162

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:129
> +            swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, () => {

Generally, we want to define `event` as a parameter to this closure even though it isn't used so that we know it is available and so that a future diff that uses the event would be cleaner.
```
swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, (event) => {
```

[Razvan Caliman]

Comment on attachment 421324 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:37
>>>> +        this._colorIndexForNodeMap = new Map;
>>> 
>>> I don't understand this. Could you write down steps for me to take to make colors become duplicates of the existing values?
>> 
>> Sure!
>> 
>> - Open a page with more than one CSS grid container
>> - Switch to Layout panel
>> - Edit the color for the first overlay to pure black
>> - Edit the color for the second overlay to pure black
>> - Then, edit the color for the second overlay to something else
>> 
>> If we'd try to match the color by looking at values in the colors array, we'd match the first back encountered. 
>> 
>> If using a reference to the index in the array, we'll always point to the color the user actually wants to overwrite.
> 
> But shouldn't two different Color objects, even with the same values within, still `colorA !== colorB`?

The contents of `this._gridColors` is an array of arrays (HSL values), not Color objects. It gets persisted to local storage after a pass through `JSON.stringify()` (See WI.Setting.value setter)

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:39
>> +        // List of preset colors (HSL) for grid overlays with defaults provided. Will be mutated and persisted after editing overlay colors.
> 
> I think this comment is redundancy with the code right below it, as WI.Setting pretty much always has a default value(s) and are assumed to be mutated and persisted. At most, it might be worth noting that these are HSL colors, since that's not entirely obvious without the comment.
> ```
> // Default colors are notated in HSL
> ```
> or similar.

Fair enough

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:88
>> +        // The method to show the overlay will be called repeatedly while updating the grid overlay color.
> 
> I'm not sure what this comment has to do with the code below? Can you elaborate?

When changing the color of a grid overlay from a swatch in `CSSGridSection`, we call again `WI.OverlayManager.showGridOverlay()` with the same node and the new color. 
If the event handler is already attached, trying to attach it again trips an assertion about it being a duplicate in `WI.Object.addEventListener()`

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:162
>> +        // Update references for all nodes using the color at the same index.
> 
> This comment feels redundant.

Fair :)

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:170
>> +            // Update any visible grid overlays to reflect the new color.
> 
> dito :162

Only if you know the intricacies of how a visible overlay's color gets updated  :) 
I'd keep this one because it's slightly more nuanced than :162.

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:129
>> +            swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, () => {
> 
> Generally, we want to define `event` as a parameter to this closure even though it isn't used so that we know it is available and so that a future diff that uses the event would be cleaner.
> ```
> swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, (event) => {
> ```

Ok. Will add.

[Nikita Vasilyev]

Created attachment 421329 [details]
[Video] Toggling "display: grid"

(In reply to Patrick Angle from comment #5)
> Comment on attachment 421324 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421324&action=review
> 
> What is the behavior if a node from the middle of the list becomes something
> other than a grid and later becomes a grid again? Do all the colors shift
> each time the number of grids change?

I had the same question in mind :)
Colors don't shift. If they were, the implementation could be a lot easier. Here's a screen recording.

[Razvan Caliman]

Created attachment 421330 [details]
Patch

Address comments from Patrick

[Devin Rousso]

Comment on attachment 421330 [details]
Patch

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

r- for the reason explained below

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:40
> +        // Color presets for grid overlays with defaults in HSL notation.
> +        this._gridColors = new WI.Setting("grid-overlay-colors", [

This is not really how we normally do these kinds of settings.  It seems unnecessary (and perhaps undesirable) to have the list of colors persist across webpages (especially for two different webpages that have a different ordering of CSS grid contexts).  I would suggest a different approach.

IMO we only should really care about saving colors for nodes that the developer has explicitly modified.  I'd suggest that instead of modifying the original list of colors, you specifically create a new setting for that node (and also perhaps keyed with the URL of the current inspected page) so that when visiting other sites the developers choice of modifying the color for that specific node doesn't affect anything else.  Especially since we only have five colors, I can easily imagine a situation where the developer modifies the color of the 2nd grid context on one page and then has to change it to some other color on another page because it clashes.  Another problematic scenario is if the developer is trying to modify the 1st and 6th grid (which would both share the same color since `1 % 5 === 1` and `6 % 5 === 1`) then they're forced to use the same color for both.  If you instead individually save colors for each node as they are modified, it makes it impossible for these situations to occur.  I'd suggest taking a look at `WI.DOMBreakpoint` for similar prior art.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:136
> +        const hslColors = this._gridColors.value;

Style: please use `let` here as we only use `const` for things that will not change between Web Inspector sessions (i.e. actual constant values)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:146
> +    saveColorForNode(domNode, newColor)

If you're naming the other one `getColorForNode` then I'd name this `setColorForNode`.

Also,
 - please include `Grid` somewhere in the name (e.g. `saveGridColorForNode`) so that it's clear what it's used for (and so that it matches the event name).
 - we don't usually add the prefix `new*` to variables unless we also plan on having an `old*` (and even then it's pretty rare), so I'd remove it

BTW these could be made into regular `get gridColor` and `set gridColor` if all this logic was moved to `WI.DOMNode`.

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:151
> +        if (!this._colorIndexForNodeMap.has(domNode))

rather than do a `has` and then a `get` (which is two lookups), you could just check `colorIndex === undefined` instead

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:160
> +        // To persist the new value, we need to make a new array instead of mutating the existing one. See WI.Setting.value
> +        this._gridColors.value = this._gridColors.value.map((color, index) => index === colorIndex ? newColor.hsl : color );

FYI this is what `WI.Setting.prototype.save` is for.  You shouldn't need to set a brand new array.
```
    this._gridColors[colorIndex] = newColor.hsl;
    this._gridColors.save();
```

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:215
> +    GridColorChanged: "overlay-manager-grid-color-changed",

this should probably be `GridOverlayColorChanged` to be explicitly clear (and match other event names)

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:131
> +                if (swatch.value !== gridColor)
> +                    WI.overlayManager.saveColorForNode(domNode, swatch.value);

By using `swatch` here, you're implicitly capturing `swatch` as a strong reference, meaning that this event listener could keep `swatch` alive.  Since you're already providing `swatch` as the `thisObject` (3rd argument to `WI.Object.prototype.addEventListener`), you should change these `swatch` to `this` to avoid the implicit capture.

[Razvan Caliman]

Created attachment 422124 [details]
Patch

[Razvan Caliman]

Comment on attachment 421330 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:40
>> +        this._gridColors = new WI.Setting("grid-overlay-colors", [
> 
> This is not really how we normally do these kinds of settings.  It seems unnecessary (and perhaps undesirable) to have the list of colors persist across webpages (especially for two different webpages that have a different ordering of CSS grid contexts).  I would suggest a different approach.
> 
> IMO we only should really care about saving colors for nodes that the developer has explicitly modified.  I'd suggest that instead of modifying the original list of colors, you specifically create a new setting for that node (and also perhaps keyed with the URL of the current inspected page) so that when visiting other sites the developers choice of modifying the color for that specific node doesn't affect anything else.  Especially since we only have five colors, I can easily imagine a situation where the developer modifies the color of the 2nd grid context on one page and then has to change it to some other color on another page because it clashes.  Another problematic scenario is if the developer is trying to modify the 1st and 6th grid (which would both share the same color since `1 % 5 === 1` and `6 % 5 === 1`) then they're forced to use the same color for both.  If you instead individually save colors for each node as they are modified, it makes it impossible for these situations to occur.  I'd suggest taking a look at `WI.DOMBreakpoint` for similar prior art.

Implemented with one `WI.Setting` per URL/node path hashed, as discussed over chat.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:146
>> +    saveColorForNode(domNode, newColor)
> 
> If you're naming the other one `getColorForNode` then I'd name this `setColorForNode`.
> 
> Also,
>  - please include `Grid` somewhere in the name (e.g. `saveGridColorForNode`) so that it's clear what it's used for (and so that it matches the event name).
>  - we don't usually add the prefix `new*` to variables unless we also plan on having an `old*` (and even then it's pretty rare), so I'd remove it
> 
> BTW these could be made into regular `get gridColor` and `set gridColor` if all this logic was moved to `WI.DOMNode`.

Renamed these methods to `setGridColorForNode()` / `getGridColorForNode()`. 
I don't see a reason yet to make the grid overlay color a property of the `DOMNode`. We can revisit later if it proves necessary.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:215
>> +    GridColorChanged: "overlay-manager-grid-color-changed",
> 
> this should probably be `GridOverlayColorChanged` to be explicitly clear (and match other event names)

Dropped the event because it's not needed with the latest implementation.

It may be useful to add back again when we need to notify other clients about overlay color changes,  like badges in the DOM tree (https://bugs.webkit.org/show_bug.cgi?id=221923). But it's not needed now.

[Devin Rousso]

Comment on attachment 422124 [details]
Patch

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

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

Is this actually needed?  Can we just use `_savedGridColorForNodeMap` (and/or rename it to `_gridColorForNodeMap`)?

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:147
> +        let colorSetting = this._gridColorSettingForNodeMap.get(domNode);

Is this something we can assume?  Is the user able to change the color of the overlay before the overlay is shown?  Should we make a `_getSavedGridColorForNodeSetting` that has the logic currently in `_getSavedGridColorForNode`?

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:164
> +            colorSetting = new WI.Setting(`grid-overlay-color-${domNode.ownerDocument.documentURL.hash}-${domNode.path().hash}`, null);

Is it guaranteed that `documentURL` is set?  Should we have a fallback (e.g. `WI.networkManager.mainFrame.url`)?

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:124
> +            swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, (event) => {

Is this actually needed?  I would think that any modifications to the color swatch would be handled by the `WI.InlineSwatch.Event.ValueChanged` listener above.

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:127
> +            }, this);

I think you should use `swatch` instead of `this` (and then change `swatch` inside the arrow function to `this` or `event.target`) so that the event listener is kept alive so long as `swatch` is kept alive (which is either the same or shorter than `this`).

[Razvan Caliman]

Comment on attachment 422124 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:35
>> +        this._defaultGridColorForNodeMap = new WeakMap;
> 
> Is this actually needed?  Can we just use `_savedGridColorForNodeMap` (and/or rename it to `_gridColorForNodeMap`)?

That's right. We simplify this further by assigning the default color to the `WI.Setting` default value and using it from there.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:147
>> +        let colorSetting = this._gridColorSettingForNodeMap.get(domNode);
> 
> Is this something we can assume?  Is the user able to change the color of the overlay before the overlay is shown?  Should we make a `_getSavedGridColorForNodeSetting` that has the logic currently in `_getSavedGridColorForNode`?

There is no code path that sets the color without first requesting a color value for the node (for example, to initialize the color swatch). 
We can be certain that `OverlayManager.getGridColorForNode()` was called before and, therefore, a `WI.Setting` was instantiated, before ever calling `OverlayManager.setGridColorForNode()`. 
Whether the overlay is shown or not is irrelevant. The color is associated with the node, not the overlay or its state.

We could be defensive about it, but there is currently no code that tries to set the color without first requesting a preset.

>> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:164
>> +            colorSetting = new WI.Setting(`grid-overlay-color-${domNode.ownerDocument.documentURL.hash}-${domNode.path().hash}`, null);
> 
> Is it guaranteed that `documentURL` is set?  Should we have a fallback (e.g. `WI.networkManager.mainFrame.url`)?

Indeed, we can be defensive here and fallback to `WI.networkManager.mainFrame.url`.

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:124
>> +            swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, (event) => {
> 
> Is this actually needed?  I would think that any modifications to the color swatch would be handled by the `WI.InlineSwatch.Event.ValueChanged` listener above.

`WI.InlineSwatch.Event.ValueChanged` is fired with a high frequency while editing. There's no need to write to local storage so often to save intermediate values (`WI.Setting.save()`). 
We'd have to throttle/debounce the handler anyway. Explicitly committing the value when the user is done editing feels more appropriate.

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:127
>> +            }, this);
> 
> I think you should use `swatch` instead of `this` (and then change `swatch` inside the arrow function to `this` or `event.target`) so that the event listener is kept alive so long as `swatch` is kept alive (which is either the same or shorter than `this`).

I'm confused about this. It's the reverse of your previous comment:

> By using `swatch` here, you're implicitly capturing `swatch` as a strong reference, meaning that this event listener could keep `swatch` alive.  Since you're already providing `swatch` as the `thisObject` (3rd argument to `WI.Object.prototype.addEventListener`), you should change these `swatch` to `this` to avoid the implicit capture.

Now that `swatch` is no longer stored in a `Map` it would be orphaned once the DOM of the node list is replaced. Wouldn't this be enough to garbage collect it?

[Razvan Caliman]

Created attachment 422220 [details]
Patch

[Devin Rousso]

Comment on attachment 422220 [details]
Patch

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:154
> +        colorSetting.value = color.hsl;

I think at the very least we should `console.assert(colorSetting, "There should already be a setting created when the grid color is first fetched");` or something like that (feel free to adjust the message as you see fit).

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:125
> +                if (swatch.value !== gridColor)

I think you'll also want to `gridColor = swatch.value;` so that if the swatch is reactivated later it wouldn't trigger a re-save if no changes are made.

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:127
> +            }, swatch);

>> I think you should use `swatch` instead of `this` (and then change `swatch` inside the arrow function to `this` or `event.target`) so that the event listener is kept alive so long as `swatch` is kept alive (which is either the same or shorter than `this`).
> 
> I'm confused about this. It's the reverse of your previous comment:
>
>> By using `swatch` here, you're implicitly capturing `swatch` as a strong reference, meaning that this event listener could keep `swatch` alive.  Since you're already providing `swatch` as the `thisObject` (3rd argument to `WI.Object.prototype.addEventListener`), you should change these `swatch` to `this` to avoid the implicit capture.
>
> Now that `swatch` is no longer stored in a `Map` it would be orphaned once the DOM of the node list is replaced. Wouldn't this be enough to garbage collect it?

I see now that what I wrote in my first comment could be interpreted in a few ways, so let me try to be clearer now.  The event listener system provided by `WI.Object` is awesome for a few reasons:
 1) it allows us to avoid having to `Function.prototype.bind` functions (or use an arrow function) by passing a 3rd argument `thisObject` to `addEventListener` which is used with `Function.prototype.call` when the listener is to be invoked
 2) by using `WeakRef` to store the `thisObject` we can avoid situations where this custom event listener system would normally keep objects alive (but this only works so long as we're _very_ careful to make sure that we don't accidentally capture something we don't want to keep alive in the listener)
 3) we can listen for things on the prototype/constructor too, which works just like the bubbling phase of events in the DOM, but instead walks the prototype chain of the dispatching `WI.Object`
My point in the past reviews was specifically about #2, in that if you use `swatch` both as the `thisObject` _and_ inside the listener function then the benefits provided by `WeakRef` are possibly nullified because you're capturing a strong reference to the same value you're trying to have a `WeakRef` for (which means that the `WeakRef` is useless since there's also a strong reference somewhere else).  With the majority of uses of `WI.Object.prototype.addEventListener`, there's no need to be concerned about this because the listener is a member function of a `class` (and therefore doesn't have any local scope captured values), but if the listener is an inline function (like what you're doing) then there is a possibility for error.  To be clear, I'm not saying that we need to make this into a member function; there are plenty of situations where a local inline function is preferred (like this one).  All I am asking is that we make sure we don't accidentally create strong references to objects where there don't need to be.
```
    swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, (event) => {
        if (event.target.value === gridColor)
            return;

        gridColor = event.target.value;
        WI.overlayManager.setGridColorForNode(domNode, gridColor);
    }, swatch);
```
(you could also replace `event.target` with `this`, but that's perhaps more confusing to read than using `event.target`)

It's very possible (I'm pretty sure that it does actually) that the garbage collector in JSC is able to handle isolated "islands"/cycles of object keeping each other alive, but I'd rather just be safe/correct and do the right thing without even worrying.

[Razvan Caliman]

Created attachment 422251 [details]
Patch

Carry over R+; Address code comments

[EWS]

[e-mail] does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 422251 [details] from commit queue.

[Nikita Vasilyev]

Comment on attachment 422251 [details]
Patch

mac-wk2 failures are unrelated. cq+

[Nikita Vasilyev]

Created attachment 422266 [details]
Patch

Re-uploading the exact same patch for commit-queue

[EWS]

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

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