1473488 - Restore control="attachmentBucket" attribute on attachmentBucketCount label (prevent broken attachment pane access key for deviant localizations like DE)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

In Bug 1461170, I removed the control="attachmentBucket" attribute from the "N attachments" label(1). Iow, we rendered the displayed access key (Alt+M for en-US) technically dead, but restored its functionality using key_toggleAttachmentPane with identical keyboard shortcut Alt+M, via toggleAttachmentPane() function.

I did so to work around a little XUL flaw which clears the tree selection when using the access key of the controlling element. This is unexpected because imo, access key should just move focus, not clear selections. It's also ux-inconsistent in that different ways of focusing will yield different results:
Coming from subject, press Tab to focus attachment list: selection preserved.
Using Alt+M access key to focus attachment list: selection cleared.

So far so good, and it's working fine for en-US where access key and keyboard shortcut are the same per intended design (Alt+M).
Unfortunately, this workaround will backfire quite badly for non-conforming localizations like German (seen on TB60b7):
They defined Alt+N as access key, but kept Alt+M for the shortcut key.
Which in turn causes the access key (Alt+N) to be completely disfunctional, which is pretty nasty in terms of access.

I don't think we are in a position to check all localizations to enforce access key == shortcut key. So I think we must err on the safe side and restore the control attribute: Unexpected clearing of selection when using access key is a rather trivial flaw, whereas broken access key for attachment pane is quite nasty and should never happen. Removing control attributes really isn't a good way of fixing cosmetic XUL bugs. From my tests, it's also disadvantageous for screen readers as the label will no longer be associated with the list (coming from bug 1466550).

I wish someone could find that spot in XUL where the controlling access key causes the tree selection to be cleared, but for now, let's just ensure that the access key works (also for our pane toggling magic).


1):

Control attribute was removed in https://hg.mozilla.org/comm-central/rev/a0bd1399d616, when we touched the header labels to add list context menu.

Corresponding code to catch Alt+M in attachmentBucketOnKeyPress() function was removed in https://hg.mozilla.org/comm-central/rev/9c3e591f3a0b, in the polish/leftovers patch.

Comment 1

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Restore control attribute of attachmentBucketCount label

Aceman, could you please review this fast because we should land this for TB 60 to avoid broken access keys for attachment pane. Tia.

So here's a patch which restores the control attribute of attachmentBucketCount label, i.e. the direct link between the label and the attachment list.

Looking at the German localization of 60b7, it's wise to do this not only for the benefit of screen readers (to associate the label with the list), but also for ux-error-prevention in deviant localizations which don't make the access key the same as the shortcut key: German has Alt+N as access key, and Alt+M as shortcut key. Without this patch, this causes the access key to do nothing; with this patch, both access key AND different shortcut key will provide full access to the new bucket magic (focus/show or hide attachment pane).

Aceman, I have added a detailed comment in code which describes why this is needed and how things interact. It's a bit delicate so imo we should be clear about this.

In theory after this patch we could eliminate the shortcut key altogether but then it becomes harder to advertise the access key (aka shortcut key) on the View menu. Having <key> makes it simple for menu advertising, but then we must also let the key trigger the action because if we don't, we'll just reverse the problem currently seen in German locale, namely that now the access key would work but the different shortcut key wouldn't.

Fwiw, I think German should make the shortcut key and access key identical as we do in English, because composition access keys are in short supply (Bug 1469835).

Comment 2

[:aceman]

Comment on attachment 8989922 [details] [diff] [review]
Patch V.1: Restore control attribute of attachmentBucketCount label

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

Thanks.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5432,5 @@
> +  // For user's convenience, we want the access key combo for attachments pane
> +  // (e.g. Alt+M) to also work as a shortcut key to toggle the attachment pane.
> +  // Hence localizations have been advised to define the access key combo as a
> +  // shortcut key in <key_toggleAttachmentPane/>. This is important especially
> +  // for MAC which does not have access keys for UI elements, but it should work

I think MAC is written as "Mac".

@@ +5445,5 @@
> +  // control attribute of the access key breaks screen readers. Sigh.
> +  let attachmentsAccessKey = document.getElementById("attachmentBucketCount")
> +                                     .accessKey;
> +  // We can get away with hardcoding the access key modifier key as aEvent.altKey
> +  // because it's ALT for Windows and Linux, and MAC doesn't have XUL access keys.

And here.

Comment 3

[Jorg K (CEST = GMT+2)]

I'll take care of the spelling, no new patch needed.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/777d72e3e2d2
Backed out changeset a933eaed53bf since it landed with wrong bug number. a=backout
https://hg.mozilla.org/comm-central/rev/0a4b2a9b30d8
Restore control attribute of attachmentBucketCount label for screen reader access and ux-error-prevention in deviant localizations. ui-r=Paenglab, r=aceman DONTBUILD

Comment 5

[Jorg K (CEST = GMT+2)]

TB 60 beta 10 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/d15e1e303df0f85194b4a5c927f4713a7ff59713

Beta (TB 62):
https://hg.mozilla.org/releases/comm-beta/rev/082700ece005147c802e7862f0d73fe0cac271b2

