1475828 - Eliminate localization errors of extended access key functionality for toggling composition's attachment pane

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1473488 +++

This bug eliminates localization errors of the new convenience functionality where the localized attachment pane access key (e.g. Alt+M for en-US) now also works to summon or toggle the attachment pane by design.

Technically, we require (access key == command key) for this to work as expected. Unfortunately, I got it all wrong wrt implementation and we've confused localizers quite a bit in the process, so many got it wrong (1). Fortunately, we can fix this with a one-liner:

>    <key id="key_toggleAttachmentPane"
> -       key="&toggleAttachmentPaneCmd.key;" modifiers="alt"
> +       key="&attachments.accesskey;" modifiers="alt"

attachments.accesskey is the pre-existing, unique localized access key on the "N attachments" label (if not unique, it's broken). So this ensures (access key == command key) out of the box, hence bypassing the error-prone need for localizing the command key altogether. Problem solved.


More Background:

Composition's revamped attachment pane comes with a convenience feature of extending the function of the existing localized access key to also toggle the attachment pane (show/hide). E.g. for en-US, Alt+M will conveniently summon and focus the empty bucket even when it's hidden, so users only have to remember a single key combo for controlling the attachment pane.

* Convenient: high mnemonic value (same localized key combo for getting into and out of attachment pane)
* Discoverable: advertised on the label and on the |View > Attachments| menuitem.
* Efficient: one-for-all, simple key combo
* Sustainable: Avoids wasting an "international" shortcut key with little or no mnemonic value (like F11); however, the extended access key functionality does not prejudice or hinder having another dedicated shortcut key in any way.

(1): Many localizations failed ensure (attachments.accesskey == toggleAttachmentPaneCmd.key):

https://dxr.mozilla.org/l10n-central/search?q=%22%3C!ENTITY%20toggleAttachmentPaneCmd.key%22&redirect=false
   vs.
https://dxr.mozilla.org/l10n-central/search?q=%22%3C!ENTITY+attachments.accesskey%22&redirect=false

Comment 1

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Fix extended access key functionality; F11 for Mac OS

So here's the one-liner which fixes it (plus some corrections/redactions of comments).

* Eliminate localization problem by ensuring (attachments.accesskey == command key) - Windows and Linux only

>    <key id="key_toggleAttachmentPane"
> -       key="&toggleAttachmentPaneCmd.key;" modifiers="alt"
> +       key="&attachments.accesskey;" modifiers="alt"

* Mac OS doesn't have access keys afasics, so I implemented F11 for them to focus/show/hide attachment pane
* Which in turn requires making F11 secondary shortcut on Windows/Linux for cross-OS consistency

Richard, could you please test if this works correctly as before, and if F11 does the trick for MAC? For testing on en-US:

1) Alt+M (Win/Linux) or F11 (any OS) with empty, hidden bucket:
   -> summon and focus bucket
2) Alt+M (Win/Linux) or F11 (any OS) with shown, but unfocused bucket:
   -> focus bucket (classic access key)
3) Alt+M (Win/Linux) or F11 (any OS) with focused bucket:
   -> hide empty bucket/minimize full bucket

Comment 2

[Thomas D. (:thomas8)]

Oh, it's F11 because we already have F9 for contacts side bar, F10 is menu key on Windows, so the next available function key looks F11, which may have a certain mnemonic value as it kinda mirrors the visual distribution of panes:
Left side (contacts, F9) - Middle: message header/body (skip one, F10) -  Right side (attachments, F11)
Also, most character shortcut keys are already taken, would be more complex, and probably shouldn't be consumed for this if they are still free.

Comment 3

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere) from comment #1)
> Created attachment 8992208 [details] [diff] [review]
> Richard, could you please test if this works correctly as before, and if F11
> does the trick for MAC? For testing on en-US:
> 
> 1) Alt+M (Win/Linux) or F11 (any OS) with empty, hidden bucket:
>    -> summon and focus bucket

More correctly:
1) Alt+M (Win/Linux) or F11 (any OS) with empty+hidden or full+minimized bucket:
   -> summon and focus bucket

> 2) Alt+M (Win/Linux) or F11 (any OS) with shown, but unfocused bucket:
>    -> focus bucket (classic access key)
> 3) Alt+M (Win/Linux) or F11 (any OS) with focused bucket:
>    -> hide empty bucket/minimize full bucket

Comment 4

[Thomas D. (:thomas8)]

Note that on MAC, you might have to press Fn+F11 t get F11...

Comment 5

[Thomas D. (:thomas8)]

Comment on attachment 8992208 [details] [diff] [review]
Patch V.1: Fix extended access key functionality; F11 for Mac OS

Grrr. Whoever coded XUL was really careless and lazy. They allow <key id="key-id" modfier="access" key="M"/>, but there's no corresponding keyevent.accessKey, nor any representation of that modifier in the keytext of menuitems with key="key-id", resulting in an incomplete keytext " + M". So we have to restore plain vanilla modifier="alt". New patch coming.

Comment 6

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1: (Nitfix keytext) Fix extended access key functionality; F11 for Mac OS

