Currently, everything is shoved into one details sidebar. If the screen is wide enough we should support 3 panes, so that we can have this layout: DOM Tree | Styles for Selected Element | Additional Information / Tools If the inspector is too narrow, then keep the auxiliary views in the second column and don't offer to pop them out. Expected UI: - Similar to Firefox, show a popout/pop in button in the second column's navigation bar. - Upon clicking, auxiliary views (everything except Styles sidebar?) moves to a third column with its own navigation bar. - Popout can be reverted by clilcking the button again. - To determine when popout should be offered, set a reasonable min-width for auxiliary views. - Three columns should each be resizable horizontally.
<rdar://67879622>
Created attachment 410667 [details] Patch
Comment on attachment 410667 [details] Patch Clearing r? as this is a work in progress.
Comment on attachment 410667 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410667&action=review > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1417 > +localizedStrings["Use 3-Panel layout"] = "Use 3-Panel layout"; Nit: three-pane layout > Source/WebInspectorUI/UserInterface/Base/Main.js:285 > + WI.promotedDetailsSidebar = new WI.Sidebar(document.getElementById("promoted-details-sidebar"), WI.Sidebar.Sides.Right, null, null, WI.UIString("Promoted Details"), false); Nit: I don't think Promoted Details is a term we want in the UI. > Source/WebInspectorUI/UserInterface/Base/Main.js:312 > + WI.tabBrowser = new WI.TabBrowser(document.getElementById("tab-browser"), WI.tabBar, WI.navigationSidebar, WI.detailsSidebar, WI.promotedDetailsSidebar); Hmm this reads weird to me. WI.secondaryDetailsSidebar would be better IMO. And at that point, maybe rename the first to WI.primaryDetailsSidebar. > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:398 > + experimentalSettingsView.addSetting(WI.UIString("Elements Tab:"), WI.settings.experimentalEnableThreePaneElementsTab, WI.UIString("Enable 3-Panel Layout")); Nit: consider a UIString key/description for 'Elements Tab:'
Created attachment 411158 [details] Patch
Created attachment 411174 [details] [Image] Align with toolbars At the high level, this works great! UI polish: it would be nice to reduce the vertical spacing around Active/Focus/Hover/Visited to match the height of the neighboring toolbar.
With the patch applied, Active/Focus/Hover/Visited section is shown in both panels when Computed is selected. I suggest to hide the one in Computed when the 3-pane layout is enabled. If this is easier, it could be done with CSS: .sidebar.trailing > .panel.details.style-computed > .content > .pseudo-classes { display: none; }
Comment on attachment 411158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411158&action=review r-, but this is a good start! In the future, a screenshot/video would've really helped here :) I started reviewing the actual code, but I think I have some questions about the design so I'm gonna stop doing specific comments until those are answered/resolved. Having an explicitly "called out" primary/secondary sidebar seems perhaps unnecessarily restrictive, and maybe a bit verbose. When we'd chatted about this offline, my thinking was more along the lines of "have the tab indicate whether it accepts more than one sidebar (either as a `true`/`false` or as an upper limit of the number of sidebars it can accommodate) and then have each panel indicate whether it prefers being independent (similar to the `exclusive` property of `WI.ScopeBarItem` or even `WI.NavigationItem.VisibilityPriority`). The reason I suggested this is because I think we should only be showing more than one sidebar when we have the room for it (i.e. when Web Inspector is wide enough), and on _really_ large screens we then could even have more than just two sidebars (e.g. i would love to have [Styles | Computed | Changes] and [Node | Layers] so that I can see CSS and Event Listeners side by side, but that would require additional work to allow users to reorder sidebar panels across multiple sidebars). Also, I think assuming that the 0th sidebar is "the one that should be separate" is something that could be inflexible to future change and frankly kinda arbitrary. At the very least it should be a property. btw, it seems like with this patch that there would be no `WI.NavigationBar` at the top of the Styles panel? I think that would look really out of place compared to the rest of Web Inspector. Perhaps we could adjust the pseudo-class toggles to always be visible when the Styles panel is in it's own sidebar so that the spacing of things is at least preserved? > Source/WebInspectorUI/UserInterface/Base/Main.js:284 > + WI.detailsSidebar = new WI.SidebarSet(document.getElementById("details-sidebar"), WI.Sidebar.Sides.Trailing, null, WI.UIString("Details")); this name should probably be changed > Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:80 > + if (this._detailsSidebarPanelConstructors.length) { NIT: why not nest this above? ``` if (this._detailsSidebarPanelConstructors.length) { if (!this.allowsMultipleDetailSidebars) { ... } ... } ``` > Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:209 > + WI.detailsSidebar.secondaryPanel = this.detailsSidebarPanels[0]; The naming of this almost seems kinda backwards to me 🤔 My first thought of this naming is that the primary/important panel would be the Styles panel and then the secondary/ancillary panel(s) would be all the other ones. > Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:230 > + if (this._showDetailsSidebarItem) We should always allow the details sidebar to be collapsed. IMO if the user has decided to enable the setting for multiple sidebar panels, we should always respect that so long as Web Inspector is wide enough. I think the navigation item for expanding/collapsing the details sidebar should show/hide the all of the details sidebars, not just whether or not we show the secondary panel. > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:35 > + this._allowResizeToCollapse = true; NIT: maybe `allowResizingToCollapse`? > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:50 > + this._navigationBarAdditionalLeadingItems = new WI.GroupNavigationItem; Using `Items` here makes me think that this is an array, not a `WI.GroupNavigationItem`. Perhaps `navigationBarAdditionalLeadingItemsGroup.navigationItems`? Also, rather than expose the `WI.GroupNavigationItem` itself, maybe we should only expose a `set leadingNavigationItems(navigationItems) { this._navigationBarAdditionalLeadingItemsGroup.navigationItems = navigationItems; }` for better encapsulation? wdyt? > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:51 > + this._navigationBarAdditionalTrailingItems = new WI.GroupNavigationItem; is this actually needed? > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:96 > + // Index is +2 because of _navigationBarAdditionalLeadingItems and a FlexibleSpaceNavigationItem > + this._navigationBar.insertNavigationItem(sidebarPanel.navigationItem, index + 2); This seems like a hack. Could you create a `WI.GroupNavigationItem` for the panel navigation items in the `constructor` and add/remove items from that instead? > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:165 > + get allowResizeToCollapse() > + { > + return this._allowResizeToCollapse; > + } > + > + set allowResizeToCollapse(allow) > + { > + this._allowResizeToCollapse = allow; > + } Style: simple `get`/`set` can be collapsed onto a single line ``` get allowResizingToCollapse() { return this._allowResizingToCollapse; } set allowResizingToCollapse(allowResizingToCollapse) { this._allowResizingToCollapse = allowResizingToCollapse; } ``` > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:235 > + get navigationBarAdditionalLeadingItems() > + { > + return this._navigationBarAdditionalLeadingItems; > + } ditto (:157) ``` get navigationBarAdditionalLeadingItems() { return this._navigationBarAdditionalLeadingItems; } ``` > Source/WebInspectorUI/UserInterface/Views/SidebarSet.css:39 > + width: 0 !important; > + border: none !important; > + display: none; Why do we need `width`/`border` if we're already `display: none;`? > Source/WebInspectorUI/UserInterface/Views/SidebarSet.css:40 > +} NIT: missing trailing newline > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:45 > + this._primarySidebar = new WI.Sidebar(null, this._side, null, null, label, true) Style: missing `;` > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:46 > + this._primarySidebar.addEventListener(WI.Sidebar.Event.CollapsedStateDidChange, this._sidebarCollapsedStateDidChange, this, {sidebar: this._primarySidebar}); what is the `{sidebar: this._primarySidebar}` for? NIT: I like to prefix all event listener functions with `_handle*` so that when reading just that function in isolation it's understood that it's only supposed to be called from an event listener. > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:49 > + this._secondarySidebar = new WI.Sidebar(null, this._side, null, null, label, false); It seems kinda weird to me to have both an array of sidebars and an explicitly saved property. I'd either drop the array concept or remove the explicit property. > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:50 > + this._secondarySidebar.addEventListener(WI.Sidebar.Event.CollapsedStateDidChange, this._sidebarCollapsedStateDidChange, this, {sidebar: this._secondarySidebar}); what is the `{sidebar: this._secondarySidebar}` for? ditto (:46) > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:62 > + this._showSecondarySidebarNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, this._toggleSecondarySidebarVisible, this); ditto (:46) e.g. `_handleShowSecondarySidebarButtonClicked` > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:73 > + get sidebars() ditto (Sidebar.js:157) > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:78 > + get primarySidebar() ditto (Sidebar.js:157) > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:83 > + get secondarySidebar() ditto (Sidebar.js:157) > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:88 > + get sidebarPanels() ditto (Sidebar.js:157) > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:125 > + get side() ditto (Sidebar.js:157) I'd also move this up above. > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:133 > + if (!!sidebar instanceof WI.Sidebar) I don't think this is gonna do what you want it to do. We normally wrap the `instanceof` inside parenthesis and then add a `!` before it (e.g. `!(sidebar instanceof WI.Sidebar)`). In this case, however, seeing as we already have a `console.assert` (and we shouldn't ever be passing anything other than a `WI.Sidebar`), I think you can just drop the `if` entirely :) > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:138 > + this._sidebars.push(sidebar); NIT: we should also `console.assert(!this._sidebars.includes(sidebar), sidebar, this);` > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:153 > + sidebar.removeEventListener(WI.Sidebar.Event.WidthDidChange, this._sidebarWidthDidChange, this); I'd move this below the `if` so that we only do this if we know that the `sidebar` was inside `this._sidebars`. > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:158 > + if (!this._sidebars.includes(sidebar)) > + return; > + > + this._sidebars.remove(sidebar); You can combine these and avoid the second iteration since `Array.prototype.remove` (which is a utility method we define in `Source/WebInspectorUI/UserInterface/Base/Utilities.js`) returns `true`/`false` depending on whether an item was removed ``` let removed = this._sidebars.remove(sidebar); console.assert(removed, sidebar, this); ``` > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:177 > + if (this.sidebarPanels.indexOf(this.secondaryPanel) != -1 && this.sidebarPanels.indexOf(this.secondaryPanel) < index) NIT: instead of calling `indexOf` twice, can you save it to a local variable? > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:189 > + for (let sidebar of this.sidebars) { Style: no `{` `}` for single line control flow > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:210 > + for (let sidebar of this.sidebars) { ditto (:189) > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:238 > + for (let sidebar of this.sidebars) { ditto (:189) > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:288 > + } else if (typeof sidebarPanelOrIdentifierOrIndex === "number") { ditto (:189) > Source/WebInspectorUI/UserInterface/Views/SidebarSet.js:383 > + this.collapsed = this.collapsed || this.primarySidebar.collapsed; NIT: `||=` > Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:344 > + if (this._ignoreSidebarEvents || !event.data) When would `event.data` not be defined?
Created attachment 411373 [details] WIP
Created attachment 411374 [details] Demo of Attachment 411373 [details]
Created attachment 411518 [details] Patch
Created attachment 411579 [details] Patch
Created attachment 411581 [details] Demo as patched in attachment 411579 [details] Demo of Attachment 411579 [details]
Comment on attachment 411581 [details] Demo as patched in attachment 411579 [details] Looking at this video again, it appears the math to collapse the extra sidebars isn't happening quite soon enough, and there is about a 6-8 pixel range where part of the rightmost sidebar gets cut off before hiding/after showing the extra sidebar.
Comment on attachment 411579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411579&action=review This is looking really good! I think the UI/experience is fantastic, so all of my comments are about the code as written. Mostly it's just style issues :P > Source/WebInspectorUI/ChangeLog:19 > + laid out in natural language order when the `SidebarSet`'s side is `Leading` and reverse order when it is NIT: did you mean `MultiSidebar`? > Source/WebInspectorUI/ChangeLog:26 > + 3. The user must have requested the three-panel layout be visible. Isn't this the same as 1 now? > Source/WebInspectorUI/ChangeLog:43 > + * UserInterface/Views/ContentBrowserTabContentView.js: Added checks to ensure the details sidebar toggle button > + exists before manipulating it. Are these changes still necessary? > Source/WebInspectorUI/ChangeLog:51 > + * UserInterface/Views/GeneralStyleDetailsSidebarPanel.css: Display pseudo-classes as a navigation bar when the > + panel is in its own sidebar. Honestly, we could just always do this with `min-height: var(--navigation-bar-height);` even when not in an exclusive sidebar. We could even just always use a `WI.NavigationBar` and avoid the duplicated styles. One concern though is that if the text is too wide it may overflow/wrap, but we probably could fix that by hooking it into `get minimumWidth` (EDIT: I see you did that already, nice! 😁). > Source/WebInspectorUI/UserInterface/Base/Main.js:281 > + WI.navigationSidebar = new WI.SingleSidebar(document.getElementById("navigation-sidebar"), WI.Sidebar.Sides.Leading, null, WI.UIString("Navigation")); While you're here, can you add a comment for this UI string so that it's clear what "Navigation" means? Something like ``` WI.UIString("Navigation", "Navigation @ Sidebar", "Label for the navigation sidebar.") ``` > Source/WebInspectorUI/UserInterface/Base/Main.js:284 > + WI.detailsSidebar = new WI.MultiSidebar(document.getElementById("details-sidebar"), WI.Sidebar.Sides.Trailing, null, WI.UIString("Details")); Ditto (:281) > Source/WebInspectorUI/UserInterface/Base/Main.js:1343 > + if (WI.detailsSidebar.sidebars.includes(sidebar)) This means we're iterating `WI.detailsSidebar.sidebars` potentially three times. I think I'd restructure this (and the above) logic as ``` if (tabContentView.navigationSidebarPanel && sidebar !== WI.navigationSidebar) minimumWidth -= sidebar.width; if (tabContentView.detailsSidebarPanels) { // A sidebar within the detailsSidebar needs the minimum width of its siblings. for (let singleDetailsSidebar of WI.detailsSidebar.sidebars) { if (sidebar !== singleDetailSidebar) minimumWidth -= singleDetailSidebar.width; } } ``` > Source/WebInspectorUI/UserInterface/Base/Main.js:1344 > + if (WI.detailsSidebar.sidebars.includes(sidebar)) > + for (let singleDetailSidebar of WI.detailsSidebar.sidebars) Style: we only omit the `{` `}` when the body of the control flow is a single line > Source/WebInspectorUI/UserInterface/Base/Setting.js:225 > + experimentalEnableMultipleSidebarsInElementsTab: new WI.Setting("experimental-multiple-sidebars-in-elements-tab", false), As it's currently used, it's not restricted to the Elements Tab, so I'd probably just rename this `experimentalEnableMultipleSidebars`. This way we can also re-use this same `WI.ExperimentalSetting` for allowing other tabs to show multiple sidebars (e.g. I'd love to have the Scope Chain sidebar panel in the Sources Tab be exclusive too). > Source/WebInspectorUI/UserInterface/Views/ElementsTabContentView.js:117 > + get allowMultipleDetailSidebars() This isn't really protected as it's called from objects outside this class tree. Please move it above to public. > Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:50 > + if (this.isUsingExclusivePresentation) { ditto (ChangeLog:50) > Source/WebInspectorUI/UserInterface/Views/Main.css:185 > + min-width: 100px; Where did this number come from? Can Web Inspector can even get this small? > Source/WebInspectorUI/UserInterface/Views/Main.css:189 > + min-width: 200px; We should keep this in sync with `WI.Sidebar.AbsoluteMinimumWidth`. > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:36 > + this.element.classList.add(WI.MultiSidebar.ShowingMultipleStyleClassName); Shouldn't this only be set inside `set multipleSidebarsVisible`? > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:52 > + get primarySidebar() { return this._sidebars[this._sidebars.length - 1] } you can just use `.lastValue` Style: this logic is non-simple (e.g. returning a property with the same name as the function), so please make this multiple lines > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:54 > + get allowMultipleSidebars() { return this._allowMultipleSidebars; } Style: We only inline `get`-`set` pairs when both are inlined. In this case, since the `set` is multiple lines, please also make the `get` multiple lines. > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:60 > + this._allowMultipleSidebars = allow || false; NIT: when you want a boolean value, it's easier/safer to just use `!!` instead of having a fallback `|| false` > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:65 > + get multipleSidebarsVisible() { return this._multipleSidebarsVisible; } ditto (:54) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:71 > + this._multipleSidebarsVisible = visible || false; ditto (:60) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:87 > + this._sidebars.unshift(sidebar); Perhaps we should have `_sidebars` have the opposite order of how they're shown. Generally we should try to avoid `unshift` wherever possible. > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:90 > + this.dispatchEventToListeners(WI.MultiSidebar.Event.SidebarAdded, {sidebar: sidebar}); You can drop the `: sidebar`. ES2015 added a convenience for this situation such that the name of the variable is used as the property key. > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:109 > + get selectedSidebarPanel() { return this.primarySidebar.selectedSidebarPanel; } > + set selectedSidebarPanel(sidebarPanelOrIdentifierOrIndex) { this.primarySidebar.selectedSidebarPanel = sidebarPanelOrIdentifierOrIndex; } Style: this logic is non-simple (e.g. returning a property with the same name as the function), so please make this multiple lines > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:111 > + get allowCollapsing() { return super.allowCollapsing; } ditto (:54) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:162 > + return this._allowMultipleSidebars && this._multipleSidebarsVisible && this._hasSidebarPanelSupportingExclusive && this.sidebarPanels.length >= 2 && WI.settings.experimentalEnableMultipleSidebarsInElementsTab.value NIT: the setting check should probably go first, especially since it's off by default > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:168 > + for (let sidebarPanel of this.sidebarPanels) ditto (Main.js:1344) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:183 > + for (let sidebarPanel of this.sidebarPanels) ditto (Main.js:1344) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:187 > + for (let sidebarPanel of this.sidebarPanels) > + if (sidebarPanel.allowExclusivePresentation) > + return true; > + > + return false; you can simplify this ``` return this.sidebarPanels.some((sidebarPanel) => sidebarPanel.allowExclusivePresentation); ``` > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:215 > + let sidebar = new WI.SingleSidebar(null, this.side, null, sidebarPanel.navigationItem.label, false); Style: you can drop trailing arguments that are falsy > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:217 > + sidebarPanel.isUsingExclusivePresentation = true; NIT: This appears out-of-place in the middle of all these `sidebar.` calls. I'd move it above. > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:240 > + if (this.multipleSidebarsVisible) ditto (Main.js:1344) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:241 > + for (var i = index - 1; i >= 0; i--) ditto (Main.js:1344) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:248 > + _findSidebarForSidebarPanel(sidebarPanel) Would `sidebarPanel.parentSidebar` not work? > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:250 > + for (let sidebar of this._sidebars) ditto (Main.js:1344) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:285 > + this.dispatchEventToListeners(WI.Sidebar.Event.WidthDidChange, {sidebar: event.target, newWidth: event.data?.newWidth}); you can assume that `event.data` is set since we control who dispatches the event (and they all currently provide `data`) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:296 > +WI.MultiSidebar.ShowingMultipleStyleClassName = "showing-multiple" We don't follow this pattern anymore. You should just inline this. > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:299 > + MultipleSidebarsVisibleDidChange: "multi-sidebar-multiple-sidebars-visible-did-change", How about just `MultipleSidebarsVisibleChanged`? I try to only use `Did` when it's "paired" with a similar `Will` event (e.g. `WillChange` and `DidChange`). > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:301 > +} Style: missing semicolon > Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsSidebarPanel.js:32 > + please add a `// Public` comment > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:398 > + experimentalSettingsView.addSetting(WI.UIString("Elements Tab:", "Elements Tab: @ Experimental Settings", "Category label for experimental settings pertaining to the Elements tab"), WI.settings.experimentalEnableMultipleSidebarsInElementsTab, WI.UIString("Enable multiple sidebars")); Ditto (Setting.js:225) NIT: we always capitalize tab when used with the name of the tab (e.g. "Elements Tab") > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:28 > + constructor(element, identifier, side, role, label) `indentifier` really should be something unique, but every instance of `WI.SingleSidebar` passes the same `"single-sidebar"` value. I'd suggest dropping this parameter and instead have the subclasses add their own class themselves. NIT: we should just drop the `role` parameter since it's always `null` > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:30 > super(element); We should add a check to make sure this class is not used to directly instantiate something (i.e. base class). ``` // This class should not be instantiated directly. Create a concrete subclass instead. console.assert(this.constructor !== WI.Sidebar, this); ``` > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:33 > + console.assert(!side || side === WI.Sidebar.Sides.Leading || side === WI.Sidebar.Sides.Trailing); > + this._side = side || WI.Sidebar.Sides.Leading; I think we should change this slightly so that we always expect a `side`. ``` console.assert(Object.values(WI.Sidebar.Sides).includes(side), side); this._side = side; ``` > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:35 > + this._allowCollapsing = true; `_collapsable`? `_canCollapse`? > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:63 > + if (!this.willInsertSidebarPanel(sidebarPanel, index)) NIT: this really should be called `shouldInsertSidebarPanel` > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:69 > + this.didInsertSidebarPanel(sidebarPanel, index); One alternative to this approach is to have `insertSidebarPanel` return `true`/`false` and use the result of `super.insertSidebarPanel` to know in subclasses whether or not the work was actually done. EDIT: If you decide to remove `willInsertSidebarPanel` then you won't need the "return `true`/`false`" part. > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:83 > + this.didRemoveSidebarPanel(sidebarPanel); ditto (:69) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:86 > + get selectedSidebarPanel() { return this._selectedSidebarPanel; } ditto (MultiSidebar.js:54) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:91 > + if (!sidebarPanel) > + sidebarPanel = this._findSidebarPanel(0); NIT: I'd inline this > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:105 > + get collapsed() { return this._collapsed; } ditto (MultiSidebar.js:54) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:132 > + get allowCollapsing() { return this._allowCollapsing; } ditto (MultiSidebar.js:54) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:137 > + this._allowCollapsing = allow || false; ditto (MultiSidebar.js:60) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:164 > + willInsertSidebarPanel(sidebarPanel, index) { return true; /* Implemented by subclasses. */ } > + didInsertSidebarPanel(sidebarPanel, index) { /* Implemented by subclasses. */ } > + didRemoveSidebarPanel(sidebarPanel) { /* Implemented by subclasses. */ } > + willSetSelectedSidebarPanel(sidebarPanel) { /* Implemented by subclasses. */ } > + didSetSelectedSidebarPanel(sidebarPanel) { /* Implemented by subclasses. */ } > + willSetCollapsed(flag) { /* Implemented by subclasses. */ } > + didSetCollapsed(flag) { /* Implemented by subclasses. */ } Style: please put each of these onto separate lines. NIT: not all subclasses implement each of these, so it really should be `// Implemented by subclasses if needed.`. > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:202 > +}; please make sure there's a trailing newline :) > Source/WebInspectorUI/UserInterface/Views/SidebarPanel.js:35 > + this._isUsingExclusivePresentation = false; NIT: `_isExclusive (or even just `_exclusive`)? > Source/WebInspectorUI/UserInterface/Views/SidebarPanel.js:96 > + get isUsingExclusivePresentation() { return this._isUsingExclusivePresentation; } ditto (MultiSidebar.js:54) > Source/WebInspectorUI/UserInterface/Views/SidebarPanel.js:102 > + this._isUsingExclusivePresentation = exclusive || false; ditto (MultiSidebar.js:60) > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:52 > + set allowResizingToCollapse(allow) { this._allowResizingToCollapse = allow || false; } ditto (MultiSidebar.js:60) > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:62 > + get width() { return this.element.offsetWidth; } ditto (MultiSidebar.js:54) > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:77 > + return !sidebarPanel.parentSidebar || sidebarPanel.parentSidebar === this; Is this actually necessary? Could you instead just override `insertSidebarPanel` and have the check there? ``` insertSidebarPanel(sidebarPanel, index) { console.assert(!sidebarPanel.parentSidebar || sidebarPanel.parentSidebar === this, sidebarPanel); super.insertSidebarPanel(sidebarPanel, index); } ``` > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:115 > + this._navigationBar.selectedNavigationItem = this.selectedSidebarPanel ? this.selectedSidebarPanel.navigationItem : null; `this.selectedSidebarPanel?.navigationItem ?? null` > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:118 > + willSetCollapsed(flag) Can't this also use `didSetCollapsed`? > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:176 > + this.selectedSidebarPanel = event.target.selectedNavigationItem ? event.target.selectedNavigationItem.identifier : null; ditto (SingleSidebar.js:115) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:40 > +:nth-last-child(1 of .spreadsheet-css-declaration) { > + /* Unset border-bottom of last CSS rule. */ > + border-bottom: unset; > +} Clever! This should probably move to `Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.css` though so that we can better scope this rule. ``` .sidebar > .panel.details.css-style > .content > .rules > :nth-last-child(1 of .spreadsheet-css-declaration) { border-bottom: none; } ``` > Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:311 > + assert(false, "panel selection not allowed for navigation sidebar") I think this was better as it was before. My policy on `console.assert` is that it should be used to catch mistakes during development (or catch unexpected edge cases when dealing with protocol data/state). In our View code, we should never be triggering any assertions (and therefore don't need any other code after the assertion). Also, `assert` doesn't exist AFAIK so this would throw an error. > Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:318 > + tabContentView.detailsSidebarSelectedPanelSetting.value = selectedSidebarPanel ? selectedSidebarPanel.identifier : null; ditto (SingleSidebar.js:115) > Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:367 > + if (!event.data) ditto (MultiSidebar.js:285) > Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:378 > + if (sidebar) { ditto (MultiSidebar.js:285) > Source/WebInspectorUI/UserInterface/Views/TabContentView.js:201 > +WI.TabContentView.SidebarWidthSettingPrimarySidebarIdentifier = "primary-sidebar"; This should probably be on `WI.TabBrowser` given that that's where it's used.
Comment on attachment 411579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411579&action=review >> Source/WebInspectorUI/UserInterface/Views/Main.css:185 >> + min-width: 100px; > > Where did this number come from? Can Web Inspector can even get this small? This number matches that in Main.js:1325 for the minimum width of the content browser (tab-browser) - which in the Elements tab is only the DOM tree. >> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:36 >> + this.element.classList.add(WI.MultiSidebar.ShowingMultipleStyleClassName); > > Shouldn't this only be set inside `set multipleSidebarsVisible`? Is it considered better practice to set `this.multipleSidebarsVisible` in the constructor instead of setting the private `_multipleSidebarsVisible` property? >> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:248 >> + _findSidebarForSidebarPanel(sidebarPanel) > > Would `sidebarPanel.parentSidebar` not work? 🤦🏻♂️ Yup, that would work... >> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:285 >> + this.dispatchEventToListeners(WI.Sidebar.Event.WidthDidChange, {sidebar: event.target, newWidth: event.data?.newWidth}); > > you can assume that `event.data` is set since we control who dispatches the event (and they all currently provide `data`) Sidebar.js:129 doesn't provide data, but we dispatch the event because visually the width has changed. Main.js:282/285 listens for this so it can update the tabBrowser which needs to update its layout for the new width. Looking at it though, it might make more sense for Main.js to also listen for the collapse event and call that same handler, instead of having the WidthDidChangeEvent handle both width changes and collapsing in some places. >> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:69 >> + this.didInsertSidebarPanel(sidebarPanel, index); > > One alternative to this approach is to have `insertSidebarPanel` return `true`/`false` and use the result of `super.insertSidebarPanel` to know in subclasses whether or not the work was actually done. > > EDIT: If you decide to remove `willInsertSidebarPanel` then you won't need the "return `true`/`false`" part. I think the answer here is to return to just subclassing this instead of wrapping both ends with override protected methods. >> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:83 >> + this.didRemoveSidebarPanel(sidebarPanel); > > ditto (:69) I think we would need to return the removed sidebar panel instead of just `true`/`false` because MultiSidebar needs to know which panel was actually manipulated (since we accept a sidebarPanel, identifier, or index).
Comment on attachment 411579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411579&action=review >>> Source/WebInspectorUI/UserInterface/Views/Main.css:185 >>> + min-width: 100px; >> >> Where did this number come from? Can Web Inspector can even get this small? > > This number matches that in Main.js:1325 for the minimum width of the content browser (tab-browser) - which in the Elements tab is only the DOM tree. ah gotcha. in that case please add a `keep in sync` comment in both places so that we know of this relationship in each spot :) >>> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:36 >>> + this.element.classList.add(WI.MultiSidebar.ShowingMultipleStyleClassName); >> >> Shouldn't this only be set inside `set multipleSidebarsVisible`? > > Is it considered better practice to set `this.multipleSidebarsVisible` in the constructor instead of setting the private `_multipleSidebarsVisible` property? I realize I phrased my question badly. What I really meant to ask was "Why does `_multipleSidebarsVisible` default to `true`? Shouldn't it default to `false` and then switch to `true` as new sidebar panels are added (assuming there is space and the setting is enabled)? The naming of this implies that it will only be true when multiple sidebars are in fact visible, so defaulting to `true` when the aren't even any sidebars yet seems odd.". Usually we invoke the `set` in the `constructor` so that we don't have to duplicate any logic (especially if there is any CSS involved). >>> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:285 >>> + this.dispatchEventToListeners(WI.Sidebar.Event.WidthDidChange, {sidebar: event.target, newWidth: event.data?.newWidth}); >> >> you can assume that `event.data` is set since we control who dispatches the event (and they all currently provide `data`) > > Sidebar.js:129 doesn't provide data, but we dispatch the event because visually the width has changed. Main.js:282/285 listens for this so it can update the tabBrowser which needs to update its layout for the new width. Looking at it though, it might make more sense for Main.js to also listen for the collapse event and call that same handler, instead of having the WidthDidChangeEvent handle both width changes and collapsing in some places. Oh interesting! Very weird. Yeah dispatching `WI.Sidebar.Event.WidthDidChange` when toggling the collapsed state doesn't make much sense. Your fix sounds like a good idea :)
Comment on attachment 411579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411579&action=review >>> Source/WebInspectorUI/UserInterface/Views/Sidebar.js:69 >>> + this.didInsertSidebarPanel(sidebarPanel, index); >> >> One alternative to this approach is to have `insertSidebarPanel` return `true`/`false` and use the result of `super.insertSidebarPanel` to know in subclasses whether or not the work was actually done. >> >> EDIT: If you decide to remove `willInsertSidebarPanel` then you won't need the "return `true`/`false`" part. > > I think the answer here is to return to just subclassing this instead of wrapping both ends with override protected methods. Thinking about both this and :83, I think it is actually more straightforward from a reading the subclass point of view to not have to manage super and its result., particularly in light of :89 where because we resolve the sidebarPanelOrIdentifierOrIndex to an actual sidebarPanel, it seems like we are asking for trouble if someone (including me) has to revisit this code after more than a few weeks.
Created attachment 411640 [details] Patch
Comment on attachment 411640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411640&action=review r=me, awesome work! I have a few more style fixes. As a general piece of advice, consider installing <https://eslint.org> and hooking it up to your editor, as that should (hopefully) catch many issues before review (note that we don't exactly follow eslint, such as including extra parenthesis/escapes for clarity). You may have to slightly change the config to get proper linting for new language features tho (i think 2018 -> 2020 for optional chaining or logical assignment). > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.css:33 > + flex-direction: row-reverse; clever 😏 > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:52 > + get primarySidebar() { Style: `{` on separate line > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:56 > + get allowMultipleSidebars() { ditto (:52) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:70 > + get multipleSidebarsVisible() { ditto (:52) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:116 > + get selectedSidebarPanel() { ditto (:52) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:120 > + set selectedSidebarPanel(sidebarPanelOrIdentifierOrIndex) { ditto (:52) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:124 > + get collapsable() { ditto (:52) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:144 > + return this.element.offsetWidth; unnecessary trailing space > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:258 > + for (var i = index - 1; i >= 0; i--) { `let` > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:273 > + for (let sidebar of this._sidebars) Style: we only omit the `{` `}` when the body of the control flow is a single line > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:277 > + for (let sidebar of this._sidebars) > + if (sidebar.sidebarPanels.includes(sidebarPanel)) > + return sidebar; > + > + return null; I think you can simplify this ``` return this._sidebars.find((sidebar) => sidebar.sidebarPanels.includes(sidebarPanel)) || null; ``` > Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsSidebarPanel.js:35 > + get allowExclusivePresentation() { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:398 > + experimentalSettingsView.addSetting(WI.UIString("Elements Tab:", "Elements Tab: @ Experimental Settings", "Category label for experimental settings pertaining to the Elements tab"), WI.settings.experimentalEnableExclusiveDetailsPanels, WI.UIString("Show independent Styles sidebar")); NIT: We should have the name of the `WI.ExperimentalSetting` match the name of the toggle. How about `experimentalEnableIndepdendentStylesPanel`? Generally speaking it's a good idea to have the names of things in code match the name of things in the UI (I'd also consider changing `WI.SidebarPanel.prototype.get exclusive` to `get independent` as well, but that's up to you). > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:90 > + get selectedSidebarPanel() { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:112 > + get collapsed() { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:139 > + get collapsable() { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:143 > + set collapsable(allow) { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:168 > + shouldInsertSidebarPanel(sidebarPanel, index) { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:169 > + /* Implemented by subclasses if needed. */ `//` > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:173 > + didInsertSidebarPanel(sidebarPanel, index) { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:174 > + /* Implemented by subclasses if needed. */ ditto (:169) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:177 > + didRemoveSidebarPanel(sidebarPanel) { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:178 > + /* Implemented by subclasses if needed. */ ditto (:169) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:181 > + willSetSelectedSidebarPanel(sidebarPanel) { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:182 > + /* Implemented by subclasses if needed. */ ditto (:169) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:185 > + didSetSelectedSidebarPanel(sidebarPanel) { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:186 > + /* Implemented by subclasses if needed. */ ditto (:169) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:189 > + didSetCollapsed(flag) { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:190 > + /* Implemented by subclasses if needed. */ ditto (:169) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:197 > + var sidebarPanel = null; `let` > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:205 > + for (var i = 0; i < this._sidebarPanels.length; ++i) { ditto (:197) > Source/WebInspectorUI/UserInterface/Views/Sidebar.js:210 > + for (var i = 0; i < this._sidebarPanels.length; ++i) { > + if (this._sidebarPanels[i].identifier === sidebarPanelOrIdentifierOrIndex) { > + sidebarPanel = this._sidebarPanels[i]; > + break; > + } > + } I think you can simplify this ``` sidebarPanel = this._sidebarPanels.find((existingSidebarPanel) => existingSidebarPanel.identifier === sidebarPanelOrIdentifierOrIndex) || null; ``` > Source/WebInspectorUI/UserInterface/Views/SidebarPanel.js:96 > + get exclusive() { return this._exclusive; } Style: We only inline `get`-`set` pairs when both are inlined. In this case, since the `set` is multiple lines, please also make the `get` multiple lines. > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:63 > + get width() { ditto (MultiSidebar.js:52) > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:92 > + { unnecessary trailing whitespace > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:141 > + var newWidth = positionDelta + this._widthBeforeResize; `let` > Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:319 > + assert(false, "panel selection not allowed for navigation sidebar") `console.assert` Also, I'd personally suggest changing this back to the way it was, since we can assume that `event.target` will never be `this._navigationSidebar` since we only add this event listener to `this._detailsSidebar` :) > Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:323 > + if (tabContentView.managesDetailsSidebarPanels) > + return; we already did this check above so it doesn't need to be repeated > Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:327 > + Style: unnecessary newline
Created attachment 411783 [details] Patch
pangle@apple.com does not have committer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. Rejecting attachment 411783 [details] from commit queue.
pangle@apple.com does not have reviewer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. Rejecting attachment 411783 [details] from commit queue.
Committed r268691: <https://trac.webkit.org/changeset/268691> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411783 [details].