Closed
Bug 979479
Opened 10 years ago
Closed 10 years ago
Some buttons in Menu Panel still missing keyboard shortcut in tooltips
Categories
(Firefox :: Toolbars and Customization, defect)
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)
13.76 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
In bug 940116, we forgot New Tab. When it's dragged into the menu panel, the tooltip should be "Open a new tab (⌘T)"
Reporter | ||
Updated•10 years ago
|
Whiteboard: [Australis:P3][strings]
Comment 1•10 years ago
|
||
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+]
Comment 2•10 years ago
|
||
Also, "Print this page" is missing the keyboard shortcut while sync has no tooltip.
Comment 3•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8411758 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Actually, the WIP patch I just posted is a valid patch for bug 947344 :)
Depends on: 947344
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
Ah, thanks for that info :) I see it now! Have fun in Sweden!
Flags: needinfo?(jaws)
Flags: needinfo?(dolske)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8411826 -
Flags: review?(mconley)
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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+
Comment 12•10 years ago
|
||
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)
Comment 13•10 years ago
|
||
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)
Comment 14•10 years ago
|
||
(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)
Assignee | ||
Comment 16•10 years ago
|
||
Now with anonid.
Attachment #8411826 -
Attachment is obsolete: true
Attachment #8415820 -
Flags: review?(mconley)
Comment 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
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.
Assignee | ||
Comment 19•10 years ago
|
||
Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/4dc06a48ed13
Whiteboard: [Australis:P4+] → [Australis:P4+][fixed-in-fx-team]
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4dc06a48ed13
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4+][fixed-in-fx-team] → [Australis:P4+]
Target Milestone: --- → Firefox 32
Updated•10 years ago
|
Comment 21•10 years ago
|
||
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.
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox32:
--- → verified
Whiteboard: [Australis:P4+] → [Australis:P4+] [bugday-20140514]
Comment 22•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•