|
|
Created:
5 years, 4 months ago by samli Modified:
5 years, 3 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, yurys+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDevtools UI: Remove redundant tooltips
Tooltips should be used where they add value to the the user. This change removes instances where tooltips are used with the same text as is already displayed.
BUG=513066
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201577
Patch Set 1 #Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 7
Patch Set 4 : #Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Total comments: 6
Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 26 (5 generated)
samli@chromium.org changed reviewers: + dgozman@chromium.org
PTAL. - for Linkifier/ResourceUtils, I wanted to switch them to Tooltips but they are in bindings which is loaded before the UI module (where Tooltip.js is) - NetworkConditionsSelector - I removed that since it appears to be unused (cannot invoke).
paulirish@chromium.org changed reviewers: + paulirish@chromium.org
A little unclear about removing some of these. https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/components/DOMPresentationUtils.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components/DOMPresentationUtils.js:76: parentElement.title = title; can you tell me where this is currently used? https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/components/NetworkConditionsSelector.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components/NetworkConditionsSelector.js:48: option.title = WebInspector.UIString("Maximum download throughput: %s.\r\nMinimum round-trip time: %dms.", throughputText, preset.value.latency); We definitely shouldn't drop this one. It's helped me before. https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/elements/ElementsBreadcrumbs.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/elements/ElementsBreadcrumbs.js:180: crumb.title = crumbTitle; since the breadcrumb list is often "div > div > div > div" you can hover to get a resolved selector which helps you understand which exact div each one is.
https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/components/DOMPresentationUtils.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components/DOMPresentationUtils.js:76: parentElement.title = title; On 2015/07/30 at 07:20:49, paulirish wrote: > can you tell me where this is currently used? Any node description. This is the actual one you see in breadcrumbs. If you see it in breadcrumbs, I think it should only be when it is compacted/collapsed and not on other node descriptions. I think all other instances use it in full. https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/components/NetworkConditionsSelector.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components/NetworkConditionsSelector.js:48: option.title = WebInspector.UIString("Maximum download throughput: %s.\r\nMinimum round-trip time: %dms.", throughputText, preset.value.latency); On 2015/07/30 at 07:20:49, paulirish wrote: > We definitely shouldn't drop this one. It's helped me before. How do I invoke it? I couldn't. https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/elements/ElementsBreadcrumbs.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/elements/ElementsBreadcrumbs.js:180: crumb.title = crumbTitle; On 2015/07/30 at 07:20:49, paulirish wrote: > since the breadcrumb list is often "div > div > div > div" you can hover to get a resolved selector which helps you understand which exact div each one is. DOMPresentationUtils is the one you're looking at. This one is actually for things like "<DOCTYPE>". As you can see on 178, this is always on the screen anyway.
I'm not sure how I'm supposed to review non-obvious feature removing without explanation. Also, how does removing tooltips align with issue "Add descriptive tooltips to the UI"? https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/bindings/Linkifier.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/bindings/Linkifier.js:93: a.title = title || text; This change needs explanation. https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/bindings/ResourceUtils.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/bindings/ResourceUtils.js:181: a.title = url; This one needs explanation too.
> Also, how does removing tooltips align with issue "Add descriptive tooltips to the UI"? The point is to add /descriptive/ tooltips. This includes removing the ones which are not descriptive at all. In these cases, I've removed instances where the tooltip does not provide any additional information. https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/bindings/Linkifier.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/bindings/Linkifier.js:93: a.title = title || text; On 2015/07/30 at 10:31:20, dgozman wrote: > This change needs explanation. Can't find where I was hitting this, sorry. Reverted. https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/bindings/ResourceUtils.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/bindings/ResourceUtils.js:181: a.title = url; On 2015/07/30 at 10:31:20, dgozman wrote: > This one needs explanation too. Updated. If the url is already displayed on the page, don't show an additional tooltip. Also the condition (typeof tooltip !== "string") is unused. This should also be Tooltip.install but it is in bindings/
On 2015/08/03 01:16:26, samli wrote: > > Also, how does removing tooltips align with issue "Add descriptive tooltips to > the UI"? > > The point is to add /descriptive/ tooltips. This includes removing the ones > which are not descriptive at all. In these cases, I've removed instances where > the tooltip does not provide any additional information. I see. I will have to apply and examine case-by-case then. Unfortunately, I'm on the move right now, so this will be slow. > This should also be Tooltip.install but it is in bindings/ I'll think of it. Actually, there was a similar problem a while ago, and we talked about moving Linkifier out from bindings. Resolved without moving though. I'll be back on this.
Can you attach screenshots for the DOMPresentation & ElementsBreadcrumbs titles you've removed? https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/components/NetworkConditionsSelector.js (left): https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... Source/devtools/front_end/components/NetworkConditionsSelector.js:48: option.title = WebInspector.UIString("Maximum download throughput: %s.\r\nMinimum round-trip time: %dms.", throughputText, preset.value.latency); http://i.imgur.com/zCPwEXx.png "RTT" is something i want this extra tooltip for. https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/elements/ComputedStyleWidget.js (right): https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... Source/devtools/front_end/elements/ComputedStyleWidget.js:57: toolbar.appendToolbarItem(new WebInspector.ToolbarCheckbox(WebInspector.UIString("Show inherited"), undefined, this._showInheritedComputedStylePropertiesSetting)); Would rather improve the tooltip than remove it. "Inherited properties propagate the computed value from a parent element to its children." That could be one approach. Technically this is a toggle for "Show all" as the list of inherited props is quite small. http://stackoverflow.com/questions/5612302/which-css-properties-are-inherited https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/elements/StylesSidebarPane.js (left): https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... Source/devtools/front_end/elements/StylesSidebarPane.js:2285: this.nameElement.title = this.property.propertyText; If someone has forced colors to render as HSL, the wont be able to see the original text. But that appears to be the only issue. I'm fine with nuking this. https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/ui/treeoutline.js (left): https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... Source/devtools/front_end/ui/treeoutline.js:316: this.tooltip = title; Appears to be useful for truncated strings like data uris http://i.imgur.com/aOd4vGd.png I'd be fine with removing if we knew the displayed text was identical, but otherwise it seems useful to keep for these cases
https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/components/NetworkConditionsSelector.js (left): https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... Source/devtools/front_end/components/NetworkConditionsSelector.js:48: option.title = WebInspector.UIString("Maximum download throughput: %s.\r\nMinimum round-trip time: %dms.", throughputText, preset.value.latency); On 2015/08/03 at 16:26:20, paulirish wrote: > http://i.imgur.com/zCPwEXx.png > "RTT" is something i want this extra tooltip for. This must be platform specific, I can't see that on Linux. Reverted for now and not using a tooltip since I don't know how tooltips plays with context menus. https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/elements/ComputedStyleWidget.js (right): https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... Source/devtools/front_end/elements/ComputedStyleWidget.js:57: toolbar.appendToolbarItem(new WebInspector.ToolbarCheckbox(WebInspector.UIString("Show inherited"), undefined, this._showInheritedComputedStylePropertiesSetting)); On 2015/08/03 at 16:26:20, paulirish wrote: > Would rather improve the tooltip than remove it. > > "Inherited properties propagate the computed value from a parent element to its children." > > That could be one approach. > > Technically this is a toggle for "Show all" as the list of inherited props is quite small. > http://stackoverflow.com/questions/5612302/which-css-properties-are-inherited Changed the checkbox to say "Show all", since I think this is very clear. https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/ui/treeoutline.js (left): https://codereview.chromium.org/1262063003/diff/40001/Source/devtools/front_e... Source/devtools/front_end/ui/treeoutline.js:316: this.tooltip = title; On 2015/08/03 at 16:26:20, paulirish wrote: > Appears to be useful for truncated strings like data uris > > http://i.imgur.com/aOd4vGd.png > > I'd be fine with removing if we knew the displayed text was identical, but otherwise it seems useful to keep for these cases This is not that and still works. The only instances I could find where this was used was where the string was either empty or: CPU PROFILES HEAP SNAPSHOTS HEAP TIMELINES RESULTS This is actually where the empty tooltip bug is coming from.
PTAL. Only a few left now. https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/components/DOMPresentationUtils.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components/DOMPresentationUtils.js:76: parentElement.title = title; On 2015/07/30 at 08:02:33, samli wrote: > On 2015/07/30 at 07:20:49, paulirish wrote: > > can you tell me where this is currently used? > > Any node description. This is the actual one you see in breadcrumbs. > > If you see it in breadcrumbs, I think it should only be when it is compacted/collapsed and not on other node descriptions. I think all other instances use it in full. Reverted. https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/elements/ElementsBreadcrumbs.js (left): https://codereview.chromium.org/1262063003/diff/20001/Source/devtools/front_e... Source/devtools/front_end/elements/ElementsBreadcrumbs.js:180: crumb.title = crumbTitle; On 2015/07/30 at 08:02:33, samli wrote: > On 2015/07/30 at 07:20:49, paulirish wrote: > > since the breadcrumb list is often "div > div > div > div" you can hover to get a resolved selector which helps you understand which exact div each one is. > > DOMPresentationUtils is the one you're looking at. > This one is actually for things like "<DOCTYPE>". As you can see on 178, this is always on the screen anyway. Reverted.
> This is not that and still works. The only instances I could find where this was used was where the string was either empty or: > CPU PROFILES > HEAP SNAPSHOTS > HEAP TIMELINES > RESULTS > > This is actually where the empty tooltip bug is coming from. Ok. removing that wfm. plz handle empty strings in tooltip.js lgtm
ping dgozman
https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/bindings/ResourceUtils.js (right): https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... Source/devtools/front_end/bindings/ResourceUtils.js:182: else if (tooltipText && tooltipText.length) Just |if (tooltipText)| since empty string evaluates to false. https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/elements/StylesSidebarPane.js (left): https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... Source/devtools/front_end/elements/StylesSidebarPane.js:2285: this.nameElement.title = this.property.propertyText; Why remove this one? It does no harm. If it plays bad with new tooltips - that's a good sign that we should improve something so all possible tooltips are pleasant to use. https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/ui/treeoutline.js (left): https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... Source/devtools/front_end/ui/treeoutline.js:316: this.tooltip = title; Although this is a rare case, I was able to see this tooltip, for example in local modifications view. It is fine there. Do you have examples where this looks bad to weight removing vs leaving?
https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/bindings/ResourceUtils.js (right): https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... Source/devtools/front_end/bindings/ResourceUtils.js:182: else if (tooltipText && tooltipText.length) On 2015/08/12 at 17:19:34, dgozman wrote: > Just |if (tooltipText)| since empty string evaluates to false. Done. https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/elements/StylesSidebarPane.js (left): https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... Source/devtools/front_end/elements/StylesSidebarPane.js:2285: this.nameElement.title = this.property.propertyText; On 2015/08/12 at 17:19:34, dgozman wrote: > Why remove this one? It does no harm. If it plays bad with new tooltips - that's a good sign that we should improve something so all possible tooltips are pleasant to use. Again there is no added value here and is more likely to get in your way. This abuses the tooltip UI construct. We want tooltips to always be useful so users will learn to discover detail there. If its always used for echoing content already there, this effort would be in vain. https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/ui/treeoutline.js (left): https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... Source/devtools/front_end/ui/treeoutline.js:316: this.tooltip = title; On 2015/08/12 at 17:19:34, dgozman wrote: > Although this is a rare case, I was able to see this tooltip, for example in local modifications view. It is fine there. > Do you have examples where this looks bad to weight removing vs leaving? Even in local modifications, it is just showing what is already there. This adds zero value and devalues the overall usefulness of tooltips.
https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/elements/StylesSidebarPane.js (left): https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... Source/devtools/front_end/elements/StylesSidebarPane.js:2285: this.nameElement.title = this.property.propertyText; On 2015/08/17 at 00:53:26, samli wrote: > On 2015/08/12 at 17:19:34, dgozman wrote: > > Why remove this one? It does no harm. If it plays bad with new tooltips - that's a good sign that we should improve something so all possible tooltips are pleasant to use. > > Again there is no added value here and is more likely to get in your way. This abuses the tooltip UI construct. We want tooltips to always be useful so users will learn to discover detail there. If its always used for echoing content already there, this effort would be in vain. http://i.imgur.com/jz4xg8u.png https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/ui/treeoutline.js (left): https://codereview.chromium.org/1262063003/diff/80001/Source/devtools/front_e... Source/devtools/front_end/ui/treeoutline.js:316: this.tooltip = title; On 2015/08/17 at 00:53:26, samli wrote: > On 2015/08/12 at 17:19:34, dgozman wrote: > > Although this is a rare case, I was able to see this tooltip, for example in local modifications view. It is fine there. > > Do you have examples where this looks bad to weight removing vs leaving? > > Even in local modifications, it is just showing what is already there. This adds zero value and devalues the overall usefulness of tooltips. http://i.imgur.com/4w8ymwp.png
samli@chromium.org changed reviewers: + pfeldman@chromium.org
ping +pfeldman
https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... File Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... Source/devtools/front_end/components/Linkifier.js:513: if (!tooltipText && linkText !== url) What about the ones with the overflow? https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... File Source/devtools/front_end/elements/StylesSidebarPane.js (left): https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... Source/devtools/front_end/elements/StylesSidebarPane.js:2283: this.nameElement.title = this.property.propertyText; This one I am fine with. https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... File Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... Source/devtools/front_end/resources/ResourcesPanel.js:1187: WebInspector.Tooltip.install(this._listItemNode, resource.url); Why did this change? https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... File Source/devtools/front_end/ui/treeoutline.js (left): https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... Source/devtools/front_end/ui/treeoutline.js:315: if (typeof title === "string") This I am fine with.
https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... File Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... Source/devtools/front_end/components/Linkifier.js:513: if (!tooltipText && linkText !== url) On 2015/08/19 at 19:08:42, pfeldman wrote: > What about the ones with the overflow? I have not been able to find any instance of this. https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... File Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/1262063003/diff/100001/Source/devtools/front_... Source/devtools/front_end/resources/ResourcesPanel.js:1187: WebInspector.Tooltip.install(this._listItemNode, resource.url); On 2015/08/19 at 19:08:42, pfeldman wrote: > Why did this change? Reverted.
ping
lgtm
The CQ bit was checked by samli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from paulirish@chromium.org Link to the patchset: https://codereview.chromium.org/1262063003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1262063003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1262063003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201577 |