Last Comment Bug 781466 - Shortcut keys used to assign tags are not consistent with those shown in the Menu.
: Shortcut keys used to assign tags are not consistent with those shown in the ...
Status: RESOLVED FIXED
[gs]
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 22.0
Assigned To: :aceman
:
Mentors:
https://getsatisfaction.com/mozilla_m...
Depends on:
Blocks: tb-tagsmeta
  Show dependency treegraph
 
Reported: 2012-08-09 02:22 PDT by Matt
Modified: 2013-04-01 07:29 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
crude WIP patch (6.04 KB, patch)
2012-12-03 13:35 PST, :aceman
bwinton: ui‑review-
Details | Diff | Review
simpler patch (1.46 KB, patch)
2013-01-04 16:05 PST, :aceman
mkmelin+mozilla: review+
mconley: ui‑review+
Details | Diff | Review

Description Matt 2012-08-09 02:22:36 PDT
Using the shortcut keys Alt + 0 through Alt + 7 does not assign a tag to the mail, nor does the colour change.

Starting in safe mode has no effect and pressing the key combination does not see anything appear in the error console.

about:buildconfig
Build Machine

w64-ix-slave21
Source

Built from http://hg.mozilla.org/mozilla-central/rev/2dfbe4b9403b
Build platform
target
i686-pc-mingw32
Build tools
Compiler 	Version 	Compiler flags
cl 	16.00.30319.01 	-TC -nologo -W3 -Gy -Fdgenerated.pdb -we4553 -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -O1 -Oy
cl 	16.00.30319.01 	-TP -nologo -W3 -Gy -Fdgenerated.pdb -wd4800 -we4553 -GR- -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -O1 -Oy
Comment 1 Jim Porter (:squib) 2012-08-09 10:27:14 PDT
Don't hold the alt key. Just use the number keys by themselves.
Comment 2 Matt 2012-08-11 08:00:00 PDT
I have changed the title of this bug, to reflect what I now know to be the case.

The key strokes appearing in the menu system show underlined numbers, indicating they are menu keystrokes to be accessed with the Alt key.

The reality is that they are short cut keys and as such should appear on the right of the menu. As is the case with all other true short cut key strokes.
Comment 4 :aceman 2012-08-13 05:24:35 PDT
I try this.

https://developer.mozilla.org/en-US/docs/XUL/menuitem
Comment 5 :aceman 2012-08-13 08:52:36 PDT
Using acceltext attribute shows the keys ('0' - '9') aligned to the right. However, to make them still operate when the menu is expanded (is it needed?) the accesskey attribute must still be set and that needs the label to also contain the number (0 - 9). So it is ugly. Can the underlined accesskey character be hidden in some way?
Comment 6 Blake Winton (:bwinton) (:☕️) 2012-11-19 08:03:31 PST
Huh, I really don't know whether we can hide underlined accesskeys…  My guess is "No".  Can you post a patch with what you've got, and I'll give it a try, and play around with it, and point a couple of the Accessibility people at it to see what they think?

Thanks,
Blake.
Comment 7 :aceman 2012-12-03 13:35:20 PST
Created attachment 687935 [details] [diff] [review]
crude WIP patch

So this looks as expected but the code is a monstrosity. What do you think? :)
Comment 8 Magnus Melin 2012-12-09 03:11:25 PST
Comment on attachment 687935 [details] [diff] [review]
crude WIP patch

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

Indeed it's quite ugly code-wise. I wonder if it won't create problems for accessibility aids (which are a prime target group for the feature to begin with).
I think i might go with having it the semi natural way like

 Important (1)  1
 Work (2)       2

(Which you'd get with acceltext and accesskey)

That way at least the accesskey would be underlined, and the key shown where it normally shows.
Comment 9 Blake Winton (:bwinton) (:☕️) 2012-12-10 08:53:12 PST
Comment on attachment 687935 [details] [diff] [review]
crude WIP patch

This removes them from the Mac, which doesn't show shortcut keys, so we're left with "  Important" which both looks odd, and is unhelpful.  ;)  ui-r-.

