Closed Bug 1077339 Opened 5 years ago Closed 5 years ago

Display keyboard shortcuts when hovering panel tabs

Categories

(DevTools :: Framework, defect, P3)

defect

Tracking

(firefox41 verified)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox41 --- verified

People

(Reporter: rik, Assigned: 15electronicmotor, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=easy][good first bug][lang=javascript][bugday-20150708])

Attachments

(1 file, 3 obsolete files)

When you hover the Console tab, a tooltip says "Web Console". It would be nice if it also said "Cmd-Alt-K". Would be nice to do so for every panel and button (like the Inspect button, Cmd-Alt-C).
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
The fix for this problem should be realtively easy.

The toolbox panels are registered in definitions.js: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/definitions.js

In that file, if you look at the creation of `Tools.inspector`, you can see that both keyboard shortcut and tooltip are defined close to each other. To fix this bug, you need to update the tooltips of all devtools panels to include their respective keyboard shortcut (e.g. instead of "Web Console", the tooltip should say "Web Console (Ctrl+Shift+K)", or on Mac: "Web Console (Cmd+Alt+K)").

I would suggest the following fix:

1. Change the panel tooltip l10n strings to accept a string parameter (e.g. "inspector.tooltip" in https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/inspector.properties#50 should become "DOM and Style Inspector (%S)"). They're all in different ".properties" files under browser/locales/en-US/chrome/browser/devtools.

2. Change the "function l10n" at the bottom of definitions.js to accept an optional "args" parameter (if that parameter is defined, you should use "bundle.formatStringFromName(string, args)" instead of "GetStringFromName").

3. Set each panel's tooltip defined in definitions.js to something like this:

    tooltip: l10n(inspectorStrings, "inspector.tooltip", [(osString == "Darwin" ? "Cmd+Alt" : "Ctrl+Shift") + "+" + key)])

Please ask me any questions you might have by replying to this bug (and please use the needinfo flag if you can). Good luck!
Mentor: janx
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][good first bug][lang=javascript]
One note: the string names will have to change at the same time as the values to help localization tools detect the change.  So instead of:

inspector.tooltip=DOM and Style Inspector

It would become

inspector.tooltip2=DOM and Style Inspector (%S)

