Bug 224772 – Web Inspector: REGRESSION(r270134) Elements Tab: Details Sidebar toggle is unexpectedly disabled after switching from Timelines tab

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

[Razvan Caliman]

<rdar://73470211>

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.

[Razvan Caliman]

Created attachment 426465 [details]
Patch

[Devin Rousso]

Comment on attachment 426465 [details]
Patch

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?

[Razvan Caliman]

Created attachment 426548 [details]
Patch

Update Changelog

[Razvan Caliman]

Created attachment 426808 [details]
Patch

[Devin Rousso]

Comment on attachment 426808 [details]
Patch

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)
        return;

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

    this.contentBrowser.showContentViewForRepresentedObject(mainDOMTree);
```

[Razvan Caliman]

Created attachment 426900 [details]
Patch

[Devin Rousso]

Comment on attachment 426900 [details]
Patch

r=me

[EWS]

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].