Ok, so here's the quasi one-liner again, with comment 5 nitfix.

For (ui-)review, pls refer to comment 1, comment 2, comment 3, comment 4.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1475828.png - screenshot with access key set to Y

So this forces the shortcut key, in this case a faked "Y", onto the header of the bucket, yes?

In the current solution from bug 1473488 the access key of the header forces the shortcut action. Was that so bad?

This looks strange to me: key="&attachments.accesskey;"

Sorry, I didn't read all the comments, can you give me an executive summary.

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 8992210 [details] [diff] [review]
Patch V.1.1: (Nitfix keytext) Fix extended access key functionality; F11 for Mac OS

F11 doesn't work on Mac because this key is used to show the desktop. Also does ^m no more work. In the menu is only "V" as access kex shown but this doesn't work.

Comment 9

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #7)
> Created attachment 8992212 [details]
> 1475828.png - screenshot with key set to Y
> 
> So this forces the shortcut key, in this case a faked "Y", onto the header
> of the bucket, yes?

No, the other way round: This forces the header access key (attachments.accesskey) onto the command shortcut key (so that the access key can work for extended functionality):

>    <key id="key_toggleAttachmentPane"
> -       key="&toggleAttachmentPaneCmd.key;" modifiers="alt"
> +       key="&attachments.accesskey;" modifiers="alt"

> In the current solution from bug 1473488 the access key of the header forces
> the shortcut action. Was that so bad?

It did NOT enforce it, that was the problem. We left it to localizers, who didn't understand what we're trying to do, and got it all wrong (and it's also misleading in tb60b10 release notes, which presents Alt+M like a universally valid shortcut key, but it's really just a localized access key with extended functionality - we're "overloading" the access key, so to speak).

> This looks strange to me: key="&attachments.accesskey;"

That's where we force the access key from the header into the command shortcut key. Can't fail for Windows and Linux.
Richard has pointed out problems on Mac; difficult for me to fix because it's a blind flight.

> Sorry, I didn't read all the comments, can you give me an executive summary.

More like you didn't read any of the comments, not even comment 0 ;-)

Comment 10

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #8)
> Comment on attachment 8992210 [details] [diff] [review]
> Patch V.1.1: (Nitfix keytext) Fix extended access key functionality; F11 for
> Mac OS
> 
> F11 doesn't work on Mac because this key is used to show the desktop.

Did you try fn+F11 which is the "real" F11 key?
Are you sure that this is a default shortcut key? If so, why then is it not listed in Mac official keyboard documentation, which looks recent (2018-06-15)?
According to official docs, Desktop is Command+Mission Control, which is Command + (unmodified) F3 key (with some random window symbols of different sizes).
MAC Keyboard shortcuts: https://support.apple.com/en-us/ht201236
> Command–Mission Control: Show the desktop. 
Mission Control key depiced: https://support.apple.com/en-us/ht204100
MAC Qwerty keyboard layout (with Mission control key == F3): https://cdn-images-1.medium.com/max/1600/1*hGEEitUgxU4nQxtX8q1rsA.jpeg

Btw we should check if we hijacked Control–Up Arrow for attachment reordering, which also seems to be alternative shortcut for mission control. But sometimes hijacking is good ;-)

> Also does ^m no more work.

Control+M? Yes, I removed that, because according to many sources, MAC does NOT have a concept of access keys.
How would users even know about it if there's no underlined character?
The problem is that even though Control-M would work for en-US and be free according to MAC shortcut list, there are dozens of Control-* default MAC shortcuts which are taken, so Control+attachments.accesskey is guaranteed to fail for other access keys of other localizations. So we can't do it; hence F11 instead, which is free according to the list.

> In the menu is only "V" as access key shown

Strange. Where would the "V" come from?
Oh. Maybe from key="VK_F11"?? but strangely that works on Windows. Meaning XUL coders have messed up again.
We can try keycode="VK_F11" instead, I think that's cleaner and should work.

>  <key id="key_toggleAttachmentPane2"
        keycode ="VK_F11"
>       command="keyToggleAttachmentPaneOnCommand();"/>

> but this doesn't work.

Yeah, "V" alone won't work as it's not defined anywhere :-)

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere) from comment #9)
> No, the other way round: This forces the header access key
> (attachments.accesskey) onto the command shortcut key (so that the access
> key can work for extended functionality):
> >    <key id="key_toggleAttachmentPane"
> > -       key="&toggleAttachmentPaneCmd.key;" modifiers="alt"
> > +       key="&attachments.accesskey;" modifiers="alt"

But that's exactly what the majority of people on the l10n list *DID NOT WANT*. They *DO NOT WANT* to localise the shortcut command keys. They say it's bad, even in MS/Libre Office.
https://groups.google.com/d/msg/mozilla.dev.l10n/zaGN6GQObcI/9ay5p_XFCAAJ

So are you trying to force your view onto them? The localised access key will then become the shortcut command key?

Clearly a WONTFIX and r-. Or what have I not read again?

Comment 12

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #7)
> In the current solution from bug 1473488 the access key of the header forces
> the shortcut action. Was that so bad?

