Last Comment Bug 597761 - display keyboard shortcut to access Panorama on toolbar button tooltip
: display keyboard shortcut to access Panorama on toolbar button tooltip
Status: RESOLVED WONTFIX
[b8][ui][good first bug]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: unspecified
: All All
: P1 normal
: Firefox 4.0
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
: 597294 (view as bug list)
Depends on: 592183
Blocks: 597043
  Show dependency treegraph
 
Reported: 2010-09-18 16:19 PDT by Aza Raskin [:aza]
Modified: 2016-04-12 14:00 PDT (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Test patch (2.37 KB, patch)
2010-09-20 02:24 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
WIP v1 (3.28 KB, patch)
2010-09-20 09:02 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Aza Raskin [:aza] 2010-09-18 16:19:39 PDT
Pretty much as the title says.
Comment 1 Aza Raskin [:aza] 2010-09-18 16:20:26 PDT
In particular, this is to help with discoverability of the keyboard shortcut.
Comment 2 Aza Raskin [:aza] 2010-09-18 16:38:50 PDT
*** Bug 597294 has been marked as a duplicate of this bug. ***
Comment 3 Aza Raskin [:aza] 2010-09-18 16:39:11 PDT
More information can be found in the dup bug: bug 597761
Comment 4 Raymond Lee [:raymondlee] 2010-09-19 22:31:50 PDT
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] 2010-09-20 02:24:50 PDT
Created attachment 476743 [details] [diff] [review]
Test patch

dietrich: could you check out this patch please.
Comment 6 Dão Gottwald [:dao] 2010-09-20 03:23:30 PDT
(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] 2010-09-20 03:32:42 PDT
(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] 2010-09-20 09:02:33 PDT
Created attachment 476824 [details] [diff] [review]
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] 2010-09-22 14:33:05 PDT
WARNING: the shortcut might change, see bug 592183
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-22 15:05:30 PDT
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] 2010-09-22 15:15:58 PDT
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) 2010-09-22 17:21:07 PDT
(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) 2010-09-23 00:42:28 PDT
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] 2010-09-27 06:44:15 PDT
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) 2010-09-27 06:59:33 PDT
(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: gavin@gavinsharp.com] 2010-09-27 13:00:04 PDT
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] 2010-09-27 15:37:13 PDT
(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] 2010-09-27 18:38:17 PDT
(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] 2010-09-28 11:24:41 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.