Bug 225317 – Web Inspector: Sources: Inconsistent selection in source tree when grouped by path

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

[Razvan Caliman]

Created attachment 427578 [details]
Reduced test case

<rdar://75004834>

*Steps to reproduce:*

- Load the attachment (sources-path.html) in a new tab
- Open Web Inspector > Sources
- Group resources by path
- Click the top-level tree elements for each domain

*Expected result:*

All tree items can be selected.

*Actual result:*

Tree items for domains for font resources don't get selected (fonts.googleapis.com, fonts.gstatic.com). The top-level tree item for the main frame gets selected instead.

[Razvan Caliman]

Created attachment 427579 [details]
Video recording of bug

[Razvan Caliman]

Created attachment 427583 [details]
Patch

[Razvan Caliman]

Created attachment 427584 [details]
Patch

[Devin Rousso]

Comment on attachment 427584 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1050
> +                        let representedObject = resource.type === WI.Resource.Type.Document ? resource : null;

Shouldn't this be `resource.parentFrame`?  I think we want a different `representedObject` as otherwise the same `WI.Resource` would have two `WI.TreeElement` in the same `WI.TreeOutline`.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1051
> +                        originTreeElement = new WI.OriginTreeElement(origin, representedObject, {hasChildren: true});

What does this show if `representedObject === null`?  Is it just like any other (non-Images) folder in that it just shows some text?

[Razvan Caliman]

Created attachment 427586 [details]
Patch

[Razvan Caliman]

(In reply to Devin Rousso from comment #4)
> > Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1050
> > +                        let representedObject = resource.type === WI.Resource.Type.Document ? resource : null;
> 
> Shouldn't this be `resource.parentFrame`?  I think we want a different
> `representedObject` as otherwise the same `WI.Resource` would have two
> `WI.TreeElement` in the same `WI.TreeOutline`.

Yes, thanks for catching this!

> 
> > Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1051
> > +                        originTreeElement = new WI.OriginTreeElement(origin, representedObject, {hasChildren: true});
> 
> What does this show if `representedObject === null`?  Is it just like any
> other (non-Images) folder in that it just shows some text?

Same behavior as with clicking on nested folders in Sources when grouped By Path: the previously rendered content stays in the main area until explicitly selecting another resource.

[Devin Rousso]

Comment on attachment 427586 [details]
Patch

r=me

[Joseph Pecoraro]

Comment on attachment 427586 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1051
> +                        originTreeElement = new WI.OriginTreeElement(origin, representedObject, {hasChildren: true});

Should we assert in `WI.OriginTreeElement` so that this doesn't happen again? And what would happen if we use `null` here?

[Razvan Caliman]

Created attachment 427664 [details]
Patch

Carry over R+; Address code review feedback

[EWS]

Committed r276958 (237291@main): <https://commits.webkit.org/237291@main>

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