Bug 224772

Summary: Web Inspector: REGRESSION(r270134) Elements Tab: Details Sidebar toggle is unexpectedly disabled after switching from Timelines tab
Product: WebKit Reporter: Razvan Caliman <rcaliman>
Component: Web InspectorAssignee: Razvan Caliman <rcaliman>
Severity: Normal CC: ews-watchlist, hi, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Description Flags
Patch none

Description Razvan Caliman 2021-04-19 11:13:34 PDT

Occurs when starting Web Inspector on any tab other than Elements.

Steps to reproduce:
1. Ensure Timelines Tab will open on Inspector launch (select Timelines, close inspector)
2. Open Inspector, timelines tab should be showing.
3. Switch to Elements Tab
=> Sidebar Pane toggle is disabled
    - Icon Tooltip: "Hide the details sidebar"
=> Elements Tab does not have an actual selection.
    - Breadcrumbs are not shown in the navigation bar.

Workaround: select a new DOM node and the sidebars come back with the icon toggled on.
Comment 1 Razvan Caliman 2021-04-19 12:54:05 PDT
Created attachment 426465 [details]
Comment 2 Devin Rousso 2021-04-19 13:52:27 PDT
Comment on attachment 426465 [details]

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

> Source/WebInspectorUI/ChangeLog:10
> +        because`WI.DOMTreeOutline.selectedTreeElement`, not `WI.DOMTreeOutline.selectedDOMNode`, is ultimately used to

missing space between "... because `WI.DOMTreeOutline..."

> Source/WebInspectorUI/ChangeLog:11
> +        identify the current represented object  when rendering the Elements tab details sidebar panels.

Style: we capitalize "Tab" when used with a tab name (e.g. "Elements Tab" vs "a tab")

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:521
> +        this.dispatchEventToListeners(WI.ContentView.Event.SelectionPathComponentsDidChange);

I'm slightly confused.  `WI.DOMTreeContentView.prototype._selectedNodeDidChange` should already be called in response to `WI.DOMTreeContentView.prototype._domTreeSelectionDidChange`.  `WI.DOMTreeContentView.prototype._selectedNodeDidChange` is the event handler for `WI.DOMTreeOutline.Event.SelectedNodeChanged`, which is dispatched inside `WI.DOMTreeOutline.prototype._selectedNodeChanged`, which is called by `WI.DOMTreeOutline.prototype.selectDOMNode`.  So why is it that `WI.DOMTreeOutline.Event.SelectedNodeChanged` isn't getting dispatched?  Is the `_selectedDOMNode` already somehow set?
Comment 3 Razvan Caliman 2021-04-20 08:23:20 PDT
Created attachment 426548 [details]

Update Changelog
Comment 4 Razvan Caliman 2021-04-22 07:01:10 PDT
Created attachment 426808 [details]
Comment 5 Devin Rousso 2021-04-22 11:00:53 PDT
Comment on attachment 426808 [details]

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

> Source/WebInspectorUI/UserInterface/Views/ElementsTabContentView.js:117
>      }

Please still call `super.detached()` at the end just in case one of the inherited classes decides to add a `detached` in the future so we don't subtly miss out on that behavior.  Also, before this change we would've used `super.detached` by default, so we should preserve that.

As a general rule of thumb we almost always (and you need a good reason not to) want to call `super` for any methods inherited from `WI.View`.

> Source/WebInspectorUI/UserInterface/Views/ElementsTabContentView.js:143
> +    _showDOMTreeContentViewIfNeeded()

I think we can get rid of `_showDOMTreeContentView` while we're at it, as all the callsites are being called either
 - in response to a main frame/resource change, so we should have a new `WI.DOMTree` anyways
 - when there is currently no content view being shown, meaning we should show one with whatever the main frame main resource `WI.DOMTree` is right now

> Source/WebInspectorUI/UserInterface/Views/ElementsTabContentView.js:147
> +        if (mainDOMTree && this.contentBrowser.currentContentView?.representedObject !== mainDOMTree);
> +            this.contentBrowser.showContentViewForRepresentedObject(mainDOMTree);

NIT: I'd maybe turn this into an series of early-return so that it's clearer what the cases are for the "if needed" part
    let mainDOMTree = WI.networkManager.mainFrame?.domTree;
    if (!mainDOMTree)

    if (this.contentBrowser.currentContentView?.representedObject === mainDOMTree)

Comment 6 Razvan Caliman 2021-04-23 05:33:40 PDT
Created attachment 426900 [details]
Comment 7 Devin Rousso 2021-04-23 10:19:25 PDT
Comment on attachment 426900 [details]

Comment 8 EWS 2021-04-26 05:28:04 PDT
Committed r276587 (237023@main): <https://commits.webkit.org/237023@main>

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