Thanks,
Blake.
Comment 10 :aceman 2012-12-14 12:24:13 PST
So what about the visually ugly solution in comment 8?
Comment 11 Thomas D. (currently busy elsewhere; needinfo?me) 2012-12-22 07:49:08 PST
If it's really needed, perhaps it will be slightly less ugly if we keep the status quo (with access keys on the *left*) and just add shortcut keys on the right?

_1_ Important     1

That's technically correct, but keeps the numbers more apart from each other so imo it's slightly less painful to the eye than the solution of comment 8.
Comment 12 :aceman 2012-12-22 08:19:35 PST
Yes, that would be easy.
Comment 13 Blake Winton (:bwinton) (:☕️) 2013-01-01 12:24:45 PST
Isn't that basically what we have now?  (I'm at home, so don't have a Windows box to test it on.)

But yeah, I agree with Thomas D in comment 11.
Comment 14 :aceman 2013-01-04 16:05:14 PST
Created attachment 698140 [details] [diff] [review]
simpler patch

Patch implementing comment 11. When there are more tags than 9 I put "  " (2 spaces) as the accesskey so that the tag names align in their column.
Comment 15 Blake Winton (:bwinton) (:☕️) 2013-02-16 11:17:36 PST
(FTR, looks good on Mac, Windows seems odd, since it lists the menu item as "1 Work 1", but I think I could live with that if that's what it used to do…)
Comment 16 WADA 2013-02-16 20:22:57 PST
(In reply to :aceman from comment #5)
> Using acceltext attribute shows the keys ('0' - '9') aligned to the right.
> However, to make them still operate when the menu is expanded (is it
> needed?) the accesskey attribute must still be set and that needs the label
> to also contain the number (0 - 9). So it is ugly. Can the underlined
> accesskey character be hidden in some way?

Keyboard shortcut(==combination of "keyset & key element" and "key attribute") looks independent from "accesskey attribute" for a string in menueitem label.
> https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Keyboard_Shortcuts
>   Keyboard Shortcuts
> http://www.mozilla.org/access/keyboard/
>   Mozilla Keyboard Planning FAQ and Cross Reference
> https://developer.mozilla.org/en-US/docs/XUL/Attribute/accesskey
>   accesskey
> https://developer.mozilla.org/en-US/docs/XUL/Attribute/acceltext
>   acceltext
>   Text that appears beside beside the menu label to indicate
>   the shortcut key (accelerator key) to use to invoke the command.
>   If this value is set, it overrides an assigned key set in the key attribute. 
It looks for me keyset/key/key= is implemented first and accesskey is implemented second.
acceltext may be a member of accesskey feature and may be a replacement of simple keyset/key without modifier. 

<keyset id="Tag_Keyset>
  <key id="numkey_0" key="0">
  <key id="numkey_1" key="1">
                  |       |
  <key id="numkey_9" key="9>
</keyset>
<menupopup>
  <menuitem id="menu_numkey_0" key="numkey_0" label="Clear tag" accesskey="C" />
  <menuitem id="menu_numkey_1" key="numkey_1" label="Important" accesskey="I" />
                            |              |                               |
  <menuitem id="menu_numkey_9" key="numkey_9" label="Tag XYZ"   accesskey="X" />
</menupopup>

Because any string can be assigned to a Tag of Tb, I don't think access key(underscore in menueitem label) is appropriate for adding tag/removing tag via menu. 
F1 to F12 may be a good candidate of shortcut key for tag.
Addtional key like "Alt+NumberKey" may be convenient for some heavy tag users.
Comment 17 WADA 2013-02-16 21:22:52 PST
FYI.
id="appmenu_tagMenu" is defined here.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1912
> 1912             <menu id="appmenu_tagMenu"
> 1915               <menupopup id="appmenu_tagMenu-tagpopup"
> 1916                          onpopupshowing="InitMessageTags(this);">
> Nothing after menuitem for removeTags(=0) is defined here. Addition of menueitems for
> number key 1 to 9 and other tags is done by onpopupshowing event handler.
> Number of items(menuitem,menuseparator) from top till removeTags(=0) is currently 5,
> and the "5" is currently hard coded in tag menu initialization logic.

id="appmenu_tagMenu" is shown/hidden by InitAppMessageMenu() for both Message/Tag of menu and Tag of context menu of a selected mail, so consistency between main menu and context menu is kept always. 
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#463
> 463 function InitAppMessageMenu()
> 501 : final enable/disable Tag menu decision
> 505 : If(winType==mail:3pane)&&!isFeed, openMessageWindowMenuitem.hidden=true
>    i.e. "Open Message in new Window/Tab" is shown only at context menu of mail.
Comment 18 WADA 2013-02-17 02:28:44 PST
FYI.
After(or during) sorting out of shortcut key setting of each <menueitem> for Tag by this bug, adding backend support like "mailnews.tags.<tagname>.shortcutkey = 1 to 9, f1 to f12, alt+1 to 9, ctrl+1 to 9, shift+1 to 9 etc." may be a simple solution for request of "9 is insufficient for usable number of shortcut keys for heavy user of tag" or "custmizable shortcut for tag". UI for it is not mandatory initially, and can be postponed.
Comment 19 Magnus Melin 2013-02-17 04:06:58 PST
Many of the Function keys are already reserved.
Comment 20 WADA 2013-02-17 21:16:28 PST
(In reply to Blake Winton (:bwinton) from comment #15)
> (FTR, looks good on Mac, Windows seems odd, since it lists the menu item as
> "1 Work 1", but I think I could live with that if that's what it used to do…)

If Mac, accesskey in label is not shown with underscore, but Command+accesskey seems to work always. What display result do you call by "good on Mac"?

(a) First "1"(perhaps with underscore on Win) before "Work".
By following code, which is essentially same as original, "1 Work" is set as label of <menuitem>, and because of "accesskey=1", underscore is added to the "1".
> +  let label = document.getElementById("bundle_messenger")
>                        .getFormattedString("mailnews.tags.format",
>                                           [accesskey, name]);
>    menuitem.setAttribute("label", label);

> http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#199
> 208 mailnews.tags.format=%1$S %2$
> http://mxr.mozilla.org/comm-central/source/mozilla/services/common/stringbundle.js#177

(b) Last "1" after "Work"(perhaps adjusted to right most position).
By following code, acceltext="1" is set in the <menuitem>.
> +    menuitem.setAttribute("acceltext", accesskey);
So, perhaps by following mechanism, acceltext is added after label of <menuitem>. 

> http://mxr.mozilla.org/comm-central/source/mozilla/layout/xul/base/src/nsMenuFrame.cpp#124
> 124 NS_IMETHOD Run()
>       if (mAttr == nsGkAtoms::checked) { frame->UpdateMenuSpecialState(frame->PresContext()); }
>       else if (mAttr == nsGkAtoms::acceltext) { ... ;  frame->BuildAcceleratorText(true);     }
>       else if (mAttr == nsGkAtoms::key) { frame->BuildAcceleratorText(true);                  }
>       else if (mAttr == nsGkAtoms::type || mAttr == nsGkAtoms::name) { frame->UpdateMenuType(frame->PresContext()); }
> http://mxr.mozilla.org/comm-central/source/mozilla/layout/xul/base/src/nsMenuFrame.cpp#999
> 999 nsMenuFrame::BuildAcceleratorText(bool aNotify)

If acceltext attribute value is not set, and if key attribute is set, text for keyboard shortcut, including modifier, looks generated from <key> and it looks placed after label.

<key> for 0-9 was already defined by following code(and, as mentioned by Magnus Melin, Fn keys were already used by Tb). It looks that the keyboard shortcut of 0-9 works without menu open at thread pane/message pane, because <key> is definition for window.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#485

What is best for label of <menuitem> when both keyboard shortcut of N and accesskey=N are used?

  <menuitem id=... value=... oncommand=...
            label="combination of name and N from <key>"
            accesskey="N obtained from <key> definition"
            key="key_tagN" or acceltext="N" />

  Current          After proposed patch    Proposal by Magnus Melin
  _1_ Important    _1_ Important    1      Important (_1_)    1
  _3_ Work         _3_ Work         3      Work (_3_)         3
Comment 21 WADA 2013-02-18 01:01:15 PST
(In reply to :aceman from comment #5)
> However, to make them still operate when the menu is expanded (is it needed?)

It looks expansion is needed.
For example, Edit->Select->All
 Alt(=Hot Key) => Focus moves to File => E => Edit is expanded => S => Select is expanded => A => If original focus(or cursor) position is Thread pane, Edit/Select/All is executed.  
In contrast to it, because 0-9 for add tag/remove tag is defined as <key>, keyboard shortcut of 0-9 works without opening/expanding menu/context menu.

> the accesskey attribute must still be set and that needs the label to also contain the number (0 - 9). So it is ugly.
> Can the underlined accesskey character be hidden in some way?

It seems impossible, according to XUL doc.
> https://developer.mozilla.org/en-US/docs/XUL/Attribute/accesskey
> accesskey
>  Type: character
>  This should be set to a character that is used as a shortcut key.
>  This should be one of the characters that appears in the label for the element.
Comment 22 WADA 2013-02-18 01:26:42 PST
(In reply to Matt from comment #2)
> The key strokes appearing in the menu system show underlined numbers,
> indicating they are menu keystrokes to be accessed with the Alt key.
> The reality is that they are short cut keys and as such should appear on the
> right of the menu. As is the case with all other true short cut key strokes.

Because 0-9 has both attribute of <key> and attribute of accesskey, current "underlined number" in menu/context menu is never incorrect.
  Alt => M        => g    => number
        (Message)   (Tag)
  works as expected, if original focus was at thread pane or message pane.
Problem is simply "string what indicates 0-9's attribute of <key> is not shown in menu and context menu".

If following case, it's never ugly.
   <key id="CTRL_V" key="V" modifiers="accel" oncommand="abc()"/>
   <menuitem label="Paste" accesskey="P" key="CTRL_V" oncommand="x()" >
or <menuitem label="Paste" accesskey="P" acceltext="Ctrl+V"    oncommand="x()"/>
  (<menuitem label="Paste" accesskey="P" acceltext="Command+V" oncommand="x()"/>)
  Text in menu (_P_ == P with underline)
   "_P_aste    Ctrl+V"  (Win)   or   "Paste   Command+V" (on Mac)
Comment 23 WADA 2013-02-18 01:33:32 PST
Matt(bug opener), what do you think about best string in menu/context menu?
Comment 24 :aceman 2013-02-18 02:46:38 PST
Window's <key>s seem not accessible from a popup menu. Can we attach an event listener to the menu that if a key from 0-9 is pressed closes the menu and invokes <key> from the top level window?

In that case we could just use acceltext to show the proper key to the right, and no accelkey.
Comment 25 :aceman 2013-02-18 12:35:28 PST
(In reply to WADA from comment #20)
>   Current          After proposed patch    Proposal by Magnus Melin
>   _1_ Important    _1_ Important    1      Important (_1_)    1
>   _3_ Work         _3_ Work         3      Work (_3_)         3

This is a good summary. So which version do we go with?
My proposal from comment 24 also seems inoperable.
Comment 26 WADA 2013-02-18 14:16:15 PST
(In reply to :aceman from comment #24)
> Window's <key>s seem not accessible from a popup menu.

When menu is expanded, conflict on "number key" occurs, although same oncommand is specified for <key key="N"> and <menuitem accesskey="N"> by Thunderbird.
In this case, I think following is applicable.
> http://www.mozilla.org/access/keyboard/#accesskeyconflicts
> If there is a conflict, the web page's accesskey will get the key.
In our case,
  Web page's accelerator == <menuitem accesskey="N"> for menu
  Browser's  accelerator == <key key="N"> for window

Because oncommand is identical in our case, I think <menuitem label="string with N" accesskey="N" key="key_tagN"> is a good way to avoid confusion by who will maintain tag menu related code, if unwanted string by modifiers="shift any" won't appear in menu.
Comment 27 WADA 2013-02-18 17:13:04 PST
(In reply to :aceman from comment #25)
> >   Current          After proposed patch    Proposal by Magnus Melin
> >   _1_ Important    _1_ Important    1      Important (_1_)    1
> >   _3_ Work         _3_ Work         3      Work (_3_)         3

> So which version do we go with?

I prefer Magnus's proposal, because (a) is easier to read/understand than (b) especially when checked. "Checked is 'Work' instead of '_3_' or '_3_ Work'" is visible, and "_3_ is complementary string" can be indicated by "(" and ")".  
(a)   Important (_1_)    1
    x Work (_3_)         3
(b)   _1_ Important      1
    x _3_ Work           3
However, (b) may be better for continuity, because change from current is only "added number at right most position of menu item".
Comment 28 Matt 2013-02-20 12:50:46 PST
(In reply to WADA from comment #23)
> Matt(bug opener), what do you think about best string in menu/context menu?

If the string is going to be underlined, it must work with Alt+ Underlined character.  That is how Windows Menus system works. If there is a short cut then that must appear on the right in the appropriate place.

I have stayed out of these discussions, as I don't really have a strong opinion other than to make the short cut key discoverable with reference to some one who "knows" how they work. I personally prefer the one proposed in the patch.  It might look a little odd in practice but it truly reflects the accelerator keys and offers a reason for the order of the list. The second proposal keeps the numeric accelerator keys, but in reality they no longer serve a meaningful purpose, (it is no longer a numbered list), and should be replaced in favour of alpha numeric accelerators (a pain to do)

To summarise:- I say the the current patch does it for me.
Comment 29 Blake Winton (:bwinton) (:☕️) 2013-02-25 07:24:21 PST
Comment on attachment 698140 [details] [diff] [review]
simpler patch

Ugh.  I've been trying to review this for a while, but my Windows box can't build Thunderbird any more.  :(

So I'm going to redirect the review request to mconley, and see if he can push it forward.

Sorry about that,
Blake.
Comment 30 Mike Conley (:mconley) - (Needinfo me!) 2013-03-29 14:36:27 PDT
Comment on attachment 698140 [details] [diff] [review]
simpler patch

Finally was able to get around to this.

I actually thought this was going to look annoying, what with the repeated numbers. It actually doesn't look bad, confusing, or really repetitive at all. I think the spacing helps with that.

This is also consistent with how we display both shortcut and accesskeys in menus. So this is good by me.
Comment 31 :aceman 2013-03-29 15:49:16 PDT
Comment on attachment 698140 [details] [diff] [review]
simpler patch

Thanks.
Comment 32 Magnus Melin 2013-03-30 12:32:31 PDT
Comment on attachment 698140 [details] [diff] [review]
simpler patch

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

Yeah this looks good, thx! r=mkmelin

::: mail/base/content/mailWindowOverlay.js
@@ +883,5 @@
>    // if a <key> is defined for this tag, use its key as the accesskey
>    // (the key for the tag at index n needs to have the id key_tag<n>)
> +  let shortcutkey = document.getElementById("key_tag" + index);
> +  let accesskey = shortcutkey ? shortcutkey.getAttribute("key") : "  ";
> +  if (accesskey != "  ") {

It looks like it really shows one space to me, so it's not 100% aligned, but i doubt i'd really give it much notice in real use.
Comment 33 :aceman 2013-03-30 14:40:04 PDT
It will surely depend on the font used. Space may have a different width than the '0' - '9' characters.
Comment 34 Ryan VanderMeulen [:RyanVM] 2013-04-01 07:29:08 PDT
https://hg.mozilla.org/comm-central/rev/3b436975ce82

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