The following error is printed to the console when opening Web Inspector: resource:///org/webkit/inspector/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:66:23: CONSOLE ERROR Shown panel style-rules must be visible. false
shown@resource:///org/webkit/inspector/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:66:131 selectedSidebarPanel@resource:///org/webkit/inspector/UserInterface/Views/Sidebar.js:139:45 showDetailsSidebarPanels@resource:///org/webkit/inspector/UserInterface/Views/ContentBrowserTabContentView.js:200:38 _contentBrowserCurrentRepresentedObjectsDidChange@resource:///org/webkit/inspector/UserInterface/Views/ContentBrowserTabContentView.js:331:38 dispatch@resource:///org/webkit/inspector/UserInterface/Base/Object.js:165:30 dispatchEventToListeners@resource:///org/webkit/inspector/UserInterface/Base/Object.js:172:17 resource:///org/webkit/inspector/UserInterface/Views/ContentBrowser.js:96:42 _execute@resource:///org/webkit/inspector/UserInterface/Base/Debouncer.js:132:29 resource:///org/webkit/inspector/UserInterface/Base/Debouncer.js:75:26
Created attachment 381847 [details] Patch
Comment on attachment 381847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381847&action=review > Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:212 > - if (!WI.detailsSidebar.sidebarPanels.length) > + if (!currentSidebarPanels.length) This change shouldn't actually modify any logic. `currentSidebarPanels` should be a reference to the same array instance as `WI.detailsSidebar.sidebarPanels`. As such, please revert this. > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:139 > + if (!this.collapsed) { This part looks fine, but I'd like to test it myself before I say yes/no.
Comment on attachment 381847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381847&action=review >> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:212 >> + if (!currentSidebarPanels.length) > > This change shouldn't actually modify any logic. `currentSidebarPanels` should be a reference to the same array instance as `WI.detailsSidebar.sidebarPanels`. As such, please revert this. Yes, it doesn't change any logic. Its a leftover from me try to fix the logic in this method first. A mix of currentSidebarPanels.length and WI.detailsSidebar.sidebarPanels.length in this method is somewhat confusing that's why I updated that for consistency. So let me know if you'd still prefer to revert it. >> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:139 >> + if (!this.collapsed) { > > This part looks fine, but I'd like to test it myself before I say yes/no. SG.
Comment on attachment 381847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381847&action=review r=me, I think this is fine. I tried moving around between various tabs and changing state when certain panels weren't visible and it didn't seem to cause any regressions. >>> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:212 >>> + if (!currentSidebarPanels.length) >> >> This change shouldn't actually modify any logic. `currentSidebarPanels` should be a reference to the same array instance as `WI.detailsSidebar.sidebarPanels`. As such, please revert this. > > Yes, it doesn't change any logic. Its a leftover from me try to fix the logic in this method first. A mix of currentSidebarPanels.length and WI.detailsSidebar.sidebarPanels.length in this method is somewhat confusing that's why I updated that for consistency. So let me know if you'd still prefer to revert it. I think I'd prefer it as it was (and even to have the other cases of `currentSidebarPanels` be removed/renamed), as there's no immediate indication that `currentSidebarPanels` is ever modified. Since we're calling a bunch of `add*`/`remove*` methods on `WI.detailsSidebar`, it makes more sense that we'd check it there. Plus, using `currentSidebarPanels` because it's the same object is an implementation detail that could theoretically change in the future, and we'd never know about it causing issues here with the change. >>> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:139 >>> + if (!this.collapsed) { >> >> This part looks fine, but I'd like to test it myself before I say yes/no. > > SG. Should we also be doing this for the `hidden()` call above?
Created attachment 382467 [details] Patch for landing
Comment on attachment 381847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381847&action=review >>>> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:212 >>>> + if (!currentSidebarPanels.length) >>> >>> This change shouldn't actually modify any logic. `currentSidebarPanels` should be a reference to the same array instance as `WI.detailsSidebar.sidebarPanels`. As such, please revert this. >> >> Yes, it doesn't change any logic. Its a leftover from me try to fix the logic in this method first. A mix of currentSidebarPanels.length and WI.detailsSidebar.sidebarPanels.length in this method is somewhat confusing that's why I updated that for consistency. So let me know if you'd still prefer to revert it. > > I think I'd prefer it as it was (and even to have the other cases of `currentSidebarPanels` be removed/renamed), as there's no immediate indication that `currentSidebarPanels` is ever modified. Since we're calling a bunch of `add*`/`remove*` methods on `WI.detailsSidebar`, it makes more sense that we'd check it there. Plus, using `currentSidebarPanels` because it's the same object is an implementation detail that could theoretically change in the future, and we'd never know about it causing issues here with the change. Agreed. Updated the code to not use currentSidebarPanels at all and access WI.detailsSidebar.sidebarPanels directly instead. >>>> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:139 >>>> + if (!this.collapsed) { >>> >>> This part looks fine, but I'd like to test it myself before I say yes/no. >> >> SG. > > Should we also be doing this for the `hidden()` call above? I considered that but it could lead to a situation when hidden() is not called at all if the Sidebar is collapsed and _selectedSidebarPanel changes.
Comment on attachment 382467 [details] Patch for landing Clearing flags on attachment: 382467 Committed r251853: <https://trac.webkit.org/changeset/251853>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56785760>