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

Don't hold the alt key. Just use the number keys by themselves.

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.

I try this. https://developer.mozilla.org/en-US/docs/XUL/menuitem

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?

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.

So this looks as expected but the code is a monstrosity. What do you think? :)

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 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.

So what about the visually ugly solution in comment 8?

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.

Yes, that would be easy.

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.

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.

(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…)

(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.

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.

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.

Many of the Function keys are already reserved.

(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

(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.

(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)

Matt(bug opener), what do you think about best string in menu/context menu?

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.

(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.

(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.

(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".

(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 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 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 on attachment 698140 [details] [diff] [review] simpler patch Thanks.

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.

It will surely depend on the font used. Space may have a different width than the '0' - '9' characters.