Closed Bug 979479 Opened 6 years ago Closed 6 years ago

Some buttons in Menu Panel still missing keyboard shortcut in tooltips

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox32 --- verified

People

(Reporter: zfang, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P4+] [bugday-20140514])

Attachments

(1 file, 2 obsolete files)

In bug 940116, we forgot New Tab. When it's dragged into the menu panel, the tooltip should be "Open a new tab (⌘T)"
Whiteboard: [Australis:P3][strings]
Also Downloads, Home, and the star portion of the Bookmark/Bookmarks button when in the navbar.
Summary: New Tab button in Menu Panel has inconsistent tooltips → Some buttons in Menu Panel still missing keyboard shortcut in tooltips
Whiteboard: [Australis:P4+]
Also, "Print this page" is missing the keyboard shortcut while sync has no tooltip.
(In reply to Cornel Ionce [QA] from comment #2)
> Also, "Print this page" is missing the keyboard shortcut while sync has no
> tooltip.

On Windows/Linux this opens print preview. There is no shortcut for doing this.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
I was able to get a whole lot extra keyboard shortcuts in the subview panels, but not for the buttons mentioned in comment 0 and comment 1. This is because these buttons are old-style, baked-in XUL widgets that are localized by a DTD source and inserted into the toolbar/ nav-bar as-is.

What I *could* do is change the strings in the DTD to contain `%S` as well, which will be replaced with the keyboard shortcut when placed in an area.

Justin, Jared, what do you think of this?
Flags: needinfo?(jaws)
Flags: needinfo?(dolske)
Attachment #8411758 - Attachment is obsolete: true
Actually, the WIP patch I just posted is a valid patch for bug 947344 :)
Depends on: 947344
From a phone on a train in rural Sweden (PTO, can't easily look up details): we actually do shortcuts for some XUL buttons already, and this should use the same pattern. IIRC we do this for the print and bookmarks buttons, at least.
Ah, thanks for that info :) I see it now! Have fun in Sweden!
Flags: needinfo?(jaws)
Flags: needinfo?(dolske)
Attachment #8411826 - Flags: review?(mconley)
Adding a Keyboard shortcut for the Home button is a bit problematic, since it already sets it dynamically to the the page title or URL of the user-defined home page. It's hard wrt localization to determine the location of the shortcut `(CTRL-BLAH)`; after the tooltip or before it?

Similarly, adding a shortcut bit to the tab-close-button is tricky.
Comment on attachment 8411826 [details] [diff] [review]
Patch v1: add dynamic tooltips to various buttons

Review of attachment 8411826 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is fine - but I'm wary about the tabs-newtab-button class thing. Let's hear from Dao and see if we can just give the thing an ID instead.

::: browser/base/content/browser.js
@@ +4807,5 @@
>  const gDynamicTooltipCache = new Map();
>  function UpdateDynamicShortcutTooltipText(aTooltip) {
>    let nodeId = aTooltip.triggerNode.id;
> +  if (!nodeId && aTooltip.triggerNode.className.length)
> +    nodeId = aTooltip.triggerNode.className.split(" ")[0].trim();

This makes me antsy. I've never felt comfortable that the ID of certain items is stored in the class attribute. That's always felt wrong for things that there are (or should only be) one of.

Do we know about any good reasons why we use a class instead of an ID for tabs-newtab-button? I'll needinfo Dao.

If not, we might want to consider taking this opportunity to give it an ID.
Attachment #8411826 - Flags: review?(mconley) → feedback+
Hey Dao - is there a reason that tabs-newtab-button has its identity strapped to a class, but not an ID? Is having multiple tabs-newtab-button's in the same window a thing we do / want to support?

Are there good reasons for _not_ assigning an ID to tabs-newtab-button?
Flags: needinfo?(dao)
The button is anonymous content and setting an id there probably isn't a good idea, e.g. getElementById might not find it or there might be other surprises.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #13)
> The button is anonymous content and setting an id there probably isn't a
> good idea, e.g. getElementById might not find it or there might be other
> surprises.

Alright, how about a compromise then - can we set an anonid on it, and have mikedeboer's patch check for that instead of the first class? What do you think of that?
Flags: needinfo?(dao)
Sure.
Flags: needinfo?(dao)
Now with anonid.
Attachment #8411826 - Attachment is obsolete: true
Attachment #8415820 - Flags: review?(mconley)
Comment on attachment 8415820 [details] [diff] [review]
Patch v1.1: add dynamic tooltips to various buttons

Review of attachment 8415820 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good - although, according to the interdiff between this and your last diff[1], it looks like bookmarksMenuButton.tooltip got re-added to browser.dtd. Was that on purpose? If not, you should probably keep it removed...

Thanks!

[1]: https://bugzilla.mozilla.org/attachment.cgi?oldid=8411826&action=interdiff&newid=8415820&headers=1
Attachment #8415820 - Flags: review?(mconley) → review+
Thanks Mike!

The `bookmarksMenuButton.tooltip` string was already removed in https://hg.mozilla.org/mozilla-central/rev/1eab0ad18dff (bug 991034) by Dão.

So it not showing up in the latest patch is me resolving a conflict.
Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/4dc06a48ed13
Whiteboard: [Australis:P4+] → [Australis:P4+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4dc06a48ed13
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4+][fixed-in-fx-team] → [Australis:P4+]
Target Milestone: --- → Firefox 32
Blocks: 947344
No longer depends on: 947344
Depends on: 1006490
Hi, 

I was able to reproduce it in Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140305030201 CSet: e5b09585215f

I can confirm this is fixed in:

- latest Nightly Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140514030203 CSet: 2f8af55d6e9a

Still present in the latest Aurora (Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140514004005 CSet: fcbb78e6392e) and in the latest Beta Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140512231802 CSet: 25886b573924.
Whiteboard: [Australis:P4+] → [Australis:P4+] [bugday-20140514]
Argh. I just realized that I shouldn't have probably set the fx30 and fx31 → affected tags. Reverting now, sorry for the noise!

Also, as target milestone was fx32, and I verified on that the fix as per comment 21, I'm setting Verified → Fixed.

Cheers,
Francesca
Status: RESOLVED → VERIFIED
Depends on: 1047643
Depends on: 1062128
You need to log in before you can comment on or make changes to this bug.