Then any associated JavaScript would be updated to point to 'inspector.tooltip2'
I will like to work on this bug.Please assign it to me.
Hi Shivang, thanks for your interest! I'll assign the bug to you. Good luck!
Assignee: nobody → 15electronicmotor
Status: NEW → ASSIGNED
Hi Shivang, please let me know when you get a chance to look at this bug, or if you have any questions / problems. I'm always happy to help :)
Flags: needinfo?(15electronicmotor)
Hi Jan,this is my first bug and I'm figuring out the things.I'll let you know,if I need help.
Thanks.
Flags: needinfo?(15electronicmotor)
(In reply to shivang nagaria from email)
> Can you explain me comment number 3 on page ( https://bugzilla.mozilla.org/show_bug.cgi?id=1077339 ).
> Why should I replace inspector.tooltip by inspector.tooltip2 ? 

The reason we also need to change the string ID ("inspector.tooltip" to "inspector.tooltip2" or to anything else really) is because that string has already been translated into many languages, and if we just change its value, our localizers won't notice that the string named "inspector.tooltip" has changed, and the updated string won't be re-translated. That's the way our localization tools work: if we change the value of a string, we have to make it look like a new string.
I used "get tooltip()" functions because I want to use "this.key".
Attachment #8612891 - Flags: review?(bgrinstead)
Attachment #8612891 - Flags: feedback?
Comment on attachment 8612891 [details] [diff] [review]
Display keyboard shortcuts when hovering panel tabs.

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

Hi, thanks for the patch.  I've left some comments, generally minor formatting things.  Can you update it and then send it back up with the review flag set for janx?

::: browser/devtools/definitions.js
@@ +88,4 @@
>    url: "chrome://browser/content/devtools/inspector/inspector.xul",
>    label: l10n("inspector.label", inspectorStrings),
>    panelLabel: l10n("inspector.panelLabel", inspectorStrings),
> +  get tooltip(){

Whitespace consistency issues I see throughout the file:

Use 2 spaces for indenting
Limit the line length to 80 characters when possible.
And have a space in between each argument passed into functions

And please have a space after the parens:
get tooltip() {

@@ +88,5 @@
>    url: "chrome://browser/content/devtools/inspector/inspector.xul",
>    label: l10n("inspector.label", inspectorStrings),
>    panelLabel: l10n("inspector.panelLabel", inspectorStrings),
> +  get tooltip(){
> +  	return l10n("inspector.tooltip2", inspectorStrings,(osString == "Darwin" ? "Cmd+Alt" : "Ctrl+Shift") + "+" + this.key);

I think moving the last '+' into each part of the ternary makes it easier to read.  Also, 80 chars on the line please.  For example:

  return l10n("inspector.tooltip2", inspectorStrings,
    (osString == "Darwin" ? "Cmd+Alt+" : "Ctrl+Shift+") + this.key);

@@ +124,4 @@
>    label: l10n("ToolboxTabWebconsole.label", webConsoleStrings),
>    menuLabel: l10n("MenuWebconsole.label", webConsoleStrings),
>    panelLabel: l10n("ToolboxWebConsole.panelLabel", webConsoleStrings),
> +  get tooltip(){ 

Nit: trailing whitespace

@@ +185,5 @@
>    url: "chrome://browser/content/devtools/styleeditor.xul",
>    label: l10n("ToolboxStyleEditor.label", styleEditorStrings),
>    panelLabel: l10n("ToolboxStyleEditor.panelLabel", styleEditorStrings),
> +  get tooltip(){
> +	 return  l10n("ToolboxStyleEditor.tooltip3", styleEditorStrings,"Shift" + "+" + (this.key).split('_')[1]);

What's up with (this.key).split('_')[1]?  I see it in a few of these - why can't they be like the others?

@@ +417,3 @@
>  {
>    try {
> +	  if(args !== 'undefined'){

if (args) { is fine

@@ +418,5 @@
>    try {
> +	  if(args !== 'undefined'){
> +		return bundle.formatStringFromName(name,[args],1);
> +	  }
> +	  else{

Please move the else onto the same line as the closing bracket for the if

if {

} else {

}

::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
@@ +320,4 @@
>  variablesViewUninitialized=(uninitialized)
>  variablesViewMissingArgs=(unavailable)
>  
> +anonymousSourcesLabel=Anonymous Sources

This looks like a whitespace change that can be removed?

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +32,4 @@
>  close.accesskey=C
>  update.button=Update
>  update.accesskey=U
> +#change 'k' to 'K'

You can remove this comment, and also please remove the trailing whitespace after the next line
Attachment #8612891 - Flags: review?(bgrinstead)
Attachment #8612891 - Flags: feedback?
Attachment #8612891 - Flags: feedback+
Issue of "(this.key).split('_')[1]":
Properties files of tools like storage,profiler (they use 'shift' key not 'ctrl+shift/cmd+alt') sends shortcut key as "VK_F5",hence I used split function to trap respective function key.
Attachment #8613040 - Flags: review?(janx)
Attachment #8613040 - Flags: feedback?
Hi Shivang! Thanks for the patch, I'll have a look later today or early tomorrow.
Comment on attachment 8612891 [details] [diff] [review]
Display keyboard shortcuts when hovering panel tabs.

(marking older patch as obsolete to avoid confusions)
Attachment #8612891 - Attachment is obsolete: true
Comment on attachment 8613040 [details] [diff] [review]
Display keyboard shortcuts when hovering panel tabs.

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

Your patch works great! Thanks a lot Shivang :)

It's very cool to finally see these tooltips. I can't wait to approve it, but I added a few comments below. Please address them first.

Nit: The patch you uploaded looks like a diff. Usually, we add a bit of formatting:

- We use patches with 8 lines of context instead of 4 (e.g. generate with "diff -u8" or "git show -U8")

- We add a mercurial-like header at the top, e.g.

> # HG changeset patch
> # User Shivang Nagaria <15electronicmotor@gmail.com>
> 
> Bug 1077339: Display keyboard shortcuts when hovering panel tabs. r=janx f=bgrins
>

::: browser/devtools/definitions.js
@@ +89,5 @@
>    label: l10n("inspector.label", inspectorStrings),
>    panelLabel: l10n("inspector.panelLabel", inspectorStrings),
> +  get tooltip() {
> +  	return l10n("inspector.tooltip2", inspectorStrings,
> +		( osString == "Darwin" ? "Cmd+Alt+" : "Ctrl+Shift+" ) + this.key);

Nit: Your editor probably uses tabs when you indent. Please use only spaces, here and below. (For this, you could replace all "\t" characters with "  ").

@@ +126,5 @@
>    menuLabel: l10n("MenuWebconsole.label", webConsoleStrings),
>    panelLabel: l10n("ToolboxWebConsole.panelLabel", webConsoleStrings),
> +  get tooltip() {
> +	  return l10n("ToolboxWebconsole.tooltip2", webConsoleStrings,
> +	 	( osString == "Darwin" ? "Cmd+Alt+" : "Ctrl+Shift+" )  + this.key);

Nit: You have two spaces between ")" and "+", please remove one.

@@ +189,5 @@
>    label: l10n("ToolboxStyleEditor.label", styleEditorStrings),
>    panelLabel: l10n("ToolboxStyleEditor.panelLabel", styleEditorStrings),
> +  get tooltip() {
> +	 return  l10n("ToolboxStyleEditor.tooltip3", styleEditorStrings,
> +		"Shift+" + (this.key).split('_')[1]);

Nit: "split('_')[1]" does look a little weird. Could you please write a function for that at the bottom of the file, near "l10n()", called something like "functionkey()"? Then you could write "functionkey(this.key)" here.
Nit: Also, please you double quotes (") instead of single quotes (') to stay consistent with the rest of the file.

@@ +424,4 @@
>  {
>    try {
> +	  if(args) {
> +		return bundle.formatStringFromName(name,[args],1);

Nit: Since "args" can only be a single argument, not an array, maybe it's better to call it "arg"?
Nit: Also, please use spaces after each comma ("name, [args], 1").

@@ +425,5 @@
>    try {
> +	  if(args) {
> +		return bundle.formatStringFromName(name,[args],1);
> +	  } else {
> +		return bundle.GetStringFromName(name);

Nit: Maybe this would look better in ternary, like "return args ? bundle.formatStringFromName(...) : bundle.GetStringFromName(...)" ?

::: browser/locales/en-US/chrome/browser/devtools/storage.properties
@@ +35,3 @@
>  # This string is displayed in the tooltip of the tab when the storage editor is
>  # displayed inside the developer tools window.
> +storage.tooltip3=Storage Inspector (Cookies, Local Storage …) (%S)

Nit: Since we're changing the string, please also add a "," between "Local Storage" and " …". Thanks!

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +32,4 @@
>  close.accesskey=C
>  update.button=Update
>  update.accesskey=U
> +cmd.commandkey=K

Thanks for fixing this!
Attachment #8613040 - Flags: review?(janx)
Attachment #8613040 - Flags: feedback?
Attachment #8613040 - Flags: feedback+
I made a mistake, the mercurial-like header at the top should look like this:

> # HG changeset patch
> # User Shivang Nagaria <15electronicmotor@gmail.com>
> 
> Bug 1077339 - Display keyboard shortcuts when hovering panel tabs. r=janx f=bgrins
> 

(Using "-" instead of ":" in the "Bug" line).
Shivang, I submitted your patch to "try", our test infrastructure, using the following command (I use git and moz-git-tools):

    $ git push-to-try ../mozilla-central/ -b do -p linux,linux64,macosx64,win32,win64 -u mochitest-dt,mochitest-e10s-devtools-chrome -t none

The results will start showing up here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be3fe1d78d75

Green is good, orange/red means something is broken (but it's not always the patch's fault).
Thanks Jan,I'll fix the patch and resubmit it.
Attachment #8613040 - Attachment is obsolete: true
Attachment #8616489 - Flags: review?(janx)
Attachment #8616489 - Flags: feedback?
Comment on attachment 8616489 [details] [diff] [review]
Display keyboard shortcuts when hovering panel tabs.

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

Thanks a lot Shivang! We're almost there, please fix these remaining 3 small things and I'll validate your patch:

::: browser/devtools/definitions.js
@@ +207,5 @@
>  Tools.shaderEditor = {
>    id: "shadereditor",
>    ordinal: 5,
>    visibilityswitch: "devtools.shadereditor.enabled",
> +  icon: "chrome://browser/skin/devtools/tool-styleeditor.svg",

Why change the ShaderEditor's icon? I don't think you meant to do this.

@@ +424,4 @@
>  {
>    try {
> +	  return arg ? bundle.formatStringFromName(name, [arg], 1) 
> +	  : bundle.GetStringFromName(name);

Nit: This line and above, please replace [tab] with [two spaces] and remove the space after "1)".

@@ +432,5 @@
>  }
> +
> +function functionkey(shortkey)
> +{
> +	return shortkey.split("_")[1];

Nit: Please replace tab with two spaces.
Attachment #8616489 - Flags: review?(janx)
Attachment #8616489 - Flags: feedback?
Attachment #8616489 - Flags: feedback+
Also, as an afterthought, maybe it's a good idea to tell the localizers that a shortcut will be shown in the tooltip?

If you think that's a good idea, please add a comment-line above each modified string, e.g.:

(In browser/locales/en-US/chrome/browser/devtools/debugger.properties) 
 # LOCALIZATION NOTE (ToolboxDebugger.tooltip2):
 # This string is displayed in the tooltip of the tab when the debugger is
 # displayed inside the developer tools window.
+# A keyboard shortcut will be shown inside the brackets.
 ToolboxDebugger.tooltip2=JavaScript Debugger (%S)
Attachment #8616489 - Attachment is obsolete: true
Attachment #8616742 - Flags: review?(janx)
Comment on attachment 8616742 [details] [diff] [review]
Display keyboard shortcuts when hovering panel tabs.

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

Thanks Shivang, looks good to me!

I submitted your new patch to our test infrastructure. The previous run was all green, so I'm confident everything will be green this time as well (if not, problems will probably be unrelated to your patch).

The results will appear here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a525489d294c

Once you're reasonably confident that your patch will work, please add the "checkin-needed" keyword to this bug. Sheriffs look for this keyword a few times per day to land patches into a Firefox integration branch. Once the patch lands into the "mozilla-central" branch and all goes well, the sheriff will mark this bug as "RESOLVED FIXED".

Congratulations on your first patch to Firefox!
Attachment #8616742 - Flags: review?(janx) → review+
Attachment #8616742 - Flags: checkin?
Attachment #8616742 - Flags: checkin?
Patrick: FYI, a checkin? flag would have worked as well, even if unassigned. The sheriffs look for both "checkin-needed" and "checkin?".
Ok, my bad ...
https://hg.mozilla.org/integration/fx-team/rev/fa429d0aec1a
Whiteboard: [devedition-40][difficulty=easy][good first bug][lang=javascript] → [devedition-40][difficulty=easy][good first bug][lang=javascript][fixed-in-fx-team]
Depends on: 1173291
Depends on: 1173373
Blocks: 1173291
No longer depends on: 1173291
(In reply to Jan Keromnes [:janx] from comment #23)
> Patrick: FYI, a checkin? flag would have worked as well, even if unassigned.
> The sheriffs look for both "checkin-needed" and "checkin?".

RyanVM frequently comments saying "use checkin-needed, not checkin?" as in bug 418986 comment 63 and many other bugs.  So, I guess they notice checkin?, but they seem to dislike it.  I believe it requires manual work for them, as it's not managed by their tools.
You're right, "checkin-needed" seems to be the preferred option nowadays. My information about "checkin?" dates from last year, when RyanVM showed my the bug query that sheriffs use to find patches to land (I use it as well to see how long the queue is).
https://hg.mozilla.org/mozilla-central/rev/fa429d0aec1a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][good first bug][lang=javascript][fixed-in-fx-team] → [devedition-40][difficulty=easy][good first bug][lang=javascript]
Target Milestone: --- → Firefox 41
Thank you for this fix, Shivang!

FYI, your patch landed in "mozilla-central", which is the Firefox master branch used to build Nightly every night. About every 6 weeks, a snapshot of "mozilla-central" becomes the new Aurora branch, the one used for Firefox Developer Edition. After another 6 weeks of bug hunting and fixing, Aurora becomes Beta, and another cycle later Beta become stable Firefox, and is sent as an update to all Firefox users around the world.

What this means for your patch:
- It will be in tomorrow's Nightly.
- In about 3 weeks, it will be in Firefox Developer Edition.
- In about 3 months, it will be in stable Firefox.
Reproduced this bug in Firefox nightly (2014-10-03) on Windows 10 64 bit

Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:35.0) Gecko/20100101 Firefox/35.0

The bug is fixed on Latest Aurora 41.0a2 

Build ID: 20150709004007
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0
Reproduced the bug in 35.0a1 (2014-10-03) on Linux x64 with the comment 0's instruction!

Verified as fixed with Latest Aurora 41.0a2 (2015-07-09)

Build ID: 20150709004007
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
QA Whiteboard: [bugday-20150708]
Whiteboard: [devedition-40][difficulty=easy][good first bug][lang=javascript] → [devedition-40][difficulty=easy][good first bug][lang=javascript][bugday-20150708]
As per Comment 31 and Comment 32, The bug is fixed now!
Status: RESOLVED → VERIFIED
Whiteboard: [devedition-40][difficulty=easy][good first bug][lang=javascript][bugday-20150708] → [polish-backlog][difficulty=easy][good first bug][lang=javascript][bugday-20150708]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.