Summary: | Web Inspector: Critical content browser toolbar buttons are hidden at narrow widths | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 175808 | ||||||||||||||||
Attachments: |
|
Description
Matt Baker
2017-08-25 15:30:13 PDT
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. |