Bug 195314 - Web Inspector: CPU Usage Timeline - Give long thread names a tooltip if they could be ellipsized
Summary: Web Inspector: CPU Usage Timeline - Give long thread names a tooltip if they ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks: 194455
  Show dependency treegraph
 
Reported: 2019-03-04 22:16 PST by Joseph Pecoraro
Modified: 2019-03-15 16:48 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (1.42 KB, patch)
2019-03-04 22:17 PST, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (3.67 MB, application/zip)
2019-03-05 01:46 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-03-04 22:16:40 PST
CPU Usage Timeline - Give long thread names a tooltip if they could be ellipsized

Example: The Worker on maps.google.com
Comment 1 Joseph Pecoraro 2019-03-04 22:17:13 PST
Created attachment 363602 [details]
[PATCH] Proposed Fix
Comment 2 Nikita Vasilyev 2019-03-05 00:18:17 PST
Comment on attachment 363602 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:42
> +            if (displayName.length >= 20)
> +                detailsNameElement.title = displayName;

I think this is fine because `.cpu-usage-view > .details` is fixed at 150px. I think it deserves a comment on how you came up with the number 20.

If the width wasn't fixed, you could check if the text is overflowing by `element.scrollWidth > element.offsetWidth`.
Comment 3 EWS Watchlist 2019-03-05 01:46:09 PST
Comment on attachment 363602 [details]
[PATCH] Proposed Fix

Attachment 363602 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11374494

New failing tests:
editing/selection/thai-word-at-document-end.html
Comment 4 EWS Watchlist 2019-03-05 01:46:11 PST
Created attachment 363622 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 5 Devin Rousso 2019-03-05 01:59:15 PST
Comment on attachment 363602 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:41
> +            if (displayName.length >= 20)

I don't like the idea of only adding a tooltip if the name is long enough.  I think we should be consistent and always add it.
Comment 6 Joseph Pecoraro 2019-03-05 10:33:30 PST
(In reply to Devin Rousso from comment #5)
> Comment on attachment 363602 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363602&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:41
> > +            if (displayName.length >= 20)
> 
> I don't like the idea of only adding a tooltip if the name is long enough. 
> I think we should be consistent and always add it.

Adding tooltips all over is a bad idea. Then the user can't rest their mouse anywhere without tooltips showing up all over the place, which is a poor experience.
Comment 7 Matt Baker 2019-03-05 11:17:55 PST
Comment on attachment 363602 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:42
>> +                detailsNameElement.title = displayName;
> 
> I think this is fine because `.cpu-usage-view > .details` is fixed at 150px. I think it deserves a comment on how you came up with the number 20.
> 
> If the width wasn't fixed, you could check if the text is overflowing by `element.scrollWidth > element.offsetWidth`.

I like Nikita's suggestion. It also eliminates issues due to different character widths.
Comment 8 Joseph Pecoraro 2019-03-05 16:18:06 PST
(In reply to Matt Baker from comment #7)
> Comment on attachment 363602 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363602&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:42
> >> +                detailsNameElement.title = displayName;
> > 
> > I think this is fine because `.cpu-usage-view > .details` is fixed at 150px. I think it deserves a comment on how you came up with the number 20.
> > 
> > If the width wasn't fixed, you could check if the text is overflowing by `element.scrollWidth > element.offsetWidth`.
> 
> I like Nikita's suggestion. It also eliminates issues due to different
> character widths.

I believe this would cause multiple forced layouts (depending on the number of sections) in the initial layout, which seems unnecessary.

The number 20 here is a conservative estimate to avoid having too many tooltips on sections that would obviously not have clipped text.
Comment 9 Devin Rousso 2019-03-05 17:19:37 PST
Comment on attachment 363602 [details]
[PATCH] Proposed Fix

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

>>> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:41
>>> +            if (displayName.length >= 20)
>> 
>> I don't like the idea of only adding a tooltip if the name is long enough.  I think we should be consistent and always add it.
> 
> Adding tooltips all over is a bad idea. Then the user can't rest their mouse anywhere without tooltips showing up all over the place, which is a poor experience.

This isn't "all over".  This is in a place where it's not uncommon for data to be clipped.  I would find it more weird as a user to hover something and have it show a tooltip, and then hover something right next to it and not have it show a tooltip.  I'd expect consistency.
Comment 10 Joseph Pecoraro 2019-03-07 10:17:06 PST
(In reply to Devin Rousso from comment #9)
> Comment on attachment 363602 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363602&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:41
> >>> +            if (displayName.length >= 20)
> >> 
> >> I don't like the idea of only adding a tooltip if the name is long enough.  I think we should be consistent and always add it.
> > 
> > Adding tooltips all over is a bad idea. Then the user can't rest their mouse anywhere without tooltips showing up all over the place, which is a poor experience.
> 
> This isn't "all over".  This is in a place where it's not uncommon for data
> to be clipped.  I would find it more weird as a user to hover something and
> have it show a tooltip, and then hover something right next to it and not
> have it show a tooltip.  I'd expect consistency.

If I get a tooltip on text that matches the exact text I'd consider that a bug. Ideally it only happens on ellipsized text.

We do consistently always put tooltips on non-text items, like navigation items, tree elements, and links. But I don't think we do that for just text / titles across the system.

Does anyone have an example on the system of repeating a tooltip for non-ellipsized text?
Comment 11 Devin Rousso 2019-03-08 18:03:52 PST
Comment on attachment 363602 [details]
[PATCH] Proposed Fix

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

>>>>> Source/WebInspectorUI/UserInterface/Views/CPUUsageView.js:41
>>>>> +            if (displayName.length >= 20)
>>>> 
>>>> I don't like the idea of only adding a tooltip if the name is long enough.  I think we should be consistent and always add it.
>>> 
>>> Adding tooltips all over is a bad idea. Then the user can't rest their mouse anywhere without tooltips showing up all over the place, which is a poor experience.
>> 
>> This isn't "all over".  This is in a place where it's not uncommon for data to be clipped.  I would find it more weird as a user to hover something and have it show a tooltip, and then hover something right next to it and not have it show a tooltip.  I'd expect consistency.
> 
> If I get a tooltip on text that matches the exact text I'd consider that a bug. Ideally it only happens on ellipsized text.
> 
> We do consistently always put tooltips on non-text items, like navigation items, tree elements, and links. But I don't think we do that for just text / titles across the system.
> 
> Does anyone have an example on the system of repeating a tooltip for non-ellipsized text?

I don't have an example of the system showing a consistent tooltip, but the system also only shows a tooltip if text is overflowing.  We don't perform that check (we could though by using `sizeDidChange` and some `offsetWidth` comparisons).  We do, however, always show tooltips for all values in a `WI.DataGrid` regardless of whether it is overflowing, because of the fact that we don't want to have to do that logic.  I think we should follow that pattern, because it's worse to not have a tooltip for overflowing text than it is to have a tooltip for non-overflowing text.

With that having been said, if the size of the name element is "fixed", I'm more OK with what you've done, as that's less likely to encounter the situation where we don't show a tooltip for overflowing text (assuming we have the right length).
Comment 12 Devin Rousso 2019-03-15 16:28:57 PDT
Comment on attachment 363602 [details]
[PATCH] Proposed Fix

r=me
Comment 13 Joseph Pecoraro 2019-03-15 16:47:09 PDT
https://trac.webkit.org/changeset/243026/webkit
Comment 14 Radar WebKit Bug Importer 2019-03-15 16:48:15 PDT
<rdar://problem/48944451>