Fyi, I have not touched or reverted bug 1473488. I actually enforced/secured the intention of bug 1473488 by eliminating potential and real localization errors (localizers can no longer define different shortcut key).
My claim that after bug 1473488, if access key and shortcut key are different, both would trigger the full 3fold magic, was wrong. The access key and onkeypress of bucket alone can't do it because when bucket is hidden, access key is not available, so only shortcut key can summon and focus the empty bucket.

Comment 13

[Jorg K (CEST = GMT+2)]

As I said:
>    <key id="key_toggleAttachmentPane"
> -       key="&toggleAttachmentPaneCmd.key;" modifiers="alt"
> +       key="&attachments.accesskey;" modifiers="alt"
is inherently wrong. See
https://groups.google.com/d/msg/mozilla.dev.l10n/zaGN6GQObcI/C5BeYUhfBwAJ
  It looks like this UI is mixing up access keys and shortcuts?

We can't make most the l10n community our enemies just because we had some neat idea.

Comment 14

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #11)
> (In reply to Thomas D. (currently busy elsewhere) from comment #9)
> > No, the other way round: This forces the header access key
> > (attachments.accesskey) onto the command shortcut key (so that the access
> > key can work for extended functionality):
> > >    <key id="key_toggleAttachmentPane"
> > > -       key="&toggleAttachmentPaneCmd.key;" modifiers="alt"
> > > +       key="&attachments.accesskey;" modifiers="alt"
> 
> But that's exactly what the majority of people on the l10n list *DID NOT
> WANT*. They *DO NOT WANT* to localise the shortcut command keys.

TL;DR:
- we already have localized "shortcut keys", i.e. key combos which trigger commands (e.g. localized Alt+* for "Add to To|CC|BCC" buttons), what's the fuss about? The difference between access keys and shortcut keys is very subtle and rather technical, not something most users would ever notice as a problem.
- as I already stated on the newsgroup and here (but then you don't read me and hence make me to explain more and repeat everything and appear insistent), enabling and securing extended functionality of the localized access key does NOT hinder or prejudice having "international" shortcut/command key for the same functionality in any way (so wontfix would be totally inappropriate), and moreover:
- did you even read my patch? As my patch already DOES implement "international" shortcut/command key for attachment toggling, namely F11. Only I don't advertise that on the menu yet, because I still think that overloading the local access key is superior UX compared to F11. Please tell my why a German user would prefer F11 over Alt+N as a key combo for this functionality? But changing the key(text) on the menu to advertise F11 is no problem if we agree on that, and just requires another 1-line change.
- I had also asked the newsgroup to propose an international shortcut key for toggling attachment pane, and there was no response except for general prejudiced and uninformed clamor. Because Alt+M *cannot* be the international shortcut key, and was never meant to be, according to the currently implemented design of just overloading the access key which has ui-r+ after being tried and tested.

In depth:

Is it about the process of localizing shortcut command keys, or having localized shortcut keys, or both?
After this bug (per my proposed patches here), there's nothing to localize any more, it will just work out of the box.
Conceptually, there is NO localized shortcut key, only the existing localized access key, whose functionality we extend. That concept already has ui-r+ and I have spelled out the benefits in several comments on bugs (e.g. comment 0 here) and on the newsgroup, and I haven't heard a single specific counter-argument, only general sentiments.
The claim that we do not have localized shortcut keys is simply not true, here's why:
From a general users pov, there's no essential or practical difference between access keys and shortcut keys - both of them are just key combos consisting of modifier(s) plus some key. Our access keys also trigger commands, e.g. the "Add to To", "Add to CC" and "Add to BCC" buttons, and all of these access/shortcut key combos are localized. Please explain to me how Alt+M to trigger some function is fundamentally different to Ctrl+M to trigger some function.

> They say it's bad, even in MS/Libre Office.
> https://groups.google.com/d/msg/mozilla.dev.l10n/zaGN6GQObcI/9ay5p_XFCAAJ

Dito, we already have localized "shortcut"/command keys, e.g. all access keys on buttons.
Two or three people clamoring on a newsgroup is not a crowd, nor are they UX experts.
Good UX doesn't come from clamor. But I'm not ignoring, please don't get me wrong.

> So are you trying to force your view onto them?

I'm not forcing anything on anyone, please refrain from such imputations.
The problem is that localizers have not understood what we are trying to do, and why this is good, and why this can't be harmful or in the way of anything else which we might want on top of this. Without the patch here, they can't even see the overloaded access key in action, because it simply won't work for deviant localizations.

> The localised access key will then become the shortcut command key?

Nothing new here, that's the UX concept which already has ui-r+, which we already have in en-US, and which we want other locales to also benefit from. But most importantly, as I said, overloading the access key can never be detrimental, nor does it stop us from having AND advertising another, dedicated "international" shortcut key like F11 which I already implemented.

> Clearly a WONTFIX and r-. Or what have I not read again?

Almost everything... :/

Comment 15

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #13)
> As I said:
> >    <key id="key_toggleAttachmentPane"
> > -       key="&toggleAttachmentPaneCmd.key;" modifiers="alt"
> > +       key="&attachments.accesskey;" modifiers="alt"
> is inherently wrong.

It can't be wrong because overloading the access key in itself can never hurt anyone and doesn't even involve localization after this patch - just don't use it if you don't like it, but the localized access key already exists and is taken so nothing to lose. Then we can still take it from there and try to make the alleged majorities happy...

> See
> https://groups.google.com/d/msg/mozilla.dev.l10n/zaGN6GQObcI/C5BeYUhfBwAJ
>   It looks like this UI is mixing up access keys and shortcuts?

No, not really, as explained above (extended/overloaded access key functionality). And there's no big difference anyway, we already have localized key combos in the UI.
 
> We can't make most the l10n community our enemies just because we had some
> neat idea.

Those few clamors on the newsgroup are not "most of the l10n community".
This is about good UX and "enemies" is an inappropriate and exaggerated notion for that.
But anyway, I already implemented "international" shortcut key F11 so nothing stops us from advertising that instead if we agree.

Comment 16

[Thomas D. (:thomas8)]

Where we really mix access key and shortcut key is Alt+X for Reorder Attachments, which is likely to go wrong for other localizations as it already has for German (without attachments, Alt+X triggers Extras menu (ok); with attachments, Alt+X triggers "Reorder Attachments", i.e. the reorder shortcut key Alt+X hijacks the menu access key Alt+X. (which is actually an oversight of German localizers, but anyway...)
Alt+X is fine as alternative shortcut for en-US but maybe we need Ctrl+Shift+X as a locale-independent shortcut which won't break easily. Until we hear some addon complaining that we've taken their shortcut key...

Comment 17

[Richard Marti (:Paenglab)]

(In reply to Thomas D. (currently busy elsewhere) from comment #10)
> (In reply to Richard Marti (:Paenglab) from comment #8)
> > Comment on attachment 8992210 [details] [diff] [review]
> > Patch V.1.1: (Nitfix keytext) Fix extended access key functionality; F11 for
> > Mac OS
> > 
> > F11 doesn't work on Mac because this key is used to show the desktop.
> 
> Did you try fn+F11 which is the "real" F11 key?

Naturally I did, without fn I have no F-keys on the touchbar.

> > Also does ^m no more work.
> 
> Control+M? Yes, I removed that, because according to many sources, MAC does
> NOT have a concept of access keys.
> How would users even know about it if there's no underlined character?
> The problem is that even though Control-M would work for en-US and be free
> according to MAC shortcut list, there are dozens of Control-* default MAC
> shortcuts which are taken, so Control+attachments.accesskey is guaranteed to
> fail for other access keys of other localizations. So we can't do it; hence
> F11 instead, which is free according to the list.
> 
> > In the menu is only "V" as access key shown
> 
> Strange. Where would the "V" come from?
> Oh. Maybe from key="VK_F11"?? but strangely that works on Windows. Meaning
> XUL coders have messed up again.
> We can try keycode="VK_F11" instead, I think that's cleaner and should work.

Until now ^m is shown in the menu on Mac. So this is very well visible.

Comment 18

[Thomas D. (:thomas8)]

Comment on attachment 8992212 [details]
1475828.png - screenshot with access key set to Y

Clarifying description to match reality. The pre-existing, unique localized access key is "overloaded" with extended functionality of also toggling the pane, which we then advertise on the menu (technically realized via shortcut <key key="&attachments.accesskey;">, working correctly out of the box without any further localization).

Comment 19

[Thomas D. (:thomas8)]

Attachment: Patch V.1.2: (Nitfixes) Fix extended access key functionality; F11 for Mac OS (and alternative on Win/Linux)

This patch still has F11 for Mac just to try that out, but it looks not feasible.

(In reply to Richard Marti (:Paenglab) from comment #17)
> (In reply to Thomas D. (currently busy elsewhere) from comment #10)
> > (In reply to Richard Marti (:Paenglab) from comment #8)
> > > Comment on attachment 8992210 [details] [diff] [review]
> > > Patch V.1.1: (Nitfix keytext) Fix extended access key functionality; F11 for
> > > Mac OS
> > > 
> > > F11 doesn't work on Mac because this key is used to show the desktop.

Yeah, that's true (though it's not on the official list); F11 seems the older and alternative shortcut for Command+Mission-Control(F3), perhaps also needed for non-Mac keyboards used with Mac (does that work at all?). So if we don't want to steal F10 because it's Window's alternative "menu" key, maybe we also don't want to steal F11 because it's Mac's alternative "Show Desktop" key...
Back to square one, no shortcut for Mac. It's still keyboard accessible on Mac via the menus I believe, so I guess we just have to live with the fact that we can't make Mac as convenient as Win/Linux.

> > Did you try fn+F11 which is the "real" F11 key?
> 
> Naturally I did, without fn I have no F-keys on the touchbar.
> 
> > > Also does ^m no more work.
> > 
> > Control+M? Yes, I removed that, because according to many sources, MAC does
> > NOT have a concept of access keys.
> > How would users even know about it if there's no underlined character?
> > The problem is that even though Control-M would work for en-US and be free
> > according to MAC shortcut list, there are dozens of Control-* default MAC
> > shortcuts which are taken, so Control+attachments.accesskey is guaranteed to
> > fail for other access keys of other localizations. So we can't do it; hence
> > F11 instead, which is free according to the list.
> > 
> > > In the menu is only "V" as access key shown
> Until now ^m is shown in the menu on Mac.

Strange, were you seeing "V" or "Ctrl+M" with the previous patch? Ctrl+M on the menu looks impossible because it's removed from last patch, maybe cache issue?

Anyway, there were two nits in my patch which I fixed, maybe it works as advertised now. This patch should hijack Mac's F11 "Show Desktop" shortcut, but then we probably don't want that.

Comment 20

[Thomas D. (:thomas8)]

Attachment: Patch V.2: Fix extended access key functionality (Win/Linux only; Mac OS does not have access keys)

Right, so here's a plain vanilla patch which just fixes the current design of extending/overloading the access key on Windows/Linux, as already ui-reviewed by Richard. This fixes the problem correctly identified by Jörg that many wrong localizations are breaking the extended access key design. It comes at no cost as it is not prejudicial to finding and having another, dedicated international shortcut key if we really want that and are able to find one.

Please find below a step by step explanation where we are coming from and why we are doing this. Please correct me if I'm wrong.

Otherwise, can we please just land this one-liner asap because as Jörg has pointed out, most localizations did not honor the intended design and therefore are broken (extended access key will NOT work to summon the bucket when hidden, different/deviant shortcut key may or may not work).

1) Every localization already has a unique (localized) access key for "N attachments" label (attachments.accesskey). If it's not unique, it's broken. For en-US, it's Alt+M. In TB 52, the access key can only focus the attachment pane when it's already shown. But then, how to show it when it's not there?

2) We figured (together) that it would be "cool" (highly efficient/mnemonic) if the same access key would also summon the bucket when it's NOT shown; and iirc, Jörg's comment was "I love this 100%", specifically on that feature of allowing the access key (Alt+M for en-US) to also summon the hidden bucket.

3) From there, it took just a little tweak to allow the same access key (e.g. Alt+M) to also HIDE the bucket when it is already focused - there can never be any other reason to use the access key in that situation as the pane already has focus. So pressing the access key (e.g. Alt+M) more than once now offers full control over attachment pane toggling in a cyclic manner:

Extended Access key functionality (at the example of EN-US which has Alt+M):
 a) Alt+M: show ("summon") bucket when unfocused and hidden/minimized ("summon")
[b) Alt+M: focus bucket when unfocused and shown (classic access key)]
 c) Alt+M: hide/minimize bucket when focused and shown ("dismiss")

4) Full extended access key functionality as described in 3 has already been discussed, accepted, and implemented, with ui-r+ from Richard.

