Bug 221763 – Web Inspector: Grid list in Layout panel missing empty message

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

[Razvan Caliman]

When inspecting a page with no CSS Grid contexts, the grid list in the Layout panel should show an empty message instead of the "Grid Overlays" header.

[Radar WebKit Bug Importer]

<rdar://problem/74242610>

[Razvan Caliman]

Created attachment 420453 [details]
Patch

[Razvan Caliman]

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

[Devin Rousso]

Comment on attachment 420453 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:110
> +        this._gridDetailsSectionGroup = new WI.DetailsSectionGroup([]);

Rather than saving the `WI.DetailsSectionGroup`, you always need to have a `WI.DetailsSectionRow`, so why not create that here and save that to a member?
```
    this._gridDetailsSectionRow = new WI.DetailsSectionRow(WI.UIString("No CSS Grid Contexts", "No CSS Grid Contexts @ Layout Details Sidebar Panel", "Message shown when there are no CSS Grid contexts on the inspected page."));
    let gridGroup = new WI.DetailsSectionGroup([this._gridDetailsSectionRow]);
```
and then later you can call `this._gridDetailsSectionRow.showEmptyMessage()` or `this._gridDetailsSectionRow.hideEmptyMessage()` as needed.

Frankly tho it may be better for data encapsulation and clarity to modify `WI.GridDetailsSection` to instead subclass `WI.DetailsSectionRow` (similar to what `WI.BoxModelDetailsSectionRow` does).

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:119
> +        if (this._gridNodeSet.size === 0)

Style: we avoid explicitly checking against `0` and instead use `!`
```
    if (!this._gridNodeSet.size)
```

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:190
> +        this._noGridMessage = new WI.DetailsSectionRow(WI.UIString("No CSS Grid contexts found", "No CSS Grid contexts found @ Elements details sidebar", "Message shown when there are no CSS Grid contexts on the inspected page."));

NIT: I think "found" is a bit redundant
NIT: since this isn't a full sentence, I think we should capitalize "Contexts"

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:198
> +        this.needsLayout();

NIT: Since this is called inside `attached` I'm think it will already be in a dirty state, so this call is possibly unnecessary.

[Razvan Caliman]

Created attachment 420517 [details]
Patch

[Razvan Caliman]

Comment on attachment 420453 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:110
>> +        this._gridDetailsSectionGroup = new WI.DetailsSectionGroup([]);
> 
> Rather than saving the `WI.DetailsSectionGroup`, you always need to have a `WI.DetailsSectionRow`, so why not create that here and save that to a member?
> ```
>     this._gridDetailsSectionRow = new WI.DetailsSectionRow(WI.UIString("No CSS Grid Contexts", "No CSS Grid Contexts @ Layout Details Sidebar Panel", "Message shown when there are no CSS Grid contexts on the inspected page."));
>     let gridGroup = new WI.DetailsSectionGroup([this._gridDetailsSectionRow]);
> ```
> and then later you can call `this._gridDetailsSectionRow.showEmptyMessage()` or `this._gridDetailsSectionRow.hideEmptyMessage()` as needed.
> 
> Frankly tho it may be better for data encapsulation and clarity to modify `WI.GridDetailsSection` to instead subclass `WI.DetailsSectionRow` (similar to what `WI.BoxModelDetailsSectionRow` does).

This is clever! I like it! Updated.

> Frankly tho it may be better for data encapsulation and clarity to modify `WI.GridDetailsSection` to instead subclass `WI.DetailsSectionRow` (similar to what `WI.BoxModelDetailsSectionRow` does).

That would be elegant, but we can't do it straight away. As discussed on private chat, `WI.DetailsSectionRow` extends `WI.Object` not `WI.View` which would prevent from using `attached()`/`detached()`/`needsLayout()` and other handy methods.
Leaving it as is for now and added a FIXME to investigate the effort of updating `WI.DetailsSection` & friends to extend `WI.View`

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:119
>> +        if (this._gridNodeSet.size === 0)
> 
> Style: we avoid explicitly checking against `0` and instead use `!`
> ```
>     if (!this._gridNodeSet.size)
> ```

Done!

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:190
>> +        this._noGridMessage = new WI.DetailsSectionRow(WI.UIString("No CSS Grid contexts found", "No CSS Grid contexts found @ Elements details sidebar", "Message shown when there are no CSS Grid contexts on the inspected page."));
> 
> NIT: I think "found" is a bit redundant
> NIT: since this isn't a full sentence, I think we should capitalize "Contexts"

Done!

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:198
>> +        this.needsLayout();
> 
> NIT: Since this is called inside `attached` I'm think it will already be in a dirty state, so this call is possibly unnecessary.

Indeed, it was unnecessary. Removed.

[Devin Rousso]

Comment on attachment 420517 [details]
Patch

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

r=me, nice fix :)

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:128
> +        }
> +        else {

Style: we put `else` on the same line as `}`

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:131
> +                this._gridSection = new WI.CSSGridSection;

IMO you could/should just create this once in `initialLayout` to avoid having extra allocations/deallocations.  You could then always know that `this._gridSection` is valid and not ever have to invalidate (`= null;`) it.

You are correct tho in calling `removeSubview` and `appendChild`/`addSubview` because `WI.DetailsSectionRow` will `removeChildren()` when it shows/hides the empty message, so make sure to keep those bits :)

[Razvan Caliman]

Comment on attachment 420517 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:131
>> +                this._gridSection = new WI.CSSGridSection;
> 
> IMO you could/should just create this once in `initialLayout` to avoid having extra allocations/deallocations.  You could then always know that `this._gridSection` is valid and not ever have to invalidate (`= null;`) it.
> 
> You are correct tho in calling `removeSubview` and `appendChild`/`addSubview` because `WI.DetailsSectionRow` will `removeChildren()` when it shows/hides the empty message, so make sure to keep those bits :)

Setting up `this._gridSection = new WI.CSSGridSection` in `initialLayout()` can work. But it doesn't preclude from checking a flag as to whether it was already added or removed from the view. Repeated calls to addSubview/removeSubview trip assertions about duplicate calls.

> You could then always know that `this._gridSection` is valid and not ever have to invalidate (`= null;`) it.
We use the truthiness of this value to decide whether to create and **append** `CSSGridSection` to the view or to **remove** it. `layout()` can get called multiple times during a debugging session.

We must support cases where the empty state flips on and off during a debugging session. For example, the scenario in https://bugs.webkit.org/show_bug.cgi?id=221919 where the grid context toggles on and off based on media queries.

However, in most scenarios, we expect either no grid contexts on the page ever (always show empty message), or at least one grid context which doesn't change state (always show list of grids).

[Razvan Caliman]

Created attachment 420535 [details]
Patch

[EWS]

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

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