display keyboard shortcut to access Panorama on toolbar button tooltip

RESOLVED WONTFIX

Status

Firefox Graveyard
Panorama
P1
normal
RESOLVED WONTFIX
7 years ago
a year ago

People

(Reporter: aza, Assigned: raymondlee)

Tracking

unspecified
Firefox 4.0
Dependency tree / graph

Details

(Whiteboard: [b8][ui][good first bug])

Attachments

(2 obsolete attachments)

(Reporter)

Description

7 years ago
Pretty much as the title says.
(Reporter)

Comment 1

7 years ago
In particular, this is to help with discoverability of the keyboard shortcut.
blocking2.0: --- → ?
Priority: -- → P1
Whiteboard: [b8][ui]
Target Milestone: --- → Firefox 4.0
(Reporter)

Updated

7 years ago
Blocks: 597043
(Reporter)

Updated

7 years ago
Whiteboard: [b8][ui] → [b8][ui][good first bug]
(Reporter)

Updated

7 years ago
Duplicate of this bug: 597294
(Reporter)

Comment 3

7 years ago
More information can be found in the dup bug: bug 597761
(Assignee)

Comment 4

7 years ago
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
(Assignee)

Comment 5

7 years ago
Created attachment 476743 [details] [diff] [review]
Test patch

dietrich: could you check out this patch please.
(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.
(Assignee)

Comment 7

7 years ago
(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
(Assignee)

Comment 8

7 years ago
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?
Attachment #476743 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #476824 - Attachment is patch: true
Attachment #476824 - Attachment mime type: application/octet-stream → text/plain
WARNING: the shortcut might change, see bug 592183
blocking2.0: ? → beta7+
Depends on: 592183
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.
(Reporter)

Comment 11

7 years ago
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.
(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.
Group: mozilla-corporation-confidential
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.

Updated

7 years ago
Group: mozilla-corporation-confidential

Updated

7 years ago
Assignee: nobody → raymond
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
Status: ASSIGNED → NEW
(Assignee)

Updated

7 years ago
Attachment #476824 - Attachment is obsolete: true
Now that the shortcut will be accel-e, do we get this for free and can we unblock beta7 on it?
(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.
blocking2.0: beta7+ → beta8+
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.)
Summary: Display keyboard shortcut to access Panorama in menu item and tooltip → display keyboard shortcut to access Panorama on toolbar button tooltip
(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.
(Assignee)

Comment 18

7 years ago
(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.
(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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.