Bug 175999 – Web Inspector: Critical content browser toolbar buttons are hidden at narrow widths

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

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.

[Matt Baker]

This patch will also require the addition of a GroupNavigationItem, so that back/forward buttons can be show/hidden as a logical unit.

[Matt Baker]

Created attachment 319104 [details]
[Image] Elements tab - missing sidebar button

[Matt Baker]

Created attachment 319105 [details]
[Image] Debugger tab - missing sidebar button

[Matt Baker]

Created attachment 319108 [details]
Patch

[Matt Baker]

Created attachment 319112 [details]
[Video] Item "collapsing" in action

[Devin Rousso]

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.

[Matt Baker]

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).

[Matt Baker]

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);
}

[Matt Baker]

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;
        });
    }
}

[Matt Baker]

Created attachment 319203 [details]
Patch

[Devin Rousso]

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`.

[Matt Baker]

Created attachment 319315 [details]
Patch

[Matt Baker]

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.

[WebKit Commit Bot]

Comment on attachment 319315 [details]
Patch

Clearing flags on attachment: 319315

Committed r221338: <http://trac.webkit.org/changeset/221338>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34152193>