597761 - display keyboard shortcut to access Panorama on toolbar button tooltip

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, defect, P1)

Description

[Aza Raskin [:aza]]

Pretty much as the title says.

Comment 1

[Aza Raskin [:aza]]

In particular, this is to help with discoverability of the keyboard shortcut.

Comment 3

[Aza Raskin [:aza]]

More information can be found in the dup bug: bug 597761

Comment 4

[Raymond Lee [:raymondlee]]

From Bug 597294 comment #1

(In reply to comment #1)
> So I tried doing this, and when modifiers="alt" (for mac) and
> keycode="VK_SPACE" nothing was shown in the view menu, when I changed
> keycode="VK_SPACE" to key="Q" then the keyboard combo was shown. So I wonder if
> Firefox just doesn't display the shortcut in a menuitem when
> keycode="VK_SPACE". We could use the keytext attribute to manually input the
> shortcut shown though..
> 
> To be clear for mac one should use: modifiers="alt" keycode="VK_SPACE"
> and everything else: modifiers="accel" keycode="VK_SPACE"
> correct?
> 

Setting the keytext attribute would just be a workaround. Ian/Aza: do you know who is the best person we are get an answer from?

> Also could someone kindly point me to where the hotkey is currently
> registered/added?

http://hg.mozilla.org/mozilla-central/file/6d01d25a93bf/browser/base/content/browser-tabview.js#l202

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: Test patch

dietrich: could you check out this patch please.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> Created attachment 476743 [details] [diff] [review]
> Test patch
> 
> dietrich: could you check out this patch please.

It looks like this patch should remove the code currently handling Alt/Cmd+Space.

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 476743 [details] [diff] [review] [details]
> > Test patch
> > 
> > dietrich: could you check out this patch please.
> 
> It looks like this patch should remove the code currently handling
> Alt/Cmd+Space.

It's not a complete patch, just uploaded it here for dietrich to check why the key combo doesn't show up on the menu item

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: WIP v1

+#ifdef XP_MACOSX
+# Although this is defined, it doesn't show up on the menu item and key combo is not detected when pressed (Keycode starts with VK_* wouldn't be displayed on the menu item).
+# Therefore, keeping our custom key press event handlers.  However, switching to other key combo, it would work.
+    <key id="key_tabview" keycode="VK_SPACE" command="Browser:ToggleTabView" modifiers="alt"/>
+#else
+# The key combo shows up on the menu item but key combo is not detected when pressed.
+# Therefore, keeping our custom key press event handlers.  However, switching to other key combo, it would work
+    <key id="key_tabview" keycode="VK_SPACE" command="Browser:ToggleTabView" modifiers="accel"/>
+#endif
+

Any suggestions to fix this?

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

WARNING: the shortcut might change, see bug 592183

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

If we do this we have to do it for beta7, so blocking on that, but I don't think I'd hold the release on it so if it gets tough, we'll likely bail.

Comment 11

[Aza Raskin [:aza]]

I'm not sure why this has to be done in beta 7? It seems that while there are string changes, it adds nothing new to be translated.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #11)
> I'm not sure why this has to be done in beta 7? It seems that while there are
> string changes, it adds nothing new to be translated.

Current patch adds a string (to keys.properties) - that needs translated.

Comment 13

[Dietrich Ayala (:dietrich)]

Looks like there's consensus in bug 592183 (also b7 blocker) that the shortcut should change to ctl/cmd+E, which makes the difficult parts of this bug go away.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Now that the shortcut will be accel-e, do we get this for free and can we unblock beta7 on it?

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to comment #14)
> Now that the shortcut will be accel-e, do we get this for free and can we
> unblock beta7 on it?

Yep, the issue was that space was not supported.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

The menu item shows the shortcut automatically after bug 592183. I guess the only thing that's left for this bug is showing it as a tooltip on the toolbar button? Not sure that needs to be a blocker...

(It's a bit annoying to fix this cleanly, because the code that builds up the menu item shortcut text isn't usefully exposed, so we'd need to re-implement a subset of it.)

Comment 17

[Ian Gilman [:iangilman]]

(In reply to comment #16)
> The menu item shows the shortcut automatically after bug 592183. I guess the
> only thing that's left for this bug is showing it as a tooltip on the toolbar
> button? Not sure that needs to be a blocker...
> 
> (It's a bit annoying to fix this cleanly, because the code that builds up the
> menu item shortcut text isn't usefully exposed, so we'd need to re-implement a
> subset of it.)

Do any of the other buttons in the UI show their key combos in their tooltips? If not, we shouldn't do it specially for Panorama.

Comment 18

[Raymond Lee [:raymondlee]]

(In reply to comment #17)
> (In reply to comment #16)
> > The menu item shows the shortcut automatically after bug 592183. I guess the
> > only thing that's left for this bug is showing it as a tooltip on the toolbar
> > button? Not sure that needs to be a blocker...
> > 
> > (It's a bit annoying to fix this cleanly, because the code that builds up the
> > menu item shortcut text isn't usefully exposed, so we'd need to re-implement a
> > subset of it.)
> 
> Do any of the other buttons in the UI show their key combos in their tooltips?
> If not, we shouldn't do it specially for Panorama.

No, there are not other buttons in the UI show their key combos in their tooltips.

Comment 19

[Ian Gilman [:iangilman]]

(In reply to comment #18)
> No, there are not other buttons in the UI show their key combos in their
> tooltips.

Then our work here is done.