Bug 225317

Summary: Web Inspector: Sources: Inconsistent selection in source tree when grouped by path
Product: WebKit Reporter: Razvan Caliman <rcaliman>
Component: Web InspectorAssignee: Razvan Caliman <rcaliman>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Reduced test case
none
Video recording of bug
none
Patch
none
Patch
none
Patch
none
Patch none

Description Razvan Caliman 2021-05-03 11:12:34 PDT
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.
Comment 1 Razvan Caliman 2021-05-03 11:13:44 PDT
Created attachment 427579 [details]
Video recording of bug
Comment 2 Razvan Caliman 2021-05-03 12:00:54 PDT
Created attachment 427583 [details]
Patch
Comment 3 Razvan Caliman 2021-05-03 12:03:38 PDT
Created attachment 427584 [details]
Patch
Comment 4 Devin Rousso 2021-05-03 12:06:41 PDT
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?
Comment 5 Razvan Caliman 2021-05-03 12:23:58 PDT
Created attachment 427586 [details]
Patch
Comment 6 Razvan Caliman 2021-05-03 12:29:14 PDT
(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.
Comment 7 Devin Rousso 2021-05-03 13:03:06 PDT
Comment on attachment 427586 [details]
Patch

r=me
Comment 8 Joseph Pecoraro 2021-05-03 13:43:04 PDT
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?
Comment 9 Razvan Caliman 2021-05-04 07:37:11 PDT
Created attachment 427664 [details]
Patch

Carry over R+; Address code review feedback
Comment 10 EWS 2021-05-04 08:27:05 PDT
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].