WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
224772
Web Inspector: REGRESSION(
r270134
) Elements Tab: Details Sidebar toggle is unexpectedly disabled after switching from Timelines tab
https://bugs.webkit.org/show_bug.cgi?id=224772
Summary
Web Inspector: REGRESSION(r270134) Elements Tab: Details Sidebar toggle is un...
Razvan Caliman
Reported
2021-04-19 11:13:34 PDT
<
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.
Attachments
Patch
(3.41 KB, patch)
2021-04-19 12:54 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch
(3.51 KB, patch)
2021-04-20 08:23 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch
(4.20 KB, patch)
2021-04-22 07:01 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch
(4.76 KB, patch)
2021-04-23 05:33 PDT
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Razvan Caliman
Comment 1
2021-04-19 12:54:05 PDT
Created
attachment 426465
[details]
Patch
Devin Rousso
Comment 2
2021-04-19 13:52:27 PDT
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
Comment 3
2021-04-20 08:23:20 PDT
Created
attachment 426548
[details]
Patch Update Changelog
Razvan Caliman
Comment 4
2021-04-22 07:01:10 PDT
Created
attachment 426808
[details]
Patch
Devin Rousso
Comment 5
2021-04-22 11:00:53 PDT
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
Comment 6
2021-04-23 05:33:40 PDT
Created
attachment 426900
[details]
Patch
Devin Rousso
Comment 7
2021-04-23 10:19:25 PDT
Comment on
attachment 426900
[details]
Patch r=me
EWS
Comment 8
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug