Tooltips on Tab Bar context menu items are gratuitous

RESOLVED FIXED

Status

SeaMonkey
Tabbed Browser
--
trivial
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Gids Goldberg, Assigned: Ben Goodger (use ben at mozilla dot org for email))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1

The tooltips on the context menu of the tab bar are the same as the text of
their items, for example the item Reload Tab has the tooltip Reload Tab which is
unnecessary. These items would be better off without tooltips unless they add
additional information.

Reproducible: Always
Steps to Reproduce:
1.Create a new tab
2.Right click on the tab bar


Actual Results:  
Gratuitous tooltips appear for each item

Expected Results:  
Either there should be no tooltips or the tooltips should add additional
information.

Updated

14 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

14 years ago
Agree with this, the tooltips for tab menus need to actually describe what each
option will do, instead of just restating what the menu option is.

Here's my rough ideas for what the tooltips _should_ say, but if someone who has
usability-know-how has a better idea of what exactly a tooltip should state then
your help here would be appreciated.

New Tab [Creates a new tab, and sets focus to it *]

Reload Tab [Reloads the selected tab, equivalent to viewing a tab and pressing
the reload button]

Reload All Tabs [Reloads all tabs, equivalent to viewing each tab and pressing
the reload button individually in each *]

Close Other Tabs [Closes all tabs except the one you have selected]

Close Tab [Closes the tab you have selected]


* Appreciate that these (and maybe the others) need some re-wording.
(Reporter)

Comment 2

14 years ago
My personal opinion is that these options would be better off without tooltips,
to be consistent with the rest of the interface. I can't find another example of
a context menu with tooltips in Firefox. Furthermore, the options are quite self
explanatory in my opinion. The descriptions given in Comment 1 would be ideal
for adding to a help document. 
Just my 2p worth.
(Reporter)

Comment 3

14 years ago
For anyone interested in making a patch, the relevant file looks to be
http://lxr.mozilla.org/aviarybranch/source/toolkit/locales/en-US/chrome/global/tabbrowser.dtd

Comment 4

14 years ago
The tooltip is actually a problem in menus.  Once in a while it seems to prevent
my clicks from making it to the button.  It also messes up showing which menu
item is selected.

Comment 5

14 years ago
How about we just come up with better tooltips?

Comment 6

14 years ago
(In reply to comment #4)
> The tooltip is actually a problem in menus.  Once in a while it seems to prevent
> my clicks from making it to the button.  It also messes up showing which menu
> item is selected.

This is separate bug, for sure. I submitted bug 275619 - is this what you
experience?
*** Bug 295821 has been marked as a duplicate of this bug. ***

Comment 8

13 years ago
I'll see what I can do about this.

Bug owner (or someone with the privileges), please change OS to ALL and version
to Trunk.
(Reporter)

Updated

13 years ago
OS: Windows 98 → All
Version: unspecified → Trunk

Comment 9

13 years ago
Created attachment 184853 [details] [diff] [review]
patch-remove tooltips

Remove the tooltips altogether.

If someone is too stupid to realize what "New tab" and "close tab" means, they
shouldn't have their fingers on a keyboard to begin with (or whatever
Attachment #184853 - Flags: review?(mconnor)

Comment 10

13 years ago
Created attachment 184854 [details] [diff] [review]
patch-remove tooltips

Whoops, made a boo-boo in the last one.
Attachment #184853 - Attachment is obsolete: true
Attachment #184854 - Flags: review?(mconnor)

Updated

13 years ago
Attachment #184853 - Flags: review?(mconnor)

Comment 11

13 years ago
Comment on attachment 184854 [details] [diff] [review]
patch-remove tooltips

Not so fast apparently...
Attachment #184854 - Attachment is obsolete: true
Attachment #184854 - Flags: review?(mconnor)

Comment 12

13 years ago
Come to think of it...

Since the tab menu seems to inherit its tooltips from the tab browser bar, but
the tooltip for the menu ands its items cannot be overriden inside their
tags...is there really anyway to do this without restructuring the method used
to display the tab bar?

Comment 13

13 years ago
Created attachment 185881 [details] [diff] [review]
Patch to only show tooltip on tabs

The above patch simply adds a check for document.tooltipNode.localName ==
'tab', which is the condition we need to add for showing the tooltip.


Fuller explanation
------------------
This bit me while developing an extension which added a menubutton to the tab
bar. Not only did menuitems have a gratuitous tooltip, but if I explicitly set
the tooltip or tooltiptext then it showed my tooltip AND this default tooltip
which is just a copy of the label!

The problem is due to the code used to show tooltips on tabs (a useful feature)
in chrome://global/content/bindings/tabbrowser.xml#tabbrowser

Tooltip is set on a container of the tab bar:

<xul:hbox class="tabbrowser-strip chromeclass-toolbar" collapsed="true"
tooltip="_child" context="_child">

And this tooltip appears on any child of the tab bar which has a label:

<xul:tooltip onpopupshowing="event.preventBubble(); if
(document.tooltipNode.hasAttribute('label')) { this.setAttribute('label',
document.tooltipNode.getAttribute('label')); return true; } return false;"/>

To fix this so that only tabs get the tooltip I just added a check to the
onpopupshowing attribute of the tooltip:

<xul:tooltip onpopupshowing="event.preventBubble(); if
(document.tooltipNode.localName == 'tab' &&
document.tooltipNode.hasAttribute('label')) { this.setAttribute('label',
document.tooltipNode.getAttribute('label')); return true; } return false;"/>


Aaron: by removing the tooltip completely you prevent it being shown on tabs
either, where it should be shown (esp. if tab title is wider than tab).
Instead, the tooltip should be shown but only if document.tooltipNode is a tab,
as in the above patch.
Attachment #185881 - Flags: review?(bugs)

Comment 14

13 years ago
I tried applying this patch, but I got the no xbl binding error (and error that
plagued me while trying to do something other than the patches that I cancelled).
I also think that tooltips on context menus is bad UI.
But I think a patch should better just remove the relevant label attributes and
the related content in the dtd file:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#70
http://lxr.mozilla.org/seamonkey/source/toolkit/locales/en-US/chrome/global/tabbrowser.dtd

Comment 16

13 years ago
Created attachment 185932 [details] [diff] [review]
Patch to only show tooltip on tabs (fixed)

Sorry, forgot to encode the &'s in the attributes (hence the no binding error).
This patch fixes that.

Martijn: The labels determine what's shown on the menu. If you removed them,
the menu would be blank and completely unuseable.
Attachment #185881 - Attachment is obsolete: true
Attachment #185932 - Flags: review?

Updated

13 years ago
Attachment #185881 - Flags: review?(bugs)

Comment 17

13 years ago
Patch works (goddammit, &amp; was what I needed to do, because everytime I tried
doing something wiht && >_<....). Who's the review set for?

Comment 18

13 years ago
*** Bug 284203 has been marked as a duplicate of this bug. ***

Comment 19

13 years ago
Comment on attachment 185932 [details] [diff] [review]
Patch to only show tooltip on tabs (fixed)

Asked Michael Connor for review as he's a peer for both the Firefox and XUL
toolkit modules, and was working on the tabs only as few days ago (according to
http://www.mozilla.org/hacking/reviewers.html Ben isn't reviewing patches atm).

One question: should I change this in
mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml as well/instead?
The relevant code seems to be the same...
Attachment #185932 - Flags: review? → review?(mconnor)

Comment 20

13 years ago
Ask mike on irc.

Comment 21

13 years ago
Created attachment 185957 [details] [diff] [review]
Patch to only show tooltip on tabs (fixed)

Noticed that I'd left the old patch in the directory when 'diff'ing, so had a
"? tooltippatch.diff" line at the top which shouldn't have be there...
Attachment #185932 - Attachment is obsolete: true
Attachment #185957 - Flags: review?(mconnor)

Updated

13 years ago
Attachment #185932 - Flags: review?(mconnor)
Sorry for comment 15, that was clearly bogus.
I was thinking about the patch: maybe this check " &amp;&amp;
document.tooltipNode.hasAttribute('label')" can be removed since a tab always
has (or at least should have) a label.

Comment 23

13 years ago
(In reply to comment #22)
> Sorry for comment 15, that was clearly bogus.
> I was thinking about the patch: maybe this check " &amp;&amp;
> document.tooltipNode.hasAttribute('label')" can be removed since a tab always
> has (or at least should have) a label.
> 

It's better to be safe than sorry: we don't want any NullPointerException here.

Comment 24

13 years ago
Created attachment 186925 [details] [diff] [review]
Also patches app suite (xpfe) and removes redundant check for label

I patched the mozilla app suite (xpfe) code too as it's identical.
I also removed the check for a label as per comment #22.

(In reply to comment #23)
> It's better to be safe than sorry: we don't want any NullPointerException
here.
Actually tabs should always have a label as the default tab has one
(http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/tabbrowser.xml#108)
and the addTab method always sets one before adding the tab
(http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/tabbrowser.xml#974).
Even taking extensions into account, there should always be labels on the tabs
or they're pretty useless...
Anyway getAttribute returns an empty string if label isn't set, so at worse
we'd just show a blank tooltip...
Attachment #185957 - Attachment is obsolete: true
Attachment #186925 - Flags: review?(mconnor)

Updated

13 years ago
Attachment #185957 - Flags: review?(mconnor)

Updated

13 years ago
Attachment #186925 - Flags: superreview+

Comment 25

13 years ago
Could someone change product to Core, add the useless-UI keyword and mark bug
284203 as a duplicate of this since my patch now covers the suite too? Also
hardware should probably be All. Thanks. (odd - gerv said he'd given me editbugs
but I never got it...)

Comment 26

13 years ago
->core, ->platform:all
Actually useless-UI isn't quite applicable here.
Component: Tabbed Browser → Tabbed Browser
Flags: review?(mconnor)
Product: Firefox → Core
Hardware: PC → All

Comment 27

13 years ago
Comment on attachment 186925 [details] [diff] [review]
Also patches app suite (xpfe) and removes redundant check for label

Didn't expect changing product to cancel the review request... sorry for
bugspam
Attachment #186925 - Flags: review?(mconnor)

Comment 28

13 years ago
*** Bug 284203 has been marked as a duplicate of this bug. ***

Comment 29

12 years ago
Ben Goodger fixed this as part of bug 308396.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #186925 - Attachment is obsolete: true
Attachment #186925 - Flags: review?(mconnor)
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.