1316855 - Tooltip of "Toolbox Buttons" should show their shortcut

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

[Jens]

The "Toolbox Buttons" do not have their shortcuts in the mouseover tooltip. 

The most often shortcuts I use are ESC ("Toggle split console") and Ctrl+Shift+M ("Responsive Design Mode"). I think I learnt one by accident, the other one from a colleague - after month of usage. 

I think the tooltips of "Toolbox Buttons" should also show their shortcuts, like the tooltips of the Tools (Inspector, Console, etc.) already do. 

For reference of what's available, probably there's even more shortcuts that could be shown in tooltips: https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Seems like a good suggestion to me, thanks for filing!

Comment 2

[Jens]

Wouldn't this make for a "good first bug" so whoever comes around can grab it? Just attaching the Shortcut codes to the tooltips looks simple to me... 

My experience with bugs that are "P3" *and* "enhancement" *and not* listed on http://firefox-dev.tools/ is that these will be buried in the depth of bugzilla and not get fixed for months/years.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Sure, let's give it a try.

To get started with the code base, check out https://wiki.mozilla.org/DevTools/Hacking and https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build.

Here's a search listing the lines that pull in the tooltip for each button:

http://searchfox.org/mozilla-central/search?q=tooltipText%3A&case=true&regexp=false&path=devtools

Here's the file which contains the actual strings themselves:

http://searchfox.org/mozilla-central/source/devtools/shared/locales/en-US/gclicommands.properties

Here's a list of DevTools shortcuts:

https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts

I believe "Split console" and "Responsive Design Mode" may actually be the only two that currently have shortcuts defined.  Shortcuts can differ per platform, so check the list carefully.

I would suggest changing from `l10n.lookup` to `l10n.lookupFormat` and passing in the shortcut text.  The string will need to be changed to accept a value.  When change string values, you need to change the string ID as well[1].

If you have questions, check the "need more info" box on this page and choose "mentor".

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Updating_Entity_Names

Comment 4

[Michael Hoffmann]

Attachment: tooltips.patch

Hi, I made a patch which I'm attaching for feedback.

One shortcut seemed to be embedded in the code in `jsterm.js` while the other one I found in another properties file, `menu.properties`. So I hardcoded them for now, but please let me know if there's a better way to solve it.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8814625 [details] [diff] [review]
tooltips.patch

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

Great, these changes look good to me!

Can you repost your changes as an hg patch?  If you are using git locally, here are some tips[1] to do this.  When you repost, set the new attachment for review with my email and also mark the old one as obsolete.

Format the commit message as shown on that page and use "r=jryans" at the end of the line.

Thanks for looking into this!

[1]: https://developer.mozilla.org/en-US/docs/Tools/Contributing#Creating_a_patch_to_check_in

Comment 6

[Michael Hoffmann]

Attachment: Bug1316855.patch

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8815451 [details] [diff] [review]
Bug1316855.patch

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

Great, this looks good to me.  Thanks for working on it!

I pushed this to our CI system to make sure tests are passing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=982831c7c57e2ff4d31599b723584179d520b578

Assuming that looks good, we can set the "checkin-needed" keyword for a sheriff to land the patch.  The bug will get resolved once it has been merged to mozilla-central.

If you're interested in more DevTools bugs to work on, check out http://firefox-dev.tools for more ideas!

Comment 8

[Michael Hoffmann]

Thank you for the feedback!

I'm happy to be able to help out, I'll definitely go look at http://firefox-dev.tools for more.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Looks ready to land.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d582b93a3785
Tooltip of "Toolbox Buttons" should show their shortcut. r=jryans

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d582b93a3785

Comment 12

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 52.0a1 (2016-11-11) (64-bit) on windows 7 , 64 Bit ! 

This bug's fix is verified with latest Nightly

Build ID          20161210030206
User Agent        Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0 

[bugday-20161207]