Bug 222364 - Web Inspector: List of grid nodes is incomplete in Layout sidebar panel
Summary: Web Inspector: List of grid nodes is incomplete in Layout sidebar panel
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: Razvan Caliman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-24 09:12 PST by Razvan Caliman
Modified: 2021-02-25 12:31 PST (History)
12 users (show)

See Also:


Attachments
WIP (1.72 KB, patch)
2021-02-24 09:35 PST, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch (3.58 KB, patch)
2021-02-25 10:04 PST, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch (3.59 KB, patch)
2021-02-25 11:11 PST, Razvan Caliman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Razvan Caliman 2021-02-24 09:12:35 PST
The list of grid nodes is incomplete when opening Web Inspector and the Layout sidebar panel is selected by default.

Steps to reproduce:

- Open Web Inspector and select the Layout sidebar panel of the Elements Tab
- Close Web Inspector 
- Navigate to https://stripe.com
- Open Web Inspector again

Expected result:
 
A long list of grid nodes (100+) in the CSS Grid section of the Layout sidebar panel.

Actual result:

Either no nodes or a short list of nodes matching any grid nodes already expanded in the DOM tree.
Comment 1 Razvan Caliman 2021-02-24 09:16:05 PST
Workaround: 

Select another sidebar panel (ex: Computed), then select Layout again.
This populates the list with all the grid nodes.
Comment 2 Razvan Caliman 2021-02-24 09:35:38 PST
Created attachment 421416 [details]
WIP

quick hack that works
Comment 3 Razvan Caliman 2021-02-24 09:52:20 PST
Seems that the backend doesn't push nodes to the frontend when first setting `WI.cssManager.layoutContextTypeChangedMode = WI.CSSManager.LayoutContextTypeChangedMode.All` in `WI.LayoutDetailsSidebarPanel.attached()`

When that is set, `WI.DOMNode` instances for grid nodes should be created automatically  and `WI.DOMNode.Event.LayoutContextTypeChanged` events should fire (see last line of `WI.DOMNode` constructor). But none do. The workaround of switching between panels unblocks this and events come through as expected.

The only thing that jumps out is that `WI.LayoutDetailsSidebarPanel.detached()` changes the mode to "Observed". 🤔 

Strangely, toggling the mode in `WI.LayoutDetailsSidebarPanel.attached()` solves the bug:

```diff
+ WI.cssManager.layoutContextTypeChangedMode = WI.CSSManager.LayoutContextTypeChangedMode.Observed;
  WI.cssManager.layoutContextTypeChangedMode = WI.CSSManager.LayoutContextTypeChangedMode.All;
```

See [WIP](https://bugs.webkit.org/attachment.cgi?id=421416)

This is a hack.

@Patrick Angle, is there any reason why nodes wouldn't get pushed to the frontend when first setting the mode to "All"?
Comment 4 Radar WebKit Bug Importer 2021-02-24 10:06:55 PST
<rdar://problem/74700960>
Comment 5 Patrick Angle 2021-02-24 10:15:51 PST
(In reply to Razvan Caliman from comment #3)
> @Patrick Angle, is there any reason why nodes wouldn't get pushed to the
> frontend when first setting the mode to "All"?

I think I see my mistake. I'm not honoring the current layout state when enabling the `WebCore::InspectorCSSAgent` because the assumption is that it will be in the `Observed` state, but that isn't the case here because it was already in the `All` state when we enable it. Two solutions here, but I think the better one is to add `m_layoutContextTypeChangedMode = Protocol::CSS::LayoutContextTypeChangedMode::Observed;` to `WebCore::InspectorCSSAgent::reset`. This would be consistent with other properties that get reset when the agent is reset (which also happens when the agent is disabled).
Comment 6 Razvan Caliman 2021-02-25 10:04:04 PST
Created attachment 421538 [details]
Patch
Comment 7 EWS Watchlist 2021-02-25 10:04:53 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 8 Patrick Angle 2021-02-25 10:57:07 PST
Comment on attachment 421538 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/CSS.json:266
> +            "description": "The mode for how layout context type changes are handled (default: observed). <code>Observed</code> limits handling to those nodes already known to the frontend by other means (generally, this means the node is a visible item in the Elements tab). <code>All</code> informs the frontend of all layout context type changes and all nodes with a known layout context are sent to the frontend."

NIT: `(default: <code>Observed</code>)`
Comment 9 Razvan Caliman 2021-02-25 11:11:25 PST
Created attachment 421548 [details]
Patch

Address nit
Comment 10 Devin Rousso 2021-02-25 11:49:44 PST
Comment on attachment 421548 [details]
Patch

r=me assuming EWS is green :)
Comment 11 BJ Burg 2021-02-25 12:24:16 PST
Comment on attachment 421548 [details]
Patch

mac-wk2 passed, setting cq+
Comment 12 EWS 2021-02-25 12:31:05 PST
Committed r273502: <https://commits.webkit.org/r273502>

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