Last Comment Bug 650798 - Contacts sidebar: Missing accelerator keys on context menu
: Contacts sidebar: Missing accelerator keys on context menu
Status: VERIFIED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: 11 Branch
: All All
: P1 minor (vote)
: Thunderbird 11.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-18 08:01 PDT by Thomas D. (currently busy elsewhere; needinfo?me)
Modified: 2012-02-23 08:16 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch implementing comment 0 and comment 4 (3.69 KB, patch)
2011-11-12 14:34 PST, :aceman
no flags Details | Diff | Splinter Review
patch implementing comment 0 and comment 4, 2nd version (3.86 KB, patch)
2011-11-13 06:53 PST, :aceman
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
patch updated to comment 17 (5.08 KB, patch)
2011-12-04 07:37 PST, :aceman
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review

Description Thomas D. (currently busy elsewhere; needinfo?me) 2011-04-18 08:01:06 PDT
STR

1 In compose window, view contacts sidebar (F9)
2 Right-click to get context menu of a contact, or multiple selected contacts

Actual
- Context menu entries do not have accelerator keys, nor indications thereof
- only unique initial letters work as accelerator keys (D for delete, P for properties), Add-to... menus do not have a unique accelerator key (could use the same as on the buttons in the sidebar).

Expected
- define accelerator keys, so that they are unique for each entry, and indicated on the menus

D_e_lete
_P_roperties
_A_dd to To
A_d_d to CC
Add to _B_CC

We should not use D as an accelerator for Delete, because of ux-consistency with the buttons, where we have alt+d for "Add to To".
Comment 1 Thomas D. (currently busy elsewhere; needinfo?me) 2011-04-18 08:02:16 PDT
E is a good accelerator for Delete, because it can stand for "Erase"
Comment 2 :aceman 2011-11-12 10:00:59 PST
I could try to patch this.

Bwinton, is this wanted?
Comment 3 Blake Winton (:bwinton) (:☕️) 2011-11-12 12:10:20 PST
Yeah, I'ld like it if you could fix this.  :)

