Contacts sidebar: Missing accelerator keys on context menu

VERIFIED FIXED in Thunderbird 11.0

Status

Thunderbird
Message Compose Window
P1
minor
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Thomas D. (currently busy elsewhere; needinfo?me), Assigned: aceman)

Tracking

11 Branch
Thunderbird 11.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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".
E is a good accelerator for Delete, because it can stand for "Erase"
(Assignee)

Comment 2

6 years ago
I could try to patch this.

Bwinton, is this wanted?
Component: Address Book → Message Compose Window
QA Contact: address-book → message-compose
Yeah, I'ld like it if you could fix this.  :)

Thanks,
Blake.

Comment 4

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

Comment 5

6 years ago
Good idea.
Assignee: nobody → acelists
Priority: -- → P2
(Assignee)

Comment 6

6 years ago
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?
Attachment #574092 - Flags: ui-review?(bwinton)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Version: Trunk → 11
(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.
(Assignee)

Comment 8

6 years ago
Created attachment 574154 [details] [diff] [review]
patch implementing comment 0 and comment 4, 2nd version

OK, sounds good.
Attachment #574092 - Attachment is obsolete: true
Attachment #574092 - Flags: ui-review?(bwinton)
Attachment #574154 - Flags: review?(bwinton)
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();"/>
Attachment #574154 - Flags: ui-review+
Attachment #574154 - Flags: review?(bwinton)
Attachment #574154 - Flags: review+
(Assignee)

Comment 10

6 years ago
(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.
Priority: P2 → P1
(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.
(Assignee)

Comment 12

6 years ago
Russian translation?
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.
(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?
(Assignee)

Comment 15

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

Comment 18

6 years ago
Created attachment 578903 [details] [diff] [review]
patch updated to comment 17
Attachment #574154 - Attachment is obsolete: true
Attachment #578903 - Flags: ui-review?(bwinton)
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.
Attachment #578903 - Flags: ui-review?(bwinton)
Attachment #578903 - Flags: ui-review+
Attachment #578903 - Flags: review+
(Assignee)

Comment 20

6 years ago
Thanks.
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/29264fa15183
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
(Assignee)

Updated

5 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.