Bug 221370 – Web Inspector: Elements: show badges for CSS Grid container elements

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

[Nikita Vasilyev]

Elements with `display: grid` or `display: inline-grid` should have a "grid" badge:

    <div class="wrapper"> [grid]

Clicking [grid] should toggle the grid overlay.

<rdar://problem/67881277>

[Nikita Vasilyev]

Created attachment 419947 [details]
WIP

[Nikita Vasilyev]

Created attachment 420549 [details]
Patch

[Nikita Vasilyev]

Created attachment 420550 [details]
[Video] With patch applied

[Devin Rousso]

Comment on attachment 420549 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:26
> +.tree-outline.dom .badge-css-grid {

I think this should be more specific like `.tree-outline.dom li > .badge-css-grid` so that there's less of a chance of a selector clash.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:36
> +    margin-left: 3px;

Is the DOM tree always LTR?  If not, I think this should be `margin-inline-start`.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:42
> +    color: white;

dark mode?

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:48
> +    background-color: transparent;
> +    border-color: transparent;

Why are we removing the border and background when the element is selected?  This looks really weird IMO (especially when unfocused, as it's like the badge disappears and there's this random text left there).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:53
> +    color: white;

ditto (:+42)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:450
> +        if (this.representedObject.layoutContextType === WI.DOMNode.LayoutContextType.Grid) {
> +            WI.overlayManager.addEventListener(WI.OverlayManager.Event.GridOverlayShown, this._updateGridBadgeStatus, this);
> +            WI.overlayManager.addEventListener(WI.OverlayManager.Event.GridOverlayHidden, this._updateGridBadgeStatus, this);
> +        }

You probably need to add/remove these event listeners inside `_handleLayoutContextTypeChanged` in case a node that initially wasn't a grid context later becomes one.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2015
> +        if (!WI.settings.experimentalEnableLayoutPanel.value)

this should use a different setting (or this setting should be renamed to be more generic) as it has nothing to do with the Layout panel

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2047
> +        WI.overlayManager.toggleGridOverlay(this.representedObject);

IMO we should maintain some state on `WI.DOMNode` that indicates what overlays are currently shown and use that to determine whether to `showGridOverlay` or `hideGridOverlay`.  `toggle*` functions are not great IMO because it's very easy for them to get out of sync.

[Nikita Vasilyev]

Comment on attachment 420549 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:36
>> +    margin-left: 3px;
> 
> Is the DOM tree always LTR?  If not, I think this should be `margin-inline-start`.

It is always LTR.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:42
>> +    color: white;
> 
> dark mode?

I attached a video. It's white text on blue background on both light and dark modes.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:48
>> +    border-color: transparent;
> 
> Why are we removing the border and background when the element is selected?  This looks really weird IMO (especially when unfocused, as it's like the badge disappears and there's this random text left there).

I agree this can be improved. I'll reiterate on this.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:450
>> +        }
> 
> You probably need to add/remove these event listeners inside `_handleLayoutContextTypeChanged` in case a node that initially wasn't a grid context later becomes one.

Oh, good catch.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:2047
>> +        WI.overlayManager.toggleGridOverlay(this.representedObject);
> 
> IMO we should maintain some state on `WI.DOMNode` that indicates what overlays are currently shown and use that to determine whether to `showGridOverlay` or `hideGridOverlay`.  `toggle*` functions are not great IMO because it's very easy for them to get out of sync.

WI.OverlayManager seems more appropriate for this. It already has ` _gridOverlayForNodeMap`. I don't see the case when it gets out of sync but wouldn't with the state being on `WI.DOMNode`.

[Nikita Vasilyev]

Comment on attachment 420549 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:26
>> +.tree-outline.dom .badge-css-grid {
> 
> I think this should be more specific like `.tree-outline.dom li > .badge-css-grid` so that there's less of a chance of a selector clash.

`.tree-outline.dom li > .badge-css-grid` won't work — it would have to be `.tree-outline.dom li > span > .badge-css-grid`.

I'm not a fan of overcomplicating selectors. It would agree with you if one badge could be inside of another. It isn't the case here.

[Nikita Vasilyev]

Created attachment 420763 [details]
[Image] Selected element, gray badge

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

>>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.css:48
>>> +    border-color: transparent;
>> 
>> Why are we removing the border and background when the element is selected?  This looks really weird IMO (especially when unfocused, as it's like the badge disappears and there's this random text left there).
> 
> I agree this can be improved. I'll reiterate on this.

In my very first iteration, non-activated badges looked the same on selected and non-selected elements. Is that what you're suggesting? It looks a bit foreign on macOS.

[Nikita Vasilyev]

Created attachment 420767 [details]
Patch

[Nikita Vasilyev]

Created attachment 420768 [details]
[Video] With patch applied

I added borders for badges when the element is selected.

Note that badges may end up looking different after Bug 221923: Web Inspector: Display color swatch inside of "grid" element badge.

[Blaze Burg]

Comment on attachment 420767 [details]
Patch

r=me.

The code changes look good to me. At this point it's tweaking color styles. As we discussed, the  gray on gray on gray scheme for inactive badge in an inactive window has low contrast and doesn't look like a button. To be consistent with our other button UI (especially scope bar), the background color shouldn't change when the DOMTreeElement selection color goes from blue to gray upon losing keyboard focus or window focus.

I'm open to considering a wholesale change to dim our custom buttons when losing window focus. Even if it's a good idea, it should not be considered as part of this button patch.

[Nikita Vasilyev]

Comment on attachment 420767 [details]
Patch

I'll iterate on the colors in 'Bug 221923: Web Inspector: Display color swatch inside of "grid" element badge' or even before that.

[EWS]

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

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