Bug 183099 – Web Inspector: Remove redundant tooltips

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Jon Davis]

Tooltips that are redundant to labels already displayed should not be shown. The top-level tabs, for example, show a tooltip that is redundant to the text label of the tab.

[Jon Davis]

Created attachment 334557 [details]
Patch

[Nikita Vasilyev]

<rdar://problem/35309746>

[Nikita Vasilyev]

Comment on attachment 334557 [details]
Patch

This isn't a complete code review. All my comments are code style nitpicks.

View in context: https://bugs.webkit.org/attachment.cgi?id=334557&action=review

> Source/WebInspectorUI/ChangeLog:36
> +        Use _tooltipHandledSeprately to suppress tooltips when a title is provided.
> +
> +

One empty line break is enough.

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.js:60
> +        else this._tooltipHandledSeparately = true;
>          this._updateStatus();

We never place code on the same line after else. This should be:

    else
        this._tooltipHandledSeparately = true;

    this._updateStatus();

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElementPathComponent.js:94
>  
> +

Unnecessary empty line.

> Source/WebInspectorUI/UserInterface/Views/TimelineTreeElement.js:-64
> -

We keep an empty line for cases like this.

> Source/WebInspectorUI/UserInterface/Views/XHRBreakpointTreeElement.js:44
> +        } else tooltipHandledSeparately = true;

} else
        tooltipHandledSeparately = true;

[Nikita Vasilyev]

Created attachment 334589 [details]
[Screenshot] Tooltip in Elements navigation bar

I noticed tooltips are always shown in the Elements tab navigation bar even when the values are NOT truncated. Is it expected behavior?

[Jon Davis]

I'm still exploring approaches for suppressing tooltips when the DOM tree path elements are fully visible.

[Jon Davis]

Created attachment 334635 [details]
Patch

[Jon Davis]

I updated the patch to address the style nits.

I'm leaving tooltips enabled for the DOM tree hierarchy at the top of the Elements tab as there isn't a direct way to handle enabling/disabling the tooltips based on whether the display name is truncated or not. I'll work on addressing that in a follow-up bug.

[Matt Baker]

Comment on attachment 334635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334635&action=review

> Source/WebInspectorUI/ChangeLog:10
> +        Use _tooltipHandledSeprately to suppress tooltips when a title is provided.

It looks like this only prevents tooltips from showing on global breakpoints. The following cases we still show tooltips, even though they don't add anything not already in the title:

- Line <row:column>
- DOM breakpoints
- XHR breakpoints

> Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.js:61
> +

Subclasses shouldn't access private data in its superclasses. Just use the public setter you added.

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:54
> +            this.tooltip = "";

FYI, the preferred style is `===`, not `==`.

Instead of always setting the tooltip (line 37) and then clearing it later for text-only buttons, it would be cleaner to not set it in the first place:

if (this.buttonStyle !== WI.ButtonNavigationItem.Style.Text)
    this.tooltip = toolTipOrLabel;

For that matter, do we need it for ImageAndText buttons? Maybe we should only show the tooltip when `this.buttonStyle === WI.ButtonNavigationItem.Style.Image`.

> Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:-80
> -        super.title = title;

This works, but shadowing a base class property like this is pretty subtle and might appear like an accidental omission to someone in the future. Since what we really want is to only show tooltips for PinnedTabBarItems, why not just move tooltip logic to that class? I'm not sure that leaves much for the base class TabBarItem with regard to the title, but you could do this:

class TabBarItem
{
    // Public

    get title()
    {
        return this._title;
    }

    set title(title)
    {
        title = title || "";
        if (this._title === title)
            return;
        
        this._title = title;
        this.titleDidChange();
    }

    // Protected

    titleDidChange()
    {
        // Implemented by subclasses.
    }
}

class PinnedTabBarItem extends TabBarItem
{
    titleDidChange()
    {
        this.element.tooltip = this.title;
    }
}

> Source/WebInspectorUI/UserInterface/Views/TimelineTreeElement.js:37
> +        this._tooltipHandledSeparately = true;

Use the public property, and move below `this.editing = ...`.

> Source/WebInspectorUI/UserInterface/Views/XHRBreakpointTreeElement.js:36
> +        let tooltipHandledSeparately;

Not needed. Just set the base class property directly (see below).

> Source/WebInspectorUI/UserInterface/Views/XHRBreakpointTreeElement.js:45
> +            tooltipHandledSeparately = true;

Braces aren't needed around the one-line else:

if (!title) {
    ...
} else
    this.tooltipHandledSeparately = true;

> Source/WebInspectorUI/UserInterface/Views/XHRBreakpointTreeElement.js:51
> +            this._tooltipHandledSeparately = true;

Remove.

[Matt Baker]

Comment on attachment 334635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334635&action=review

> Source/WebInspectorUI/ChangeLog:19
> +        for truncated DOM tree elements in the path.

I think the visibility of HierarchicalPathComponent tooltips should be based on whether the parent NavigationBar is collapsed.

You'll want to take a look in NavigationBar.prototype.layout, which makes two `updateLayout` passes over its child NavigationItems. After the first pass, the collapsed state is known, and then you can:

A) Pass a second bool, `isCollapsed`, to NavigationItem.prototype.updateLayout in the second pass. This would let the item figure out what to do with its tooltip.
B) Remove tooltip handling from NavigationItem altogether. NavigationBar can set tooltips to the item's displayName, or clear them, once the collapsed state is determined.

I kind of like B, because it keeps the tooltip and collapsed-state logic together, and doesn't complicate NavigationItem. I'll leave it up to you.

[Jon Davis]

Created attachment 334772 [details]
Patch [WIP]

[Jon Davis]

Created attachment 335489 [details]
Patch

[EWS Watchlist]

Comment on attachment 335489 [details]
Patch

Attachment 335489 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6888148

New failing tests:
http/tests/preload/download_resources.html

[EWS Watchlist]

Created attachment 335498 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Matt Baker]

Comment on attachment 335489 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 335489 [details]
Patch

Clearing flags on attachment: 335489

Committed r229543: <https://trac.webkit.org/changeset/229543>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.