Aryx, do you think you could still change this to an "n":
https://hg.mozilla.org/l10n-central/de/rev/19c5811f5e3d#l9.22
attachments.accesskey
toggleAttachmentPaneCmd.accesskey
toggleAttachmentPaneCmd.key
should all be the same. You have the last one as "m". Somehow I still see the untranslated "Attachment Pane Alt+M" under "Ansicht". Could it be that due to the fact that "m" doesn't match "Anhangbereich"?

Comment 6

[Jorg K (CEST = GMT+2)]

Also see:
https://groups.google.com/forum/#!topic/mozilla.dev.l10n/zaGN6GQObcI

Comment 7

[Jorg K (CEST = GMT+2)]

Thomas, please join that discussion. I'm not in a position to argue in favour of the solution.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: attachments-DE.png

Any idea why "Attachment Pane Alt+M" isn't translated? Seen in TB 60 beta 9.

Comment 9

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #7)
> Thomas, please join that discussion. I'm not in a position to argue in
> favour of the solution.

Thanks for the pointer, I explained the solution and its motivations on the newsgroup.

Comment 10

[Onno Ekker [:nONoNonO UTC+1]]

I really don't understand why the "N attachment(s)" string does have an accesskey, because there is no way to access it but to use the command key Alt+M as defined in the menu View > Attachment pane.

Comment 11

[Thomas D. (:thomas8)]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #10)
> I really don't understand why the "N attachment(s)" string does have an
> accesskey, because there is no way to access it but to use the command key
> Alt+M as defined in the menu View > Attachment pane.

You are looking at the wrong version, and maybe you haven't read my comment 0 where this is explained in detail ;-)
That's exactly what this bug has fixed for TB 60 beta 10, for deviant localizations like German which did not follow the localization instructions to make the access key and the shortcut key the same (Alt+N for German). For conforming localizations, it already works in beta 9.

For the avoidance of doubt, there is NO dedicated (international) shortcut key for toggling attachment pane, only an access key with extended functionality (currently realized via <key key="...">, but that's just a technicality). Alt+M shortcut key seen in en-US is NOT intended as an international shortcut key, it's just the English flavor of realizing extended access key functionality for en-US, which happens to have Alt+M as an access key.

(In reply to Jorg K (GMT+2) from comment #5)
> TB 60 beta 10 (BETA_60_CONTINUATION branch):
> https://hg.mozilla.org/releases/comm-beta/rev/
> d15e1e303df0f85194b4a5c927f4713a7ff59713

As I hinted on the newsgroup, we might want to make it easier and less error-prone for localizers by removing the technicality of the shortcut <key> altogether, then adding a bit of code to advertise the local access key on the View > Attachment Pane menuitem, and some special code for Mac OS which ostensibly doesn't have access keys.

Comment 12

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/bba79f0a1b0d

Comment 13

[Thomas D. (:thomas8)]

FTR:

This bug does NOT fully fix deviant localizations as I have erroneously claimed before. The only safe and full fix to make it work correctly as designed for every locale is bug 1475828, so we should land the one-liner patch there asap for TB 60 to avoid broken localizations. Bug 1475828 is required to fix a shortcoming of the current implementation which makes the functioning of the current "extended access key" design depend on correct localization of toggleAttachmentPaneCmd.key, which failed because localizers wrongly assumed that Alt+M (the access key for en-US) is an *international shortcut key*:
<key id="key_toggleAttachmentPane" key="&toggleAttachmentPaneCmd.key;" modifiers="alt">
But an international (one-for-all) shortcut key CANNOT normally have ALT modifier, because that's reserved for access keys and hence unpredictable and error-prone.

German localization (which is normally quite accurate) is a good example of the resulting UX failure without bug 1475828:
- attachments.key = N -> Access key for German "# Anhä_n_ge" (attachment pane header label, where # represents the number of attachments)
- toggleAttachmentPaneCmd.key = M -> Deviant shortcut key, wrongly (not) localized assuming that it's an international shortcut key, which it isn't.

Consequently, the UX of deviant localizations like German is broken:
- Alt+N (German access key) FAILS to summon the bucket when it's empty+hidden or full+minimized; can hide the bucket when focused (albeit undiscoverable).
- Alt+M /generally/ works for the full magic (as designed and ui-reviewed):
  - summon (show & focus when hidden/minimized)
  - focus (when shown but not focused)
  - hide (when shown and focused)
- BUT: Alt+M FAILS completely when automagical Attachment Reminder bar happens to appear before attachments have been added (type "Anhang" in message body before adding attachments):
  The existing access key of German's "Remind me later" button ("_M_ich später erinnern"), Alt+M, hijacks the deviant, identical command <key> (Alt+M) for toggling attachment pane.

Similar failures are likely to occur for other locales, because we can't just introduce a random international shortcut key "Alt+M" which (due to the ALT modifier) is de facto an access key and hence likely to conflict with existing access keys for any locale. If deviant translations are not breaking attachment pane toggling, they might break something else instead.
It's crucial to understand that Alt+M is NOT (and was never meant to be) an international shortcut key, but just the English flavor of atachment pane access key. As seen in the English locale, everything works perfectly when localized attachments.accesskey == command <key> (== Alt+M for en-US). Note that by design, <key> is just a technical tweak: Extending/overloading the functionality of the existing, unique access key will always work reliably as designed for any given locale. Bug 1475828 simply ensures that the extended access key works for any given localization, technically bypassing the risk of deviant translations:
<key id="key_toggleAttachmentPane" *key="&attachments.key;"* modifiers="alt">