5) Thomas' implementation of (attachments.accesskey == command key) wasn't ideal because it depended on localizers understanding and ensuring the extended access key functionality, which failed. Hence this bug, a one-line patch to take the human factor out of the equation and just ensure that it always works as designed (see 3), out of the box:

> >    <key id="key_toggleAttachmentPane"
> > -       key="&toggleAttachmentPaneCmd.key;" modifiers="alt"
> > +       key="&attachments.accesskey;" modifiers="alt"

6) Re-using the access key to do more is intuitive, comes at zero cost (it's already known and taken for any given locale), and saves us from finding and wasting a dedicated "international" shortcut key, which are in short supply.
Plus, we already have a significant number of localized access keys to control the composition window functions and elements, e.g. "Add to To|CC|BCC" buttons. From users' pov, the technical difference between "access keys" and "shortcut keys" is pretty insignificant, they are just fixed key combos to get things done.

7) That said, extended access key functionality (as implemented before and secured by this patch) does NOT hinder/prejudice having a dedicated shortcut key in any way if we want that and are able to find one.

We have tried F11 and failed:
- can't work on Mac because it's OS shortcut for "Show Desktop"
- not future-proof on Win/Linux because F11 is "Full screen" (FF etc.) or "Show Calendar" shortcut, both of which might be needed in composition in the future (e.g. composition in a tab).
- existing localized access key (e.g. Alt+M for en-US) does seem to be more mnemonic than a random international shortcut key

