Summary: Critical content browser toolbar buttons are hidden at narrow widths. See screenshots. NavigationBar wraps items that can't find in the available space. It would be better to give items a priority, and remove the least important items first, as space becomes constrained. Proposal: AppKit calls this concept "visibility priority", which I think is good name (see https://developer.apple.com/documentation/appkit/nstouchbaritem). When NavigationBar does a layout: 1. Create list of candidate items in ascending priority order. Do not include dividers, flexible space, or hidden items. 2. Measure items at full width. 3. If available space is exceeded, give each item the opportunity to collapse further. 4. Measure items again. 5. If available space is exceeded, hide lowest priority item and remove from candidate list. Otherwise goto step 7. 6. Goto step 5. 7. Remove redundant dividers.
This patch will also require the addition of a GroupNavigationItem, so that back/forward buttons can be show/hidden as a logical unit.
Created attachment 319104 [details] [Image] Elements tab - missing sidebar button
Created attachment 319105 [details] [Image] Debugger tab - missing sidebar button
Created attachment 319108 [details] Patch
Created attachment 319112 [details] [Video] Item "collapsing" in action
Comment on attachment 319108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319108&action=review > Source/WebInspectorUI/ChangeLog:20 > + Set `High` and `Low` priorities. You could group these with the same message below. > Source/WebInspectorUI/ChangeLog:24 > + Set `High` and `Low` priorities, and group the back/forward buttons. Ditto. > Source/WebInspectorUI/ChangeLog:30 > + Set `High` and `Low` priorities. Ditto. > Source/WebInspectorUI/ChangeLog:47 > + Set `High` and `Low` priorities. Ditto. > Source/WebInspectorUI/ChangeLog:73 > + Let the NavigationItem control the setting of it's parent bar, which > + allows GroupNavigationItems to forward this action to their children. It might be worth adding a comment saying there are three priorities: `Low`, `Normal`, and `High`. As I was reading this I didn't understand why you didn't add a priority to each NavigationItem until I got to that file and saw that it had a default value of `Normal`. > Source/WebInspectorUI/ChangeLog:87 > + Set `High` and `Low` item priorities. Ditto. > Source/WebInspectorUI/UserInterface/Views/GroupNavigationItem.js:62 > + super.didDetach(); I personally prefer putting the super calls to detach()/hidden() style functions at the end of the function just in case the super call does something to the element/object that I don't expect. I don't think we have this issue anywhere, but it's just a thought ¯\_(ツ)_/¯ > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:178 > + function matchingSelfOrChild(item) > + { Style: brace on same line. > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:189 > + if (!(item instanceof WI.GroupNavigationItem)) > + return null; > + > + for (let childItem of item.navigationItems) { > + let result = matchingSelfOrChild(childItem); > + if (result) > + return result; > + } You still need another return value after this loop. I'd also rewrite it for a bit more clarity. function matchingSelfOrChild(item) { if (item.identifier === identifier) return item; if (item instanceof WI.GroupNavigationItem) { for (let childItem of item.navigationItems) { let result = matchingSelfOrChild(childItem); if (result) return result; } } return null; } > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:263 > + totalItemWidth += navigationItem.element.realOffsetWidth; Having this inside the loop could potentially trigger a bunch of unnecessary layouts. I think you might be better off moving it to a reduce() outside the loop. totalItemWidth = this._navigationItems.reduce((total, item) => total + item.element.realOffsetWidth, 0); if (totalItemWidth <= barWidth) return; > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:273 > + totalItemWidth -= navigationItem.element.realOffsetWidth; Is there a reason we can't cache the width of each item? flexibleSpace items are already ignored, and I don't think we have other NavigationItem types that expand/shrink, but I am not 100% sure :/ > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:307 > + _forceItemHidden(item, hidden) > + { > + item[WI.NavigationBar.ForceHiddenSymbol] = hidden; > + item.element.classList.toggle("force-hidden", hidden); > + } > + > + _isItemHidden(item) > + { > + return item.hidden || item[WI.NavigationBar.ForceHiddenSymbol]; > + } > + It seems weird to have these two functions be private when isDivider() and isFlexibleSpace() are nested. I'd either make them all nested or all class-level. > Source/WebInspectorUI/UserInterface/Views/NavigationItem.js:55 > + get visibilityPriority() { return this._visibilityPriority; } > + set visibilityPriority(priority) { this._visibilityPriority = priority; } Style: I think we usually separate get/set pairs from the general grouping of getters. > Source/WebInspectorUI/UserInterface/Views/NavigationItem.js:80 > + // Protected I think these should be Public, as they are called from NavigationBar. Also, I think using just `attach` and `detach` makes a bit more sense, as they aren't really in response to an event.
Comment on attachment 319108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319108&action=review >> Source/WebInspectorUI/UserInterface/Views/GroupNavigationItem.js:62 >> + super.didDetach(); > > I personally prefer putting the super calls to detach()/hidden() style functions at the end of the function just in case the super call does something to the element/object that I don't expect. I don't think we have this issue anywhere, but it's just a thought ¯\_(ツ)_/¯ Whether and when to call `super.myMethod()` from `myMethod` really depends. In general, overridable methods fall into four categories: 1. Construction - Call super first, then execute method body. - Metaphor: create the building floor by floor, starting with the foundation. 2. Destruction - Call super last, after executing method body. - Metaphor: tear down the building floor by floor, starting with the roof. 3. Doesn't Matter - Effect is the same whether super is called first or last. 4. Optional - Can omit call to super completely, to prevent base class behavior. I agree that didDetach()/hidden() most closely resembles #2. I'll move the call to super.didDetach. In general, we should only care about this when required by the semantics of the method, and establish a convention of calling super first in case #3. >> Source/WebInspectorUI/UserInterface/Views/NavigationItem.js:80 >> + // Protected > > I think these should be Public, as they are called from NavigationBar. Also, I think using just `attach` and `detach` makes a bit more sense, as they aren't really in response to an event. Making them public: Sometimes we use a "// Protected" or "// Called by " comment, when we actually intend something closer to `friend` in C++. I'm in favor of stopping this practice and just making them public. attach vs didAttach: To me "did" conveys something about the order in which the method is called, not it's source (event vs. app logic).
Comment on attachment 319108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319108&action=review > Source/WebInspectorUI/UserInterface/Views/GroupNavigationItem.js:50 > + // Protected Also needs to override updateLayout: updateLayout(expandOnly) { super.updateLayout(expandOnly); for (let item of this._navigationItems) item.updateLayout(expandOnly); }
It turns out NavigationBar.prototype.layout can be greatly simplified by adding: class NavigationBar { // Private get _visibleNavigationItems() { return this._navigationItems.filter((item) => { if (item instanceof WI.FlexibleSpaceNavigationItem) return false; if (item.hidden || item[WI.NavigationBar.ForceHiddenSymbol]) return false; return true; }); } }
Created attachment 319203 [details] Patch
Comment on attachment 319203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319203&action=review r=me, with a few minor comments > Source/WebInspectorUI/UserInterface/Views/GroupNavigationItem.js:40 > + this._navigationItems = []; > + > + for (let item of navigationItems) { > + console.assert(item instanceof WI.NavigationItem); > + this.element.appendChild(item.element); > + this._navigationItems.push(item); > + } Instead of adding each item to _navigationItems, why not just set them equal and then loop over _navigationItems? this._navigationItems = navigationItems; for (let item of this._navigationItems) { console.assert(item instanceof WI.NavigationItem); this.element.appendChild(item.element); } > Source/WebInspectorUI/UserInterface/Views/GroupNavigationItem.js:42 > + // Public > Source/WebInspectorUI/UserInterface/Views/GroupNavigationItem.js:48 > + get navigationItems() { return this._navigationItems; } NIT: I'd put this above minimumWidth(), as it's a one-line getter. > Source/WebInspectorUI/UserInterface/Views/GroupNavigationItem.js:52 > + updateLayout(expandOnly) I realize that this function already existed, but it's very confusing because this doesn't subclass View :( > Source/WebInspectorUI/UserInterface/Views/GroupNavigationItem.js:70 > + super.didDetach(); Were you going to move this? > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:433 > + const totalItemWidth = this._visibleNavigationItems.reduce((total, item) => item.minimumWidth, 0); Style: this value changes per invocation, so it should be `let`.
Created attachment 319315 [details] Patch
Comment on attachment 319315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319315&action=review > Source/WebInspectorUI/UserInterface/Views/GroupNavigationItem.js:53 > + updateLayout(expandOnly) I agree this name is confusing. It pre-dates the View class, but should probably be renamed.
Comment on attachment 319315 [details] Patch Clearing flags on attachment: 319315 Committed r221338: <http://trac.webkit.org/changeset/221338>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34152193>