Thanks,
Blake.
Comment 4 Jim Porter (:squib) 2011-11-12 13:30:44 PST
(In reply to Thomas D. from comment #0)
> We should not use D as an accelerator for Delete, because of ux-consistency
> with the buttons, where we have alt+d for "Add to To".

I think you mean "Add to Cc" here, but I also think it would make more sense to use "Add to _C_c" instead. Then there's no consistency issue, and it's easier to remember Alt-C = Cc anyway.
Comment 5 :aceman 2011-11-12 13:53:12 PST
Good idea.
Comment 6 :aceman 2011-11-12 14:34:10 PST
Created attachment 574092 [details] [diff] [review]
patch implementing comment 0 and comment 4

Is it OK to reuse accesskey definitions like this?
Or should I create separate addtoToFieldMenu.accesskey (etc.) entities?
Comment 7 Thomas D. (currently busy elsewhere; needinfo?me) 2011-11-13 03:15:56 PST
(In reply to :aceman from comment #6)
> Created attachment 574092 [details] [diff] [review] [diff] [details] [review]
> patch implementing comment 0 and comment 4
> 
> Is it OK to reuse accesskey definitions like this?
> Or should I create separate addtoToFieldMenu.accesskey (etc.) entities?

aceman, thank you for the patch! I think we need separate entities, because we never know the situation in other localizations, they might need different accesskeys for the menu and the buttons. Furthermore, it will be unexpected and more complicated for addons to change things in this corner without unique accesskeys for each part of the UI. Combining them will just increase the risk of broken or missing access keys in the future.
Comment 8 :aceman 2011-11-13 06:53:57 PST
Created attachment 574154 [details] [diff] [review]
patch implementing comment 0 and comment 4, 2nd version

OK, sounds good.
Comment 9 Blake Winton (:bwinton) (:☕️) 2011-11-15 13:38:31 PST
Comment on attachment 574154 [details] [diff] [review]
patch implementing comment 0 and comment 4, 2nd version

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

Yeah, it would make the most sense to me to have:
_D_elete
_P_roperties
Add to _T_o
Add to _C_C
Add to _B_CC

and make the buttons match these, but we can't, because alt-t is the Tools menu.

So aside from the comments below, I like it.

ui-r=me, and r=me with the rewrapping below done, and the localization question answered.

Thanks,
Blake.

::: mail/locales/en-US/chrome/messenger/addressbook/abContactsPanel.dtd
@@ -13,4 +18,4 @@
> >  <!ENTITY toButton.label                     "Add to To:">
> >  <!ENTITY toButton.accesskey                 "A">
> >  <!ENTITY ccButton.label                     "Add to Cc:">
> > -<!ENTITY ccButton.accesskey                 "d">
> > +<!ENTITY ccButton.accesskey                 "C">

Ooh, do we need to change this for localizers?

::: mail/components/addrbook/content/abContactsPanel.xul
@@ -70,3 +70,3 @@
> >    <menupopup id="cardProperties">
> > -    <menuitem label="&deleteAddrBookCard.label;" oncommand="AbDelete();"/>
> > +    <menuitem label="&deleteAddrBookCard.label;" accesskey="&deleteAddrBookCard.accesskey;" oncommand="AbDelete();"/>
> > -    <menuitem label="&addrBookCardProperties.label;" oncommand="AbEditSelectedCard();"/>
> > +    <menuitem label="&addrBookCardProperties.label;" accesskey="&addrBookCardProperties.accesskey;" oncommand="AbEditSelectedCard();"/>

These are really long.  Would you mind splitting them up to be more like this:
    <menuitem label="&deleteAddrBookCard.label;"
              accesskey="&deleteAddrBookCard.accesskey;"
              oncommand="AbDelete();"/>
Comment 10 :aceman 2011-11-15 14:02:36 PST
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #9)
> and make the buttons match these, but we can't, because alt-t is the Tools
> menu.

So wouldn't it be ugly inconsistent? So what is better? Should I change the menu and leave the button alone or leave it as is in the patch?

> > > -<!ENTITY ccButton.accesskey                 "d">
> > > +<!ENTITY ccButton.accesskey                 "C">
> 
> Ooh, do we need to change this for localizers?

Yes, you taught me the identifier must be changed. But it is a very ugly practice.

> These are really long.  Would you mind splitting them up to be more like
> this:
>     <menuitem label="&deleteAddrBookCard.label;"
>               accesskey="&deleteAddrBookCard.accesskey;"
>               oncommand="AbDelete();"/>

Sure, I'll do.
Comment 11 Blake Winton (:bwinton) (:☕️) 2011-11-15 14:06:09 PST
(In reply to :aceman from comment #10)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #9)
> > and make the buttons match these, but we can't, because alt-t is the Tools
> > menu.
> So wouldn't it be ugly inconsistent? So what is better? Should I change the
> menu and leave the button alone or leave it as is in the patch?

Leave it as it is in the patch.

> > > > -<!ENTITY ccButton.accesskey                 "d">
> > > > +<!ENTITY ccButton.accesskey                 "C">
> > Ooh, do we need to change this for localizers?
> Yes, you taught me the identifier must be changed. But it is a very ugly
> practice.

So, normally I'ld agree, but in this case I'm not sure…  It's unlikely that the Russian translation used "d" in the first case, and so they probably don't care if we switch it to "C"…

Thomas?  Any ideas?

Thanks,
Blake.
Comment 12 :aceman 2011-11-15 14:14:15 PST
Russian translation?
Comment 13 Blake Winton (:bwinton) (:☕️) 2011-11-15 14:17:05 PST
Just an example.  I'm not sure how much other languages care about which access keys we chose for English, and that will affect whether or not we need a new id for it.
Comment 14 Thomas D. (currently busy elsewhere; needinfo?me) 2011-11-16 01:13:32 PST
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #11)
> > > > > -<!ENTITY ccButton.accesskey                 "d">
> > > > > +<!ENTITY ccButton.accesskey                 "C">
> > > Ooh, do we need to change this for localizers?
> So, normally I'ld agree, but in this case I'm not sure…  It's unlikely that
> the Russian translation used "d" in the first case, and so they probably
> don't care if we switch it to "C"…
> Thomas?  Any ideas?

It's not that I am in any way especially knowledgeable about these (access)key and label things...
Looking at Bug 452634 Comment 38 and Bug 452634 Comment 39, and judging from common sense, I'd tend to agree with Blake that it's ok to just change our en-US accesskey as long as we do not change the associated label.

So my understanding would be this:
- any change of a .label needs a new string ID
- IDs for .label and .accesskey always have to be in sync, so changing the ID for the label requires changing the ID for the .accesskey, too.
- in this case, it's the other way round: we are just changing the .accesskey for our own locale without touching the .label. So indeed, the Russian translation won't be affected.

BUT, two questions remain:

1) We had a purpose/reason for changing the accesskeys, namely to avoid double use of same accesskey within one context, and to align accesskeys as much as possible, e.g. between the "Add to..."-Buttons and the same commands on the context menu. I suppose we want the same principle to be applied for the Russian localization, too (as an example). So, assuming they do *not* get notified of our .accesskey change for en-US (or do they?), then how do they get notified of our useful intentions so that they can try the same on their language set?
Perhaps just adding a localization note could do (but then, will localizers get notified about added notes if there are no actual string ID changes in that corner?)

The localization note could be:
".accesskey properties for the "Add to..."-Buttons on the contacts side bar and the respective "Add to... field" commands on the context menu should be the same as far as possible."

2) What about other flavors of English? How do they get notified of our .accesskey changes, which they should copy? (I know nothing about that)

As a side note, I find it very odd how an insufficient localization tool ever again leads to this sort of discussions. Somebody should fix the tool, as it's absolutely ridiculous that we have to change the string ID (and more) for any little change in the label. Furthermore, it's time-wasting, error-prone and produces bad code, with all those useless numbers added to string IDs. Which has been said before:

(Robert Kaiser (:kairo@mozilla.com) from bug 452634, comment #43):
> I hope that with L20n we'll get some tooling that will make stuff like that
> cleaner.

(:aceman (away 16.-21.) from comment #10)
> Yes, you taught me the identifier must be changed. But it is a very ugly
> practice.

+1!

Do we have a bug for improving/fixing the localization tool?
Comment 15 :aceman 2011-11-21 13:54:32 PST
I find it strange these questions were not resolved and the answers standardized when those tools are used for so many years.

I have done rewrapping from comment 9. So what else should I do?
Comment 16 Thomas D. (currently busy elsewhere; needinfo?me) 2011-11-29 01:21:28 PST
(In reply to :aceman from comment #15)
> I find it strange these questions were not resolved and the answers
> standardized when those tools are used for so many years.

I'd think that needs another new bug, like
"Provide documentation / FAQ about access keys in Thunderbird"

There's a Firefox-centric documentation from an XUL UI perspective, which however does not cover the coding part of it:
https://developer.mozilla.org/en/XUL_Accesskey_FAQ_and_Policies
Comment 17 Thomas D. (currently busy elsewhere; needinfo?me) 2011-11-29 01:54:32 PST
Gentlemen, while changing access keys on the fly, we overlooked this:

The current patch hijacks an access key which is already taken:
Add to _C_C (the button in contacts side bar, after the patch)
X atta_c_hment(s) (the label to access attachment panel)

Proposed fix:
Add to _C_C (the button in contacts side bar, after the patch)
X attach_m_ent(s) (the label to access attachment panel)

Reasons:
1) Add to CC is important enough that it should have a mnemonic access key (_c_)
2) Consequently, need to change current access key for attachment pane from _c_ to _m_ (please double check if there are any circumstances under which _m_ is taken by something else of importance)
3) _m_ for attachments is not mnemonic, but current _c_ for attachments was not mnemonic, either, so we are not loosing anything. In fact, _m_ might be better if you imagine things like "_m_anage attachments", "_m_essage attachments" etc.

FTR (testing alternatives)
4) attachments cannot have _a_ because it's taken by _A_dd to To, which cannot have _t_ because that's taken by _T_ools which as main menu takes priority.
5) _d_ for A_d_d to To would be free after the patch, but we'd loose the last inch of mnemonic quality for that important function then. It's bad enough we cannot use _t_, so let's not make things worse, given 3) where at least "m" for attach_m_ents has /some/ mnemotic value.
6) attachments cannot any other access key except _n_, as they are all taken. Arguably, _m_ has more mnemonic value than _n_, and it's the first free access letter which I think is the default procedure in such cases.

(In reply to :aceman from comment #15)
> I have done rewrapping from comment 9. So what else should I do?

aceman, could you pls provide a new patch that implements access key problem fix as explained in 2) above:
X attach_m_ent(s)

After that, pls request ui-review? (for this fix of comment 17) and review? again since we are adding more code. It's Blake's call then to look at localization issues again, as explained in my comment 14.
Comment 18 :aceman 2011-12-04 07:37:46 PST
Created attachment 578903 [details] [diff] [review]
patch updated to comment 17
Comment 19 Blake Winton (:bwinton) (:☕️) 2011-12-13 14:49:43 PST
Comment on attachment 578903 [details] [diff] [review]
patch updated to comment 17

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

The new keys are what was asked for, so ui-r=me.
And the code changes seem fine, or at least I'm willing to take the heat if the localizers complain, so I'm going to say r=me.
Comment 20 :aceman 2011-12-13 15:06:01 PST
Thanks.
Comment 21 Mark Banner (:standard8) 2011-12-16 14:36:13 PST
Checked in: http://hg.mozilla.org/comm-central/rev/29264fa15183

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