(In reply to comment 0)
> attachments.accesskey is the pre-existing, unique localized access key on
> the "N attachments" label (if not unique, it's broken). So this ensures
> (access key == command key) out of the box, hence bypassing the error-prone
> need for localizing the command key altogether. Problem solved.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8992298 [details] [diff] [review]
Patch V.2: Fix extended access key functionality (Win/Linux only; Mac OS does not have access keys)

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

OK, let's go with this. I had an IRC chat complemented by a "Skype->land line" conversation to motivate this.

For example in German, Alt+M activated the attachment reminder and destroys the command/shorcut "summon the bucket" key.

Since in a proper localisation the attachment bucket's access key needs to be unique and functional, like in German "n" for "A*n*hang", we might as well use this as the command key as well.

We're sort of working a bit against people who don't want to localise command keys, but it's warranted in this case.

Richard, can you check this out on Mac quickly? Thomas is removing the command/shorcut key from the menu.

On second thought, maybe it would be beneficial not to remove the command key on Mac, but rather offer a fixed one. I commented on the code accordingly, I haven't checked with comments need tweaking.

::: mail/components/compose/content/messengercompose.xul
@@ -442,5 @@
>         key="&reorderAttachmentsCmd.key;" modifiers="control"
>         command="cmd_reorderAttachments"/>
> -  <key id="key_toggleAttachmentPane"
> -       key="&toggleAttachmentPaneCmd.key;" modifiers="control"
> -       command="keyToggleAttachmentPaneOnCommand();"/>

Maybe this should stay as a fixed key for Mac only.

@@ +448,5 @@
>    <key id="key_reorderAttachments"
>         key="&reorderAttachmentsCmd.key;" modifiers="alt"
>         command="cmd_reorderAttachments"/>
>    <key id="key_toggleAttachmentPane"
> +       key="&attachments.accesskey;" modifiers="alt"

For Mac we might need to do
key="&toggleAttachmentPaneCmd.key;" modifiers="ctrl"
No warranty on "ctrl".

@@ +1015,2 @@
>                      key="key_toggleAttachmentPane"
> +#endif

This change would go if we left a key for Mac.

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ -97,5 @@
> -     With OS-specific access modifier, this defines the shortcut key for showing/
> -     hiding/minimizing the attachment pane, e.g. Alt+M on Windows.
> -     toggleAttachmentPaneCmd.key and attachments.accesskey must be the same
> -     for the bucket view state machinery to work. -->
> -<!ENTITY toggleAttachmentPaneCmd.key "m">

This would need to be maintained for Mac.

Comment 22

[Richard Marti (:Paenglab)]

Comment on attachment 8992298 [details] [diff] [review]
Patch V.2: Fix extended access key functionality (Win/Linux only; Mac OS does not have access keys)

Absolutely no access key is a no go. TB should be consistent in providing access keys over the platforms.

We should follow Jörgs proposal in comment 21 and use CTRL on Mac as before.

Comment 23

[Thomas D. (:thomas8)]

Attachment: Patch V.3: Fix extended access key functionality (Win/Linux); implement Ctrl+M shortcut for Mac OS which does not have access keys

Thanks.

This patch tries to implement Jörg's proposal of having a fixed, cross-l10n keyboard shortcut (Ctrl+M) for Mac instead of unsupported access keys.

Richard, pls check if Ctrl+M does the following:
 a) Ctrl+M: show ("summon") bucket when unfocused and hidden/minimized ("summon")
[b) Ctrl+M: focus bucket when unfocused and shown (mimick access key)]
 c) Ctrl+M: hide/minimize bucket when focused and shown ("dismiss")

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8994066 [details] [diff] [review]
Patch V.3: Fix extended access key functionality (Win/Linux); implement Ctrl+M shortcut for Mac OS which does not have access keys

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

As discussed with Aceman, only I'll review this. Code looks good pending Richards test and approval for Mac.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5441,5 @@
>    // We can get away with hardcoding the access key modifier key as aEvent.altKey
> +  // because it's ALT for Windows and Linux; Mac doesn't support XUL access keys,
> +  // so for Mac we have defined Ctrl+M as a cross-l10n shortcut key.
> +  if (aEvent.key == attachmentsAccessKey && aEvent.altKey
> +      && AppConstants.platform != "macosx") {

The && should go to the end of the previous line. I can fix that when landing it.

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: Patch V.3.1: Fix extended access key functionality (Win/Linux); implement Ctrl+M shortcut for Mac OS which does not have access keys

Shortened commit message (you can't tell the full story there), moved && and fixed (fatal) error in comment terminator (app didn't start), --> instead of --/>.

Richard, can you get to this quickly please, I'd like to include this in TB 60 ESR build 3 today.

Comment 26

[Richard Marti (:Paenglab)]

Comment on attachment 8994111 [details] [diff] [review]
Patch V.3.1: Fix extended access key functionality (Win/Linux); implement Ctrl+M shortcut for Mac OS which does not have access keys

This works like before. The only thing is, that I can't close the attachment pane with the shortcut. But this is already without the patch. I'm not sure this worked when it was implemented, I know only to test it opened the pane. When closing was implemented I haven't tested it on Mac because I didn't thought it is different on it.

Comment 27

[Jorg K (CEST = GMT+2)]

It's rather unfortunate that the "lovely" key doesn't fully work on Mac. So where from here?

Richard, could you please attach some screenshots. What do you mean by "This works like before"? Before you needed to press Alt+M and now Ctrl+M, no? How does this look in the menu? Why didn't we stay at Alt+M for Mac also? Looking at a Mac keyboard, it appears to have three special keys: Alt, Ctrl and the Apple key.

Can you change
key="&toggleAttachmentPaneCmdMac.key;" modifiers="control" back to
key="&toggleAttachmentPaneCmdMac.key;" modifiers="alt".

Does that work, but using the Alt key?

Comment 28

[Jorg K (CEST = GMT+2)]

OK, talked to Richard on IRC. Mac has no "Alt" key, although I've seen pictures of Apple keyboards with that key. Apparently it's used for "Option" which does something else.

The view menu shows "^M". So nothing else to try here, we just need to see how we can get the bucket closed now :-(

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1475828_attachpane-key.patch (v4)

I've debugged this with Richard over IRC.

I needed to add some code to make the bucket close. The character needs to be lower case "m", or else the user has to press Ctrl+Shift+m to close.

Richard, this works, right?

Thomas, how do I retrieve the "m" from
<!ENTITY toggleAttachmentPaneCmdMac.key "m">
?

Comment 30

[Thomas D. (:thomas8)]

Thanks a lot for debugging MAC which I can't do without a MAC...

Let me make a new patch now

Comment 31

[Thomas D. (:thomas8)]

Attachment: Patch V.4.1: Fix extended access key functionality (Win/Linux); implement Ctrl+M shortcut for Mac OS which does not have access keys

Does this fully work for MAC?

Comment 32

[Jorg K (CEST = GMT+2)]

I sent this to Richard to try previously:

+  if (AppConstants.platform == "macosx") {
+    let commandKey = document.getElementById("key_toggleAttachmentPane").key;
+    if (aEvent.key == commandKey && aEvent.ctrlKey)
+      goDoCommand('cmd_toggleAttachmentPane');
+  } else {

It didn't work. Why would the key be available as element in the DOM?

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 8994360 [details] [diff] [review]
Patch V.4.1: Fix extended access key functionality (Win/Linux); implement Ctrl+M shortcut for Mac OS which does not have access keys

Doesn't work as per the previous comment unless someone made a mistake applying my patch.

Comment 34

[Jorg K (CEST = GMT+2)]

Actually, this is also a new string: <!ENTITY toggleAttachmentPaneCmdMac.key "m">

The old one has been localised quite a bit
https://dxr.mozilla.org/l10n-central/search?q=toggleAttachmentPaneCmd.key&redirect=false
so those localisers won't be happy of we force stuff to a new string for Mac now.

Comment 35

[Jorg K (CEST = GMT+2)]

... to a new key for Mac now. Let alone the alarm bells that will come on when a new string arrives.

Comment 36

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #35)
> ... to a new key for Mac now. Let alone the alarm bells that will come on
> when a new string arrives.

Honestly, I don't want to share in speculations about localizers happiness, and I think we shouldn't be over-anxious on that. If localizers do not follow our explicit localization instructions to make access key == shortcut key, they can hardly blame us if we fix their breakage for millions of users. Also, if I was a localizer, I would certainly be happy and proud to help with improving the product before release, and be understanding towards the need of one or two last-minute corrections. I also imagine that we have systems and tools in place which make localizing easy, more so if it just affects less than a handful of strings...

Technically, we HAVE to force a new string for Mac now because our old design was wrong and having randomly localized shortcut key on MAC (as is currently the case) can cause havoc to MAC shortcuts.

Comment 37

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #35)
> ... to a new key for Mac now. Let alone the alarm bells that will come on
> when a new string arrives.

... also fortunately the alleged "alarm bells" don't come with sirens, so I guess that's just a little red spot, maybe a message saying "2 strings not localized". Nothing harmful nor surprising here, that's what happens when software gets released and errors get discovered shortly before release. Can we please assume that localizers are sympathetic with our product, that's why they volunteer to localize.

Comment 38

[Thomas D. (:thomas8)]

Jörg, FYI, we never had Alt+M on Mac, so far iirc we are sharing the same localized (!!!) key between MAC and Windows but Mac prepends another modifier, Ctrl. Clearly a design mistake because the resulting MAC shortcut is derived from avoiding local access key conflicts under Windows/Linux, so the resulting shortcut in MAC is quite random and might have undesired side effects. It's too late into the night to verify this now, but I think that's what it is.

Comment 39

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere) from comment #36)
Sorry, I don't follow at all. For example Polish has <!ENTITY toggleAttachmentPaneCmd.key "a">.

So when a Mac user hits Ctrl+A, they will trigger that function. If I apply your patch with an unchanged command key entity, then on Mac hitting Ctrl+A will trigger the very some function. Switching from Alt to Ctrl is irrelevant for Mac. It will just keep working the way it's working now. We just add some code to close the bucket.

What will indeed cause havoc is forcing the key to Ctrl+M since that might be taken already.

For non-Mac, the command key will change to the access key of the bucket title, and that key must be available, otherwise we'd start with a wrong access key.

So what we should be doing is:
- For Mac, leave the key alone, switch it from Alt to Ctrl, although that irrelevant.
- For non-Mac, use the bucket access key as command key.
- Make the Mac bucket close.

Comment 40

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #39)
> (In reply to Thomas D. (currently busy elsewhere) from comment #36)
> Sorry, I don't follow at all. For example Polish has <!ENTITY
> toggleAttachmentPaneCmd.key "a">.
> 
> So when a Mac user hits Ctrl+A, they will trigger that function. If I apply
> your patch with an unchanged command key entity, then on Mac hitting Ctrl+A
> will trigger the very some function. Switching from Alt to Ctrl is
> irrelevant for Mac.

Wishful thinking, my dear. You are right that it might still work, but we're eating random system shortcuts in the process. How would you like it if things like Ctrl+Home stopped working for your compositions because it's hijacked by attachment toggle shortcut? That sucks, right? I believe that's exactly what would happen for Polish, assuming that TB conforms with Mac default document shortcuts:

https://support.apple.com/en-us/HT201236
Control-A: Move to the beginning of the line or paragraph.

> It will just keep working the way it's working now. We
> just add some code to close the bucket.
> 
> What will indeed cause havoc is forcing the key to Ctrl+M since that might
> be taken already.

It's unclear from documentation if specifying an accesskey attribute has any effect on Mac; some official XUL documentation says it will just be ignored. Unfortunately, I can't check, but I was working based on that.
If access key has effects on Mac, you are right. Otherwise, you'd be wrong, because then Ctrl+M can't be taken by anything we don't know either from our own shortcut code or as a Mac system shortcut.

> For non-Mac, the command key will change to the access key of the bucket
> title, and that key must be available, otherwise we'd start with a wrong
> access key.

Yes.

> So what we should be doing is:
> - For Mac, leave the key alone, switch it from Alt to Ctrl, although that
> irrelevant.

Not sure, can't test.

> - For non-Mac, use the bucket access key as command key.
> - Make the Mac bucket close.

Yes.

Comment 41

[Thomas D. (:thomas8)]

Attachment: Patch V.4.2: Fix extended access key functionality (Win/Linux); implement Ctrl+M shortcut for Mac OS which does not have access keys

Per my previous comments, I assume we don't want to hijack mac system shortcuts like Ctrl+A.
That's based on the assumption that xul access key attribute is doing nothing for Mac, as documentation claims, but which I can't test.

(In reply to Jorg K (GMT+2) from comment #32)
> I sent this to Richard to try previously:
> 
> +  if (AppConstants.platform == "macosx") {
> +    let commandKey =
> document.getElementById("key_toggleAttachmentPane").key;
> +    if (aEvent.key == commandKey && aEvent.ctrlKey)
> +      goDoCommand('cmd_toggleAttachmentPane');
> +  } else {
> 
> It didn't work. Why would the key be available as element in the DOM?

Why not? Of course <key> elements are available in the DOM. But they don't have a .key property in javascript, my mistake. .getAttribute("key") fixes that problem.

Comment 42

[Richard Marti (:Paenglab)]

Comment on attachment 8994375 [details] [diff] [review]
Patch V.4.2: Fix extended access key functionality (Win/Linux); implement Ctrl+M shortcut for Mac OS which does not have access keys

This works with opening/closing the pane on Mac.

Comment 43

[Jorg K (CEST = GMT+2)]

Comment on attachment 8994375 [details] [diff] [review]
Patch V.4.2: Fix extended access key functionality (Win/Linux); implement Ctrl+M shortcut for Mac OS which does not have access keys

I don't know what you want me to give feedback on. My own code? I added the code to close the bucket on Mac. If
  document.getElementById("key_toggleAttachmentPane").getAttribute("key");
works, fine.

I'm not going to allow new strings for ESR. Let me attach a Polish patch to try for Richard.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 1475828_attachpane-key.patch - "Polish" patch to try

Richard, can you please simulate the Polish locale on Mac.

First experiment:
Do not apply this patch. Only do this
  <!ENTITY toggleAttachmentPaneCmd.key "a">
in messengercompose.dtd.

Now it you press Ctrl+a on a Mac, what happens?

Second experiment:
Try with the patch. It already has the "a" but also all the other code, but not the new string. What happens?

I think the result will be:
- Ctrl+a will not work in TB, it will position to the
  beginning of the line as per https://support.apple.com/en-us/HT201236.
OR
- In experiment 1 the bucket will open, so Ctrl+a working as
  intended and in experiment 2 the bucket should also close.

In either case, we're not doing more damage then there already is, caused by choosing the "a" in the first place.

Comment 45

[Richard Marti (:Paenglab)]

Both don't work. The menu flashes to show the key was pressed but no action on the bucket. Pressing the menu itself is still working.

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: 1475828_attachpane-key.patch - final

OK, you've confirmed that Polish is already broken and we can't break it any further. So this should be the final version.

Last try. This works, right?

Comment 47

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #46)
> Created attachment 8994430 [details] [diff] [review]
> 1475828_attachpane-key.patch - final
> 
> OK, you've confirmed that Polish is already broken and we can't break it any
> further. So this should be the final version.
> 
> Last try. This works, right?

Yes, ready to ship.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/28c699e91513
Ensure extended access key functionality for attachment pane. Implement Ctrl+M shortcut for Mac. r=jorgk, ui-r=Paenglab DONTBUILD

Comment 49

[Jorg K (CEST = GMT+2)]

TB 60 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/1ce90a945683e05a9b487da52fc8acd559ec47d9