Closed Bug 1667692 Opened 4 years ago Closed 3 years ago

Implement keyboard shortcuts to show and focus important address rows (To, CC, BCC): Ctrl+Shift+T, Ctrl+Shift+C, Ctrl+Shift+B (like gmail)

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement

Tracking

(thunderbird_esr78- wontfix)

RESOLVED FIXED
90 Branch
Tracking Status
thunderbird_esr78 - wontfix

People

(Reporter: thomas8, Assigned: aleca)

References

(Blocks 2 open bugs)

Details

(Keywords: access, ux-efficiency)

User Story

Here's a typical enterprise user story (although it has some steps which look wrong):

https://support.mozilla.org/en-US/questions/1304562?page=3#answer-1374489

Attachments

(3 files, 8 obsolete files)

32.87 KB, image/png
Details
25.36 KB, image/png
Details
22.16 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review

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

Proposal (for exploration of feasibility):

Implement the following keyboard shortcuts for easy and efficient cross-localization keyboard access to the three main addressing fields in the composition window:

Ctrl/Cmd+Shift+T --> To
Ctrl/Cmd+Shift+C --> Cc
Ctrl/Cmd+Shift+B --> Bcc

Motivation:

  • Several users have asked for easier ways of disclosing CC and BCC fields, and also the idea of having dedicated keyboard shortcuts for To, CC and BCC has repeatedly come up.

  • To, CC, and BCC are extremely important address fields, likely to be used dozens of times every day by a large number of users, especially in enterprise contexts.

  • Moreover, in the process of composing an email, users might want to go back to the addressing fields to edit/add/remove recipients.

  • Such everyday high-frequency actions should be as efficiently accessible as possible.

  • It speaks volumes that even gmail as a webmailer has deemed it necessary to have dedicated keyboard shortcuts just for disclosing and focusing the three main types of addressing fields.

  • Using the same keyboard shortcuts like gmail will ease the transition for users of gmail webmail into Thunderbird. No need to reinvent the wheel.

  • Also, as real keyboard shortcuts (as opposed to access keys) these will be non-localized in Thunderbird, which is an advantage for users, support and developer maintenance alike.

Implementation

  • Technical implementation should be a non-brainer, as these shortcuts should work from anywhere in the compose window, so probably just <keys>.
  • Making the key combinations available as unique keyboard shortcuts is less trivial, also with a view on future uses. I think it's worth trying, and even reshuffling a bit to make these premium shortcuts work.

Exploring feasibility: Current and future shortcut conflicts and ways out

  • Ctrl+Shift+T is currently available in composition context. However, in the distant future when composition might live in a tab, it will conflict with Ctrl+Shift+T for "Undo close tab". "Undo close tab" is magnitudes less frequent and important for TB than efficient acccess to To-field. We could try to think of something now, or postpone that issue to when it really arises.
  • Ctrl+Shift+C is currently available in composition context. However, in the distant future when composition might live in a tab, it will conflict with Ctrl+Shift+C for "Calendar tab". Calendar is very important, probably more important than efficient access to CC field.
    Unfortunately that would also rob us of an intuitive shortcut for "Copy formatting", a long-requested feature for the compose editor. We could try to think of something now, or postpone that issue to when it really arises.
  • Ctrl+Shift+B is currently used for "Address Book" both in composition and 3-pane contexts. That's important, but I suspect far less frequently used than efficient access to Bcc field. We could try to find another shortcut for "Address Book", but the most intuitive alternatives like Ctrl+Shift+A are also taken in both contexts. Shuffling shortcuts is an art.

Another way out: The shortcut panel approach

Trying to think out of the box, Alex' plans for a location toolbar in Thunderbird springs to mind. If we had a single entry point access/shortcut key for focusing that (or some sort of master shortcut panel in the middle of the screen), things like Calendar and Address book could subsequently be covered with single letter (on-screen) shortcuts from that toolbar/pane context, which could free up their current shortcuts. Reversely, we could also try the same for the shortcuts proposed here, to have a central access panel on one shortcut, and then one-letter shortcuts to pick one from the panel context.

Summary: Implement keyboard shortcuts to show and focus important address rows (To, CC, BCC): Ctrl+Shift+T, Ctrl+Shift+C, Ctrl+Shift+B (like gmail) → Explore implementing keyboard shortcuts to show and focus important address rows (To, CC, BCC): Ctrl+Shift+T, Ctrl+Shift+C, Ctrl+Shift+B (like gmail)
See Also: → 1666847
See Also: → 1667759
See Also: → 1616514

Aleca, any comments/insights/ideas?

This covers a high-relevance subset of your Bug 1683223 - Implement keyboard shortcuts offering quick access to primary addressing fields.

Flags: needinfo?(alessandro)
See Also: → 1683223

Indeed, this comes from the same motivation behind Bug 1683223.
In that bug I suggested to use the combination of CTRL + ALT instead of SHIFT to prevent possible conflicts with other keys.

Magnus doesn't consider this to be trivial, but I think we should definitely implement shortcuts to give quick access to important areas of every section, without exclusively relying on the TAB focus, which works in a list context but is not an optimal solution when outside a linear interaction pattern.

Do you want me to mark my bug as duplicate of this and continue the conversation here, and cover all the other fields?

Flags: needinfo?(alessandro)

(In reply to Alessandro Castellani (:aleca) from comment #2)

Indeed, this comes from the same motivation behind Bug 1683223.
In that bug I suggested to use the combination of CTRL + ALT instead of SHIFT to prevent possible conflicts with other keys.

I've commented there why I don't think that's feasible (Bug 1683223 Comment 2).

Magnus doesn't consider this to be trivial, but I think we should definitely implement shortcuts to give quick access to important areas of every section, without exclusively relying on the TAB focus, which works in a list context but is not an optimal solution when outside a linear interaction pattern.

True.

Do you want me to mark my bug as duplicate of this and continue the conversation here, and cover all the other fields?

I'd prefer to keep the focus of this bug on exploring the feasibility of the particular implementation proposed (gmail shortcuts), which does not involve covering all the other fields. I think we'll have a hard time assigning non-localized shortcuts to all of these things, and I'm not even sure if we really want that. For some of them, current localized access keys might be quite sufficient and intuitive. A perfectly consistent solution may not be possible, and would certainly come with other disadvantages (e.g. losing the localization advantage).

User Story: (updated)

We should really try if we can shuffle shortcuts around so that we can implement these. From many user reports, not having efficient keyboard access to CC / BCC field is a nasty deal-breaker for their daily routines, especially in the enterprise.

I agree, this is something we shouldn't dismiss as "non important".
I'll work on it!

Assignee: nobody → alessandro
Attached patch 1667692-compose-shortcuts.diff (obsolete) — Splinter Review

This patch implements the following shortcuts:

Ctrl/Cmd+Shift+T --> To
Ctrl/Cmd+Shift+C --> Cc
Ctrl/Cmd+Shift+B --> Bcc

I removed the Ctrl+Shift+B shortcut to open the Address Book from the Composer window as I don't think that's a trivial accessibility entry point and not as common or as important as being able to quickly enable the Bcc field.
If we want to keep a shortcut for it, we could replace it with the letter "K".

All shortcuts are defined via fluent, allowing translators to adapt them.
Also, these shortcuts are defined via JS with a simple keypress eventListener, allowing more flexibility in the future if we will ever consider adding a customizable shortcut JSON Map.

The shortcuts are also visible via tooltip in the recipients labels, making it easily discoverable.

Attachment #9196973 - Flags: review?(mkmelin+mozilla)
Attachment #9196973 - Flags: review?(bugzilla2007)
Status: NEW → ASSIGNED
Comment on attachment 9196973 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +68,5 @@
>      .label = Receipt
>      .tooltiptext = Request a return receipt for this message
> +
> +
> +to-compose-show-addressing-field-key = T

To is "always" showing, so no point wasting a hotkey on it.

@@ +71,5 @@
> +
> +to-compose-show-addressing-field-key = T
> +to-compose-address-label =
> +    .value = To
> +    .tooltiptext = { PLATFORM() ->

I could convert the compose window to top level <html> soon, then we should use title instead. The xul:label should be something else. I'm not sure if this is a button or what.

Hmm, but now that I look at this in detail, I see Alt+Ś is subject. Maybe we could just use Alt+T for to and Alt+C for cc? That is, use the accesskeys as they are + trigger opening it perhaps?
Attachment #9196973 - Flags: review?(mkmelin+mozilla)

To is "always" showing, so no point wasting a hotkey on it.

The To field is closed if the user starts sending an email from a Newsgroup account.
It's also good to have it to allow quick focus to that field.

I could convert the compose window to top level <html> soon, then we should use title instead. The xul:label should be something else. I'm not sure if this is a button or what.

Right on! I was actually thinking about this.
Those labels can easily be converted into buttons.
I can do it right away so we can inherit the benefits of the title tooltips after the window is converted to top level HTML.

Hmm, but now that I look at this in detail, I see Alt+Ś is subject. Maybe we could just use Alt+T for to and Alt+C for cc? That is, use the accesskeys as they are + trigger opening it perhaps?

Mh, I'm not sure about this.
I wouldn't mind using those accesskeys as accelerators to trigger opening/focus, but that means removing the duplicates (eg. the Tools menu uses the Alt+T combination).
I'm okay with giving higher priority to the recipient fields.

Thomas, what do you think?

Flags: needinfo?(bugzilla2007)
Comment on attachment 9196973 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

This is AWESOME!!! - why did we take so long to make this happen??
I have one nit.

::: mail/components/compose/content/messengercompose.xhtml
@@ -508,5 @@
>    <key id="key_mail"  key="&messengerCmd.commandkey;" oncommand="toMessengerWindow();" modifiers="accel"/>
> -  <key id="key_addressbook"
> -       key="&addressBookCmd.key;"
> -       modifiers="accel, shift"
> -       oncommand="toAddressBook();"/>

Yeah, let's give that a shot. Maybe nobody will miss it. If they do, we can still adjust. Going to main AB isn't very useful in the particular context of composition, it's just that so far it was a global shortcut - but we've decided that we can't keep that because the BCC shortcut (compatibel with gmail) matters more. Fair enough.

There's an error in console when opening Tools menu.
You also need to remove this line:
https://searchfox.org/comm-central/source/mail/components/compose/content/messengercompose.xhtml#1825
`                      key="key_addressbook"`
Attachment #9196973 - Flags: review?(bugzilla2007) → review+

(In reply to Alessandro Castellani (:aleca) from comment #9)

To is "always" showing, so no point wasting a hotkey on it.

The To field is closed if the user starts sending an email from a Newsgroup account.
It's also good to have it to allow quick focus to that field.

+1. And gmail parity.

Hmm, but now that I look at this in detail, I see Alt+Ś is subject. Maybe we could just use Alt+T for to and Alt+C for cc? That is, use the accesskeys as they are + trigger opening it perhaps?

Mh, I'm not sure about this.
I wouldn't mind using those accesskeys as accelerators to trigger opening/focus, but that means removing the duplicates (eg. the Tools menu uses the Alt+T combination). I'm okay with giving higher priority to the recipient fields.
Thomas, what do you think?

I think your patch as-is is just perfect. Alt+* combos would come with several issues:

  • We explicitly want to match the gmail shortcuts, for an easy transition to Thunderbird.
  • Having internationally identical shortcut keys is great for us and enterprise when it comes to support. (Note: Ctrl+Shift+* are localizable only in theory, we never want anyone to localize this because that will end up breaking all of our international shortcut keys.)
  • We don't want to depend on access key localization for these high-value shortcuts: localization can easily break things, access keys are all over (contacts side bar, attachment reminder bar, etc.), so it's actually quite hard to get that right in every language, and from experience, many localizers won't get this right.
  • As Alex said, let's avoid breaking the main menu access keys as long as we can.
  • [Edit:] Ctrl+Shift+T/C/B are also convenient to use because both modifiers are on the outer left edge on the keyboard, allowing a very controlled double-handed usage without twisting your hands or risk of errors.
Flags: needinfo?(bugzilla2007)

I think we should also start considering a more strict approach for those accesskey as we've been adding them everywhere.
It think Alt+... should be strictly used for Menu Bar items.
For UI elements, dedicated shortcuts with a consistent modifier should be used and showed in the tooltips.
With that we can also remove the ugly underscore letter in the UI, which should only be used in the menu items.

All:

As a user, I have been following this thread and the associated threads with much interest. I just want to say thank you for your efforts; they are most appreciated.

Mark Hepburn

Re Alt+, that is how accesskey works. People rely on knowing Alt+ what's showing will get them there. It's not only about menus.

(In reply to Mark Hepburn from comment #13)

As a user, I have been following this thread and the associated threads with much interest. I just want to say thank you for your efforts; they are most appreciated.

Thank you so much!

Shortcut and access keys are not localizable only in theory. They can, and will be different per locale.

(In reply to Magnus Melin [:mkmelin] from comment #16)

Shortcut and access keys are not localizable only in theory. They can, and will be different per locale.

I don't think that's true in reality.

  • Shortcut keys should be the same for all locales.
  • Only access keys are localized, exactly for that reason that they require underlined characters from the localized UI strings.

I have never heard of localized shortcut keys in Thunderbird. Can you provide an example?
Why would any localizer go through the hassle of having to change the whole set of shortcut keys when the original set is just fine and safe and also easier for support? Because as soon as you change one, you break many others, as they are virtually all taken.

There's zero difference between the Thunderbird shortcut keys of English and German localization:
https://support.mozilla.org/en-US/kb/keyboard-shortcuts#w_list-of-keyboard-shortcuts
https://support.mozilla.org/de/kb/Tastaturkuerzel_Thunderbird#w_liste-der-tastenkombinationen

Same for Firefox:
https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly
https://support.mozilla.org/de/kb/Tastaturkuerzel

And here's the official reason (I'm sure there's best practices somewhere on MDN...):
https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts

Because access keys are locale-dependent, they're not documented in this page. [From which it follows that all the shortcut keys listed are international, TD].

(In reply to Alessandro Castellani (:aleca) from comment #12)

I think we should also start considering a more strict approach for those accesskey as we've been adding them everywhere.
It think Alt+... should be strictly used for Menu Bar items.

Alt+* only for menu bar items might be the automatical result of moving our code to HTML, where using the access key attribute effects Alt+Shift+* access keys by default (on Windows). We're currently in an unannounced phase of transition, with a bit of chaos and confusion. Executive summary: https://bugzilla.mozilla.org/show_bug.cgi?id=1661679#c6

For UI elements, dedicated shortcuts with a consistent modifier should be used and showed in the tooltips.

Anything which has a permanently visible string in the UI should preferably get an access key, not a shortcut key.
Exceptions may confirm the rule. You'll run out of shortcut keys very fast if you also give them to UI elements with a visible string.
Sometimes maybe there's a bit of room for rethinking this (as the current patch does, sort of), but trust me, that room will be very small.

With that we can also remove the ugly underscore letter in the UI, which should only be used in the menu items.

The ugly underscore is actually extremely helpful for efficient navigation. I'd much prefer a little ugly over less efficient, more so that one of the main reasons for using Thunderbird is ux-efficiency. I don't find the modern alternative, showing access key tooltips only when you press ALT key, all that attractive, but ymmv. (Try pressing Alt key in Windows explorer or probably any modern MS Office app to see that in action).

Anyway, we're digressing. I suggest to get this bug done first before we go into general design discussions... ;-)

(In reply to Magnus Melin [:mkmelin] from comment #14)

Re Alt+, that is how accesskey works. People rely on knowing Alt+ what's showing will get them there. It's not only about menus.

+1, with the transitional details of my comment 18.

So, what should we do with this patch?

Magnus' proposal: Use Alt+* for the shortcuts to keep it consistent with the accesskeys.
Thomas' proposal: Use Ctrl+Shift+* for the shortcuts to emulate the Gmail implementation.

For what it's worth, I would rather have Alt, it is just logical.

re: I removed the Ctrl+Shift+B shortcut to open the Address Book from the Composer window as I don't think that's a trivial accessibility entry point and not as common or as important as being able to quickly enable the Bcc field.
If we want to keep a shortcut for it, we could replace it with the letter "K".

Ctrl+Shift+K sounds a good idea because 'Alt K' set focus on address book in contacts sidebar. It would bring the two into alignment.

In the Write composer window :
Alt K is used to select/get focus on the Contact Sidebar 'address book' selection
Alt N is used to set focus in the Search Contacts.
Alt R sets focus on the 'FROM'
It would be reasonable and logical to assume:
Alt C selects Cc
Alt B selects Bcc

when needing to select where to move pills eg: move Cc to Bcc- on highlighted pill - press right click key (or Shift f10) to open drop down menu and press B
This works very well and also keeps a consistency with the association of B to Bcc.

'Ctrl' and 'Shift' is always something I associate with an external focus such as saving a file or sending an email, copy/paste, not internal window focus on items for selection.
Used together - well it also requires additional multikey selection and personally, I'd rather have much less of that. Those types of multikey are best left to selections that are more important if acted upon and so are made deliberately less likely to press accidentally.

(In reply to Alessandro Castellani (:aleca) from comment #20)

So, what should we do with this patch?

Magnus' proposal: Use Alt+* for the shortcuts to keep it consistent with the accesskeys.

Alt+* = Access keys, which forces these "shortcuts" to be localized:

  • they will end up different on every localization (imo, that's a disadvantage here)
  • which is a nightmare for support and documentation, and not helpful for enterprise deployment.
  • localization can break these keys easily (e.g. by not translating them, or overlooking duplicates)
  • choose between breaking main menu access keys/other access keys OR using unintuitive access keys here
  • furthermore, I would not introduce more important Alt+* access keys at this time as we might have to disbandon those if we switch to browser's Alt+Shift+* access keys.
  • less convenient on the keyboard
  • transition from gmail to TB made harder

Thomas' proposal: Use Ctrl+Shift+* for the shortcuts to emulate the Gmail implementation.

Ctrl+Shift+* = Shortcut keys. That will de facto (by Mozilla tradition) make these "shortcuts" internationally the same (typically not localized):

  • easy for support and documentation, and helpful for enterprise deployment.
  • localization typically won't break these as they won't touch them
  • cannot break main menu access keys / other access keys or vice versa (as they are under developers' control)
  • reasonably intuitive internationally
  • does not affect localized access key design
  • relatively convenient to use because both modifiers are on the outer left edge on the keyboard, allowing a very controlled double-handed usage without twisting your hands or risk of errors.
  • transition from gmail to TB made easy
See Also: → 1687117

(In reply to Thomas D. (:thomas8) from comment #22)

(In reply to Alessandro Castellani (:aleca) from comment #20)

So, what should we do with this patch?

Magnus' proposal: Use Alt+* for the shortcuts to keep it consistent with the accesskeys.

Alt+* = Access keys, which forces these "shortcuts" to be localized:

  • they will end up different on every localization (imo, that's a disadvantage here)
  • which is a nightmare for support and documentation, and not helpful for enterprise deployment.
  • localization can break these keys easily (e.g. by not translating them, or overlooking duplicates)

eg: MAC uses different words/keys etc anyway. eg: Alt/Option, Options/Preferences, and Cmd for most keyboard shortcuts.
I do not see how one shoe will ever fit all, but at least the differences are known and well documented.

  • choose between breaking main menu access keys/other access keys OR using unintuitive access keys here

Not sure exactly which 'main access keys' you are refering to. I understand there is only one - opening of Address Book.
REplacement of Ctrl+Shift+K sounds a good idea because 'Alt K' already sets focus on address book in contacts sidebar. It would bring the two into alignment and make more intuitive. I see that as an improvement and ideally should be changed regardless. It also resolves the issue for using 'Alt' + B.

  • OR using unintuitive access keys here.

Nothing is more non intuitive than suggesting 'Ctrl+Shift'. It's only intuitive if you use gmail and specifically on From, Cc. Bcc.
We should not be concerned with gmail.

Thunderbird uses the 'Alt + letter throughout the focus selection in the Write window.
Alt K is used to select/get focus on the Contact Sidebar 'address book' selection
Alt N is used to set focus in the Search Contacts.
Alt R sets focus on the 'FROM'
Alt C selects Cc ...............how is this not intuitive ?
Alt B selects Bcc

  • furthermore, I would not introduce more important Alt+* access keys at this time as we might have to disbandon those if we switch to browser's Alt+Shift+* access keys.

Am I missing something here ? Why would anyone consider a switch to 'Alt+Shift', Thunderbird is not a browser and Why would a browser even consider to use 'Alt+Shift' as it is not a convenient double key action. That combination would be something rarely used as it is plain awkward.

  • less convenient on the keyboard

Pressing just 'Alt' is not inconvenient. eg: 'Alt' + 'R' which selects 'From'.
It is much easier to only have to press 'one' key with one hand. Increasing the numbers of keys you need to press is not an improvement. You cannot assume everyone has ease of dexterity and not all keyboards have the Ctrl key in bottom left corner.

  • transition from gmail to TB made harder

I'm more inclined to say the focus should be on loads of long time users of Thunderbird who would be seriously not amused to have to learn new habits.
Muscle memory is not that simple to adapt and changes that force this upon users are likely to get an unappreciated response.
There are already many complaints regarding the new 'Write' compose window and accessibility using keyboard. People in general do not like change and they are more inclined to be less positive when it comes to retraining after decades of status quo.
'Alt + F' selects From - which users are already using, it is a natural progression and intuitive for the adjacent buttons to be 'Alt +C' and 'Alt + B' with the C and B underlined.
Retraining all the valuable long term Thunderbird users because some users change from gmail (gmail users expect a change) seems incorrect.

  • transition from gmail to TB made easy

I do not see this 'potential gmail customer' as a factor that should influence anything.
I believe we should be thinking more about Thunderbird's current users who do not appreciate needing to learn new habits that interfere with work throughput and are already trying to get to grips with version 78* and not thinking about potential gmail customers who would expect a change in learning when changing to a different product. After all they chose to stop using gmail.

(In reply to Anje from comment #23)

Thunderbird uses the 'Alt + letter throughout the focus selection in the Write window.
Alt K is used to select/get focus on the Contact Sidebar 'address book' selection
Alt N is used to set focus in the Search Contacts.
Alt R sets focus on the 'FROM'
Alt C selects Cc ...............how is this not intuitive ?
Alt B selects Bcc

All of these are localized access keys, so they will only apply to en-US localization. You'll be first person to suffer when supporting users because you will not be able to guess the localized equivalent of Alt+C for Finnish where it might end up as Alt+N for "CC" in Finnish. Having international shortcuts looks like a massive advantage to me wrt support and enterprise deployment, plus easy migration from gmail to TB.

  • furthermore, I would not introduce more important Alt+* access keys at this time as we might have to disbandon those if we switch to browser's Alt+Shift+* access keys.

Am I missing something here ? Why would anyone consider a switch to 'Alt+Shift', Thunderbird is not a browser and Why would a browser even consider to use 'Alt+Shift' as it is not a convenient double key action. That combination would be something rarely used as it is plain awkward.

I agree that Alt+Shift+* is pretty akward. We're currently on auto-pilot wrt access keys. Check Preferences/options where Alt+Shift+* is used for access keys, just because this happens to be a Web page in a browser, and the Web standard has prescribed Alt+Shift+* for access keys (God knows why). We're changing our code to run on browser, so we need to take design decisions on this. On the upside, if we would use Alt+Shift+* as in-app access keys, they would be more distinct from Alt+* main menu access keys (i.e. we'd have a larger set of in-app access keys available).

  • less convenient on the keyboard

Pressing just 'Alt' is not inconvenient. eg: 'Alt' + 'R' which selects 'From'.
It is much easier to only have to press 'one' key with one hand.

Alt+B, Alt+T and even Alt+C with one hand are pretty odd.

Increasing the numbers of keys you need to press is not an improvement. You cannot assume everyone has ease of dexterity and not all keyboards have the Ctrl key in bottom left corner.

I thought most desktop keyboards do...

  • transition from gmail to TB made harder

I'm more inclined to say the focus should be on loads of long time users of Thunderbird who would be seriously not amused to have to learn new habits.

I'm not sure we can speak of habit interference here because so far, To/CC/Bcc have never been directly keyboard-accessible.
So all of these shortcuts are new.

Muscle memory is not that simple to adapt and changes that force this upon users are likely to get an unappreciated response.

Dito.

There are already many complaints regarding the new 'Write' compose window and accessibility using keyboard. People in general do not like change and they are more inclined to be less positive when it comes to retraining after decades of status quo.
'Alt + F' selects From - which users are already using, it is a natural progression and intuitive for the adjacent buttons to be 'Alt +C' and 'Alt + B' with the C and B underlined.
Retraining all the valuable long term Thunderbird users because some users change from gmail (gmail users expect a change) seems incorrect.

I'm failing to see the retraining as we've never had shortcuts for these fields. We won't indicate access keys on the labels, so it's not like anyone could expect to get CC from Alt+C - we're just adding dedicated shortcuts here because Alt+* access keys are a limited resource.
What you are forgetting is the main menu bar on top which usually requires a lot of Alt+* access keys already. If we force duplicate access keys inside the app, we're making main menu access harder (users would have to press and let go Alt first, then the menu access key). We can opt to do that, but so far this has never been discussed.

  • transition from gmail to TB made easy

I do not see this 'potential gmail customer' as a factor that should influence anything.
I believe we should be thinking more about Thunderbird's current users who do not appreciate needing to learn new habits that interfere with work throughput and are already trying to get to grips with version 78* and not thinking about potential gmail customers who would expect a change in learning when changing to a different product. After all they chose to stop using gmail.

No, you're misunderstanding the situation. WE need to convince gmail users that changing to Thunderbird is something which is easy enough for them to take the plunge. A lot of young people have never heard of email clients as all they know is webmail, whatsapp, and facebook.

'Alt + R' selects From - which users are already using, it is a natural progression and intuitive for the adjacent buttons to be 'Alt +C' and 'Alt + B' with the C and B underlined.

The reasoning is now that those new selections are adcacent due to new design and Alt+R is currently used for the first item, it would be easy and intuitive to use Alt + C and Alt + B.
Although 'Alt' + 'C' is already used for opening 'Attachment Pane' - I always wondered why it was not 'Alt M' - mainly because the 'm' is underlined in Attachment Pane option via 'Menu Bar', 'Alt V' then 'M' opens it, but not via menu use 'Alt C', so I suppose that is something that would need modification.

I can see how the 'localisation' may be an issue, but in Support Forum I've not come across it so far, usually it's variants for OS eg: Mac. Maybe that's because I'm helping in the English variant.

So would you be saying to alter the current selection for 'From' from 'Alt'+'R' to 'Ctrl'+'Shift'+'R', so that it is intuitive and logical for the adjacent items to be 'Ctrl'+'Shift'+'C' and 'Ctrl'+'Shift'+'B' ?
Note:
'Ctrl'+'Shift'+'R' is already used to 'Remove main anchors' in Write window, so this could present an issue.
'Ctrl'+'Shift'+'C' is already used within the main menu for Calendar. Although obviously that would not operate with focus in the 'Write' window. Although I've often wondered why I can open the 'Address Book' so easily, but cannot do the same with the Calendar, but that is my arguement for liking to open things in windows and not tabs.
So, currently would not be a problem assuming there is no intent on opening the Calendar whilst composing an email; it was an option I would have liked to have seen added. Lets' hope no one decides to put the 'Write' window or 'Address Book' in a tab. Tabs are not always convenient and could mess up having identical key combos that provide two different actions.

'Ctrl'+'Shift'+'B' is currently used to open 'Address Book' - although it may seem logical to chnage to 'Ctrl'+'Shift'+'K' to be similar to address book selection in Contacts Sidebar.

I'm failing to see the retraining as we've never had shortcuts for these fields

But as described above, we have used those key combos for current selections, so retraining is not so much on the new keys as learning those current key combo functions will be different key combos and what the user is used to keying will do something they do not expect.
eg: expect to 'Remove main anchors' or open Address Book

See Also: → 1686379
Type: defect → enhancement
Blocks: 1687432

Alex and I would like to create a consistent keyboard accelerator system for the addressing area, so we're trying to keep this in sync with:
Bug 1687432 - Provide no-brainer keyboard shortcut to easily move selected recipients between To/CC/BCC

Keyboard shortcuts Open/Focus ... Move selected recipient items to...
... To: Ctrl+Shift+T T (long keypress)
... Cc: Ctrl+Shift+C C (long keypress)
... Bcc: Ctrl+Shift+B B (long keypress)

After thinking about this and exploring various approaches, I think the original proposal from Thomas is the best approach to move forward.

Using accesskeys for those fields is not recommended because:

  • They can be localized, making our documentation and support hard to handle.
  • They can conflict with other accesskeys in the page (Attachment reminder, Contact Sidebar).
  • They don't offer the flexibility we have when handling keystrokes in JavaScript if we need to handle edge cases.

Also, the Addressing Widget is a self containerized Custom Element with its own event listeners and behaviours, which are drastically different from a simple input field like the Subject.

I also found out that other webmail clients (Fastmail, Protonmail, Outlook web) use the same (like Fastmail) or very similar Ctrl+Shift+* combination to handle actions in the compose area.

In terms of discoverability, those shortcuts can be listed in the title or tooltiptext, making it discoverable for mouse users and screen readers.
For keyboard users, we can add those in the Menu Bar, underneath the View menu, with a text like Show and focus Cc with the acceltext listed to the right side.

I'll update the patch.

Attached patch 1667692-compose-shortcuts.diff (obsolete) — Splinter Review

Patch updated implementing also the menu items underneath the View menu.
Those items are properly disabled in case the field is already visible.

I didn't add the eventListener for those shortcuts in the mail-recipients-area CE since that listener covers the entire document, and as recommended, a CE shouldn't leak or look for things outside itself.

I think this is a great usability improvement of the whole area.
I'm not sure if we should uplift this to 78 (if approved) as it comes with new strings.

Attachment #9196973 - Attachment is obsolete: true
Attachment #9199659 - Flags: review?(mkmelin+mozilla)
Attachment #9199659 - Flags: review?(bugzilla2007)

After thinking about this and exploring various approaches, I think the original proposal from Thomas is the best approach to move forward.

Using accesskeys for those fields is not recommended because:

  • They can be localized, making our documentation and support hard to handle.

I agree. Thomas has put a lot of thought into coming up with a good solution. (In reply to Alessandro Castellani (:aleca) from comment #27)

Comment on attachment 9199659 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

This probably works well as designed, thank you for improving the implementation and exposing the shortcuts on the menu!
Are we able to force-display the tooltip when the disclosure buttons get focus via keyboard?

This needs some improvements in the next iteration.
- Wrt localization, I think we should not hardcode pretty shortcut keys all over the place, which is inherently unsafe, unnecessarily complex, and not scalable. We can radically simplify this with a modular approach to the shortcut key localization, so that repeated parts get referenced instead of repeated.
- Let's stick to our generic naming patterns for event handlers: someElementOnCommand().
- Some other nits.

Caveat: When copying code from my review, please watch out for errand tab characters which unexpectedly got created when copying code during splinter review. Sorry!

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4070,5 @@
> +  ]);
> +
> +  document.addEventListener("keypress", event => {
> +    // Interrupt if we don't have the correct combination of CTRL/CMD + SHIFT.
> +    if (!(event.ctrlKey || event.metaKey) && !event.shiftKey) {

Something's wrong here, as this won't work as expected when Ctrl or Meta are pressed, but Shift is not, or vice versa.
I think you want:
    if (!(event.ctrlKey || event.metaKey) || !event.shiftKey) {

@@ +4076,5 @@
> +    }
> +
> +    switch (event.key) {
> +      case toKey:
> +        showAndFocusOnAddressRow("addr_to", "addressRowTo");

Remove "On" please, not needed.

@@ +4079,5 @@
> +      case toKey:
> +        showAndFocusOnAddressRow("addr_to", "addressRowTo");
> +        break;
> +      case ccKey:
> +        showAndFocusOnAddressRow("addr_cc", "addressRowCc");

dito

@@ +4082,5 @@
> +      case ccKey:
> +        showAndFocusOnAddressRow("addr_cc", "addressRowCc");
> +        break;
> +      case bccKey:
> +        showAndFocusOnAddressRow("addr_bcc", "addressRowBcc");

dito

@@ +4095,5 @@
> + *
> + * @param {string} labelID - The ID of the label to hide.
> + * @param {string} rowID - The ID of the addressing container to reveal.
> + */
> +function showAndFocusOnAddressRow(labelID, rowID) {

Do we need this helper function? It seems to add nothing. Instead, let's do this:
- rename the function in mailWidgets.js to showAndFocusAddressRow(...)
- ensure adjusting all callers
- just call the mailwidget function directly from the listener
- adjust the function comment of showAndFocusAddressRow() in mailWidgets.js

https://searchfox.org/comm-central/rev/d1670223c14ee0d3e7f869383f9f138aad09e070/mail/components/compose/content/addressingWidgetOverlay.js#1052-1070

>  * Show the container row of an hidden recipient (Cc, Bcc, etc.).
 * Show and/or focus the address row of a given recipient type (Cc, Bcc, etc.).

Also, please replace the variable 'container' with 'addressRow' (container could be anything), also in the function comment.
> * @param {string} rowID - The ID of the container to reveal.

 * @param {string} rowID - The ID of the address row to reveal.

>  let container = document.getElementById(rowID);

  let addressRow = document.getElementById(rowID);
  [etc.]

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +1380,5 @@
> +    "menu_showCcField"
> +  ).disabled = document.getElementById("addr_cc").collapsed;
> +  document.getElementById(
> +    "menu_showBccField"
> +  ).disabled = document.getElementById("addr_bcc").collapsed;

Smart!

::: mail/components/compose/content/messengercompose.xhtml
@@ -508,5 @@
>    <key id="key_mail"  key="&messengerCmd.commandkey;" oncommand="toMessengerWindow();" modifiers="accel"/>
> -  <key id="key_addressbook"
> -       key="&addressBookCmd.key;"
> -       modifiers="accel, shift"
> -       oncommand="toAddressBook();"/>

Bye, bye, AB shortcut in composition! Let's hope folks won't cry for you... But hey, no pain, no gain!

@@ +1084,5 @@
>              </menupopup>
>            </menu>
> +          <menuseparator id="viewMenuBeforeSecurityStatusSeparator"/>
> +          <menuitem id="menu_showToField"
> +                    data-l10n-id="to-compose-field-menuitem"

*****
Please adjust all data-l10n-id's in this file according to the proposed changes in messengercompose.ftl.
*****

@@ +1086,5 @@
> +          <menuseparator id="viewMenuBeforeSecurityStatusSeparator"/>
> +          <menuitem id="menu_showToField"
> +                    data-l10n-id="to-compose-field-menuitem"
> +                    data-l10n-attrs="acceltext"
> +                    oncommand="showAndFocusOnAddressRow('addr_to', 'addressRowTo')"/>

For code clarity and consistency, here's where we *do* want a dedicated event handling function (the equivalent of that is the keypress listener's event handler, which happens to be anonymous, so both are calling the same target function):

function menuShowAndFocusFieldOnCommand(label, rowID) {
  showAddressRow(label, rowID);
}

@@ +1090,5 @@
> +                    oncommand="showAndFocusOnAddressRow('addr_to', 'addressRowTo')"/>
> +          <menuitem id="menu_showCcField"
> +                    data-l10n-id="cc-compose-field-menuitem"
> +                    data-l10n-attrs="acceltext"
> +                    oncommand="showAndFocusOnAddressRow('addr_cc', 'addressRowCc')"/>

menuShowAndFocusFieldOnCommand

@@ +1094,5 @@
> +                    oncommand="showAndFocusOnAddressRow('addr_cc', 'addressRowCc')"/>
> +          <menuitem id="menu_showBccField"
> +                    data-l10n-id="bcc-compose-field-menuitem"
> +                    data-l10n-attrs="acceltext"
> +                    oncommand="showAndFocusOnAddressRow('addr_bcc', 'addressRowBcc')"/>

dito

@@ -2163,5 @@
>                  <hbox class="addressingWidgetItem" flex="1">
>                    <hbox id="addressingWidgetLabels" class="address-extra-recipients"
>                          flex="1" align="center">
>                      <hbox id="addressingWidgetSwappableLabels">
> -                      <label id="addr_to" value="&toAddr2.label;" role="button"

You also need to remove toAddr2.label and friends from messengercompose.dtd, and adjust the other consumers of that accordingly. We really want to force the top labels in extraRecipients and the labels on the rows to stay in sync, which would not be guaranteed with the current iteration. And unfortunately, I think we should reverse the logic so that the real row labels become the primary set, and the top disclosure labels duplicate that (reason: row labels have more space limitations than top labels, so translators should consciously adjust the row labels and we will then duplicate that into the disclosure labels).

https://searchfox.org/comm-central/rev/dc9a0af22809919fcdcbd3a49344bd3f5e2b9d4e/mail/components/compose/content/messengercompose.xhtml#2259-2260

                    <label id="toAddrLabel" value="&toAddr2.label;"
                           control="toAddrInput"/>

These should become:

                    <label id="toAddrLabel"
                               data-l10n-id = "to-address-row-label"                               
                               control="toAddrInput"/>

CC and BCC below by analogy.
For details how the Fluent stuff should be constructed, please refer to my comments on messengercompose.ftl

@@ +2173,5 @@
>                    <hbox id="addressingWidgetLabels" class="address-extra-recipients"
>                          flex="1" align="center">
>                      <hbox id="addressingWidgetSwappableLabels">
> +                      <label id="addr_to" role="button"
> +                             data-l10n-id="to-compose-address-label"

Adjust here and below  according to changes in the fluent file...

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +67,5 @@
>  button-return-receipt =
>      .label = Receipt
>      .tooltiptext = Request a return receipt for this message
> +
> +# Addressing fields

# Addressing Area

First thing here, please add entries for the real address row labels.

# The labels in front of each address row, also used for disclosure buttons.
to-address-row-label = To
cc-address-row-label = Cc
bcc-address-row-label = Bcc

@@ +69,5 @@
>      .tooltiptext = Request a return receipt for this message
> +
> +# Addressing fields
> +
> +#   Internationl accelerators. Do not translate.

Spelling: International

@@ +70,5 @@
> +
> +# Addressing fields
> +
> +#   Internationl accelerators. Do not translate.
> +to-compose-show-addressing-field-key = T

This file is called messengercompose.ftl, so it's all about composing, so I think we can drop the -compose- hint and shorten the rest.

to-show-address-row-key = T
(and by analogy for the next two below)

@@ +72,5 @@
> +
> +#   Internationl accelerators. Do not translate.
> +to-compose-show-addressing-field-key = T
> +cc-compose-show-addressing-field-key = C
> +bcc-compose-show-addressing-field-key = B

dito for these two (cc and bcc)

@@ +75,5 @@
> +cc-compose-show-addressing-field-key = C
> +bcc-compose-show-addressing-field-key = B
> +
> +to-compose-address-label =
> +    .value = To

To force consistency, these are the secondary top labels, so they should duplicate the primary row labels which you still need to create in this file (see above), as I explained in my comments on messenger.dtd.

to-show-address-row-label =
    .value = { to-address-row-label }

@@ +78,5 @@
> +to-compose-address-label =
> +    .value = To
> +    .tooltiptext = { PLATFORM() ->
> +        [macos] Show To field (⌘ ⇧ { to-compose-show-addressing-field-key })
> +        *[other] Show To field (Ctrl+Shift+{ to-compose-show-addressing-field-key })

Do we want to give localizers that freedom to use potentially different field names in the tooltip?
Probably not. I think we should force this right, similar to what we did for "Remove the ... field" tooltip:
https://searchfox.org/comm-central/rev/dc9a0af22809919fcdcbd3a49344bd3f5e2b9d4e/mail/locales/en-US/messenger/messengercompose/messengercompose.ftl#9
remove-address-row-type-label =
    .tooltiptext = Remove the { $type } field

So here it's a bit easier, we don't have to loop through code, but can just set things here for this limited set (see below).

More importantly, as we add more keyboard shortcuts for display: We certainly don't want dozens of manual translations of modifier keys which are always the same. I don't think we want to expose the entire shortcut string to localization errors and partly hardcoded into unrelated translations. Does Firefox have best practices for this? This type of thing used to be done via toolkit's ShortcutUtils.jsm with functions like getPrettyKey() for <key> elements - but I'm not sure if they have something for the Fluent way of things (maybe we can use some of their helper functions?).

If not, until we have such universal solution, I suggest that we use a much more modular system at least, to prevent unnecessary complexity and localization errors in the shortcuts:

* Create one dedicated entry for the combined modifier, and do the platform thing there:
(whitespace trick: https://www.projectfluent.org/fluent/guide/special.html)

# ctrl-cmd-shift-pretty-prefix:
# Please translate only Ctrl and Shift and leave the rest untouched unless your
# localization requires another connector instead of the plus (+) character.
ctrl-cmd-shift-pretty-prefix = { PLATFORM() ->	
        [macos] ⌘ ⇧{" "}
	*[other] Ctrl+Shift+ }

* Create dedicated modular entries only for the keyboard shortcuts:

# Do not translate *-show-address-row-pretty-shortcut.
to-show-address-row-pretty-shortcut = { ctrl-cmd-shift-pretty-prefix }{ to-show-address-row-key }

* The .tooltip attribute translation is now much easier and safer for translations:

    .tooltiptext = Show { to-address-row-label } field ({ to-show-address-row-pretty-shortcut })

Same by analogy for CC and BCC - much cleaner, safer and clutter-free.

@@ +81,5 @@
> +        [macos] Show To field (⌘ ⇧ { to-compose-show-addressing-field-key })
> +        *[other] Show To field (Ctrl+Shift+{ to-compose-show-addressing-field-key })
> +    }
> +
> +to-compose-field-menuitem =

to-show-address-row-menuitem =

CC and BCC by analogy

@@ +83,5 @@
> +    }
> +
> +to-compose-field-menuitem =
> +    .label = Show To field
> +    .accesskey = { to-compose-show-addressing-field-key }

I think localizers should be able to choose the access key according to their other access keys in the menu, which we don't know, so this should just be:

    .accesskey = T

@@ +86,5 @@
> +    .label = Show To field
> +    .accesskey = { to-compose-show-addressing-field-key }
> +    .acceltext = { PLATFORM() ->
> +        [macos] ⌘ ⇧ { to-compose-show-addressing-field-key }
> +        *[other] Ctrl+Shift+{ to-compose-show-addressing-field-key }

...and here's where we benefit again from the modular approach above and can simply rewrite this as:

    .acceltext = { to-show-address-row-pretty-shortcut }

@@ +116,5 @@
> +    .label = Show Bcc field
> +    .accesskey = { bcc-compose-show-addressing-field-key }
> +    .acceltext = { PLATFORM() ->
> +        [macos] ⌘ ⇧ { bcc-compose-show-addressing-field-key }
> +        *[other] Ctrl+Shift+{ bcc-compose-show-addressing-field-key }

All of these fancy things can be simplified as per above.
Attachment #9199659 - Flags: review?(bugzilla2007) → review-

if (!(event.ctrlKey || event.metaKey) && !event.shiftKey) {
Something's wrong here, as this won't work as expected when Ctrl or Meta are pressed, but Shift is not, or vice versa.

Yup, silly mistake.

to-compose-show-addressing-field-key = T
This file is called messengercompose.ftl, so it's all about composing, so I think we can drop the -compose- hint and shorten the rest.

Magnus always recommends to use long and unique IDs for fluent strings since they're not self contained in their own file, and it's easy to conflict with other strings if the ID is no very specific.

Great suggestions for the Fluent file, I'll do them.

For the rest of the code updates, especially the method and variable name changes, aren't we going towards the same issue you pointed out in bug 1640760? The patch gets too big and the code clean up improvements shouldn't be done at the same time.

(In reply to Alessandro Castellani (:aleca) from comment #31)

to-compose-show-addressing-field-key = T
This file is called messengercompose.ftl, so it's all about composing, so I think we can drop the -compose- hint and shorten the rest.

Magnus always recommends to use long and unique IDs for fluent strings since they're not self contained in their own file, and it's easy to conflict with other strings if the ID is no very specific.

Well, they are initially self-contained in their own file, but probably they end up in the same document.l10n object scope. Do we have any central design document which lays out such caveats and hints for general naming patterns?

I would still change:
addressing-field into
address-row

Great suggestions for the Fluent file, I'll do them.

For the rest of the code updates, especially the method and variable name changes, aren't we going towards the same issue you pointed out in bug 1640760? The patch gets too big and the code clean up improvements shouldn't be done at the same time.

So you are now beating me at my own game!? :-O

  • Yeah, we can get away with using the existing function showAddressRow() (rather than showAndFocusAddressRow) to avoid an unrelated renaming spree (and maybe focus isn't all that special to require the longer function name). You are right, to focus on the essentials here and not bloat the patch, variable renaming can be outsourced.
  • Event handlers should always have generic names like "someElementIdOnClick()" and not directly call a special function like showAddressRow(), to prevent mixups between the handler (only called for event handling) and the special function (which can be called from anywhere, and shouldn't have event handler stuff). Mixing the two tends to degenerate over time, and is harder to read in code because you won't know which event triggers the special function without checking in the layout layer.

Comment on attachment 9199659 [details] [diff] [review]
1667692-compose-shortcuts.diff

As I discovered in bug 1640760, this won't work in macOS as we need to use onkeydown in order to intercept the OS universal shortcut.

Attachment #9199659 - Flags: review?(mkmelin+mozilla) → review-

(In reply to Alessandro Castellani (:aleca) from comment #33)

Comment on attachment 9199659 [details] [diff] [review]
1667692-compose-shortcuts.diff

As I discovered in bug 1640760, this won't work in macOS as we need to use onkeydown in order to intercept the OS universal shortcut.

Which Mac OS shortcuts are we intercepting (wrt OS functionality)?

The CMD+SHIFT+* combination is commonly used on macOS for some Terminal and man page quick actions, and CMD+SHIFT+B is used to send a selected file via Bluetooth.

Since we can safely assume that if Thunderbird is in foreground and focus, the user won't be able to interact with terminal or the finder, therefore we can intercept those shortcuts and do what we want.
Nonetheless, this is the default behavior of the XUL <key>, which overrides OS specific shortcuts.

Attached patch 1667692-compose-shortcuts.diff (obsolete) — Splinter Review

Patch updated to work also on macOS.

Regarding the suggestion of having something like this:

to-address-row-label = To

to-show-address-row-label =
    .value = { to-address-row-label }
    .tooltiptext = Show { to-address-row-label } field

I think we should avoid it since we can't safely assume that this format will work in every language like it does for Latin based languages.
Better give translators the flexibility they need...I guess.

What do you think?

Attachment #9199659 - Attachment is obsolete: true
Attachment #9204725 - Flags: review?(mkmelin+mozilla)
Attachment #9204725 - Flags: review?(bugzilla2007)

Screenshot: Fluent localization of a referenced term with grammatical cases
https://www.projectfluent.org/

(In reply to Alessandro Castellani (:aleca) from comment #36)

to-show-address-row-label =
.value = { to-address-row-label }
.tooltiptext = Show { to-address-row-label } field


I think we should avoid it since we can't safely assume that this format will work in every language like it does for Latin based languages.
Better give translators the flexibility they need...I guess.

What do you think?

Well, it sort of depends on how good translators are with Fluent, or how good we guide them.
Fluent has been specifically designed so that you as a developer can ignore all the details of the localization, and it's up to the localizer to secure the correct grammatical forms. Fluent provides simple and explicit ways of doing that on the localization side.

In a strict sense (and correctly applying the concept of Fluent), what we want to achieve here is localizer attention to a specific fixed term (the field name), which should be used consistently throughout. The most explicit way of doing that is using the term as a reference whereever it's used. Grammatical cases can and must be handled by the localizer as shown in the screenshot above.

So it sort of boils down to the question if we trust our localizers to handle those details of fluent, and using references with grammatical casing. We can make it easier for localizers at the price of not having any explicit pointers to the fixed term, but either way we'll rely on localizer's intelligence to handle the fixed terms correctly. In a way it might be slightly easier for both sides (us and them) to skip the referencing and grammatical casing stuff, but I'm not sure if it's going to backfire when we end up with localized variations of what we expected to be fixed terms. Worse me may never notice if we don't know the target language, but users will be confused if terms are not consistently used...

I don't have a strong opinion on this.
Exploiting the Fluent concept correctly may provide higher accuracy in the long run.
It also reduces the maintenance workload of having to change many translations when a specific term changes.
Let's say if we rename "Attachment pane" to "Attachment area", with fluent casing/reference system you only change one string. Using our legacy system of flat translations without references, every string that involves "Attachment area" must be retranslated.

Comment on attachment 9204725 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

Yes, this looks much better, thank you very much for this new iteration!
- I have some nits.
- Wrt fluent design, we need dedicated fluent messages for each UI element (disclosure buttons vs. row labels), even if they accidentally have the same label text in our locale (but different tooltips etc.).
- We need to decide per my comment 37 if we want greater accuracy by referencing fixed terms like the field types (To/Cc/Bcc); grammatical cases won't matter (localizer's job to use fluent case markers). Note: Even if *we* use references, I believe that will not stop localizers from removing those references in messages and replace them with cased plaintext as required. It's more like hinting that we want exactly the same term used.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4076,5 @@
>    setKeyboardShortcuts();
>  }
>  
>  /**
>   * Enable a keypress document listener for international keyboard shortcuts.

It's no longer keypress.

 * Add a keydown document event listener for international keyboard shortcuts.

@@ +4081,5 @@
>   */
>  async function setKeyboardShortcuts() {
> +  let [
> +    filePickerKey,
> +    toggleBucket,

toggleBucketKey

@@ +4085,5 @@
> +    toggleBucket,
> +    toKey,
> +    ccKey,
> +    bccKey,
> +  ] = await l10nCompose.formatValues([

Nice!

@@ +4090,3 @@
>      { id: "trigger-attachment-picker-key" },
>      { id: "toggle-attachment-pane-key" },
> +    { id: "to-compose-show-addressing-row-key" },

to-compose-show-address-row-key (replace addressing with address, the term which we are using is address-row, and it's shorter, too), here and below for CC/BCC by analogy

@@ +4095,4 @@
>    ]);
>  
>    document.addEventListener("keydown", event => {
>      // Don't do anything if we don't have the correct combination of

Comment is not exactly matching what it does (would have to be "Only do something if... and if ... is *not* a modifier).
Can we rewrite this as an early return (which is what the comment suggests)?

@@ +4099,5 @@
>      // CTRL/CMD + SHIFT, and if the pressed key is a modifier.
>      if (
>        (event.ctrlKey || event.metaKey) &&
>        event.shiftKey &&
>        !["Shift", "Control", "Meta"].includes(event.key)

I don't think this is needed, clumsy to read. We're not matching those keys below, so nothing will happen, even if the user let's go after pressing Ctrl+Shift. Not different from any other keys which we're not matching, like x, y, z.

@@ +4102,5 @@
>        event.shiftKey &&
>        !["Shift", "Control", "Meta"].includes(event.key)
>      ) {
>        // Always alter the key to compare the lowercase and avoid inconsistencies
>        // betweent different OSs.

Can you explain? Can Shift+Character *not* return the capital .key (T/C/B) for keydown event on some OS?
If possible, please eliminate the lowercase conversion.

@@ +4110,1 @@
>          case filePickerKey.toLowerCase():

Please add a comment inside each case which mentions the complete shortcut key for clarity. They're international and localizers are not supposed to change them, so we know them. Same below by analogy.
// Ctrl/Cmd+Shift+A

@@ +4112,4 @@
>            goDoCommand("cmd_attachFile");
>            break;
>  
>          case toggleBucket.toLowerCase():

toggleBucketKey

@@ +4136,5 @@
>    });
>  }
>  
> +/**
> + * Helper function used by the menu items of the primary addressing fiselds.

fields (spelling), and View menu:
* Helper function used by the View menu items of the primary addressing fields.

@@ +4139,5 @@
> +/**
> + * Helper function used by the menu items of the primary addressing fiselds.
> + *
> + * @param {string} labelID - The ID of the label to hide.
> + * @param {string} rowID - The ID of the addressing container to reveal.

addressing container -> address row (hence rowID, not containerID)

@@ +4141,5 @@
> + *
> + * @param {string} labelID - The ID of the label to hide.
> + * @param {string} rowID - The ID of the addressing container to reveal.
> + */
> +function menuShowAndFocusFieldOnCommand(labelID, rowID) {

Sorry, but on second thoughts, I guess we could make this function name both simpler and clearer (Focus is a rather minor detail, especially coming from the menu) - congruence with the inner function intended:

function menuShowAddressRowOnCommand(labelID, rowID) {

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +1405,5 @@
> +      "menu_showCcField"
> +    ).disabled = document.getElementById("addr_cc").collapsed;
> +    document.getElementById(
> +      "menu_showBccField"
> +    ).disabled = document.getElementById("addr_bcc").collapsed;

Quite elegant!

::: mail/components/compose/content/messengercompose.xhtml
@@ +1060,5 @@
>                          accesskey="&fullZoomToggleCmd.accesskey;"
>                          type="checkbox" command="cmd_fullZoomToggle" checked="false"/>
>              </menupopup>
>            </menu>
> +          <menuseparator/>

Normally we have IDs on menuseparators (see below)

@@ +1064,5 @@
> +          <menuseparator/>
> +          <menuitem id="menu_showToField"
> +                    data-l10n-id="to-compose-address-row-menuitem"
> +                    data-l10n-attrs="acceltext"
> +                    oncommand="menuShowAndFocusFieldOnCommand('addr_to', 'addressRowTo')"/>

menuShowAddressRowOnCommand(...) and by analogy below

@@ +1083,5 @@
>                      data-l10n-id="menuitem-toggle-attachment-pane"
>                      data-l10n-attrs="acceltext"
>                      type="checkbox"
>                      command="cmd_toggleAttachmentPane"/>
> +          <menuseparator/>

Why remove the menuseparator ID? Maybe addons cannot use this any more, but Thunderbird might?

@@ +2147,5 @@
>                    <hbox id="addressingWidgetLabels" class="address-extra-recipients"
>                          flex="1" align="center">
>                      <hbox id="addressingWidgetSwappableLabels">
> +                      <label id="addr_to" role="button"
> +                             data-l10n-id="to-compose-address-row-label"

Let's not share the same data-l10n-id for different UI elements, that's risky and not scalable.
You may want different tooltips on disclosure buttons vs. real row labels (right now the real row labels will have the disclosure tooltip afasics, which is wrong). We might want to change only the disclosure labels, but not the row labels. Certain languages may need to abbreviate real row labels etc.

The correct way is for every UI element to have its own fluent ID, and then within fluent, we suggest sharing strings.
Btw, why don't we add a helpful tooltip on the disclosure buttons, which could also advertise the Shortcut in-place?
Not everyone knows what CC and BCC are all about...

data-l10n-id="to-compose-show-address-row-label"
(and others by analogy)

@@ +2157,5 @@
>                               onkeypress="showAddressRowKeyPress(event, 'addressRowTo')"
>                               control="toAddrInput"
>                               collapsed="true"/>
> +                      <label id="addr_cc" role="button"
> +                             data-l10n-id="cc-compose-address-row-label"

cc-compose-show-address-row-label

@@ +2166,5 @@
>                               ondragover="event.preventDefault();"
>                               onkeypress="showAddressRowKeyPress(event, 'addressRowCc')"
>                               control="ccAddrInput"/>
> +                      <label id="addr_bcc" role="button"
> +                             data-l10n-id="bcc-compose-address-row-label"

bcc-compose-show-address-row-label

@@ +2242,5 @@
>                    </hbox>
>                    <hbox class="address-label-container" align="top" pack="end"
>                          style="&headersSpace2.style;">
> +                    <label id="toAddrLabel"
> +                           data-l10n-id="to-compose-address-row-label"

This is the real row label. Correct.

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +116,5 @@
>      .label = Receipt
>      .tooltiptext = Request a return receipt for this message
> +
> +# Addressing Area
> +

Sorry, we definitely want a comment here that this must *not* be translated.
We can end up in total chaos if localizers mistake this for a localizable access key.
Better safe than sorry. Comments are not harmful.

# Used for international shortcut keys Ctrl/Cmd+Shift+T/C/B for To, Cc and Bcc.
# Do not translate.

@@ +121,5 @@
> +to-compose-show-addressing-row-key = T
> +cc-compose-show-addressing-row-key = C
> +bcc-compose-show-addressing-row-key = B
> +
> +to-compose-address-row-label =

You need to split this up into at least two dedicated l10n-IDs (explained above).
One set for the disclosure buttons:

to-compose-show-address-row-label =
(as explained above, others by analogy.)

Then another set for the actual row labels:

to-compose-address-row-label =
(others by analogy)

If we use references for the fixed terms, another set for the plain vanilla labels:

to-compose-address-row-label-text =

@@ +123,5 @@
> +bcc-compose-show-addressing-row-key = B
> +
> +to-compose-address-row-label =
> +    .value = To
> +    .tooltiptext = Show To field ({ ctrl-cmd-shift-pretty-prefix }{ to-compose-show-addressing-row-key })

We want the tooltip on the disclosure buttons, but not on the row labels. That's what we achieve by having a dedicated l10n-id for the disclosure buttons.

As explained in detail in my comment 37, we should probably use references for fixed terms here for greater accuracy. Grammatical cases should not be a problem as they can be handled with fluent case markers on the base term and the references as required for each locale.

To avoid problems with .value (which is implementation-level), we probably need a plain string which can be referenced whereever needed. So I think the net result should be this (other types by analogy):

# The labels in front of each address row, also used for row disclosure buttons.
to-compose-address-row-label-text = To
(Here's where localizations can create their versions with grammatical case as required)

# The label in front of the address row.
to-compose-address-row-label =
    .value = { to-compose-address-row-label-text }

# The label on the row disclosure button.
to-compose-show-address-row-label =
    .value = { to-compose-address-row-label-text }
    .tooltiptext = Show { to-compose-address-row-label-text } field ({ ctrl-cmd-shift-pretty-prefix }{ to-compose-show-addressing-row-key })

to-compose-address-row-menuitem =
    .label = Show { to-compose-address-row-label-text } field
    .accesskey = T
    .acceltext = { ctrl-cmd-shift-pretty-prefix }{ to-compose-show-addressing-row-key }
Attachment #9204725 - Flags: review?(bugzilla2007) → review-

Comment is not exactly matching what it does (would have to be "Only do something if... and if ... is not a modifier).
Can we rewrite this as an early return (which is what the comment suggests)?

I wrote this not as an early return to avoid a cumbersome condition since we're handling both CTRL and CMD key, and one of these 2 modifier keys will always be false.
What do you think about this? Do you have a better approach to suggest?

if (
      (AppConstants.platform == "macosx" && !event.metaKey) ||
      (AppConstants.platform != "macosx" && !event.ctrlKey) ||
      !event.shiftKey ||
      ["Shift", "Control", "Meta"].includes(event.key)
    ) {
      return;
    }

I don't think this is needed, clumsy to read. We're not matching those keys below, so nothing will happen, even if the user let's go after pressing Ctrl+Shift. Not different from any other keys which we're not matching, like x, y, z.

keyup fires this event at every key, so even the modifiers trigger it.
I'd prefer to return it early and avoid running the switch.

Can you explain? Can Shift+Character not return the capital .key (T/C/B) for keydown event on some OS?
If possible, please eliminate the lowercase conversion.

Yes, on macOS the CMD+SHIFT+ modifiers don't affect the capitalization of the letter, therefore the event.key is always lowercase.
Forcing everything lowercase makes this work on all OSs.

Attached patch 1667692-compose-shortcuts.diff (obsolete) — Splinter Review

Patch updated to address everything.
I also improved the fluent string by using a modular approach and avoid repetitions of the "Show {} field" string.

Attachment #9204725 - Attachment is obsolete: true
Attachment #9204725 - Flags: review?(mkmelin+mozilla)
Attachment #9204945 - Flags: review?(mkmelin+mozilla)
Attachment #9204945 - Flags: review?(bugzilla2007)
Comment on attachment 9204945 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

This looks great and works perfectly. Almost there!
- I have some nits and a small Fluent design thing about referencing more than we should (imo). It's a fine line and a bit of a moving target as we're all new to Fluent, and imagining other localizations is never easy.
- Also, I'd suggest not to disable the menuitems, to simplify their labels for consistency with other menu items, and to group them with another separator.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4095,5 @@
>    ]);
>  
>    document.addEventListener("keydown", event => {
> +    let accelKey =
> +      AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey;

This is perfect. In fact, it's the only correct solution, and highly readable.

I tried XOR (^) operator for simplification inside the conditional below, which works amazingly well, but still allows one false positive on MAC (Control). Btw, I understand that the previous `(event.ctrlKey || event.metaKey) &&` also allows false positive with Control on MAC, and we probably have a number of those in our code.

@@ +4100,2 @@
>      // Don't do anything if we don't have the correct combination of
>      // CTRL/CMD + SHIFT, and if the pressed key is a modifier.

As you don't like 'bail out', I think "return" is the best way of describing `return`...
|| in code should be 'or' in comment... Also, let's explain why we're checking for modifiers as event.key.
```
    // Return if we don't have the right modifier combination, CTRL/CMD + SHIFT,
    // or if the pressed key is a modifier (each modifier will keep firing
    // keydown event until another key is pressed in addition).
```

@@ +4107,5 @@
> +      return;
> +    }
> +
> +    // Always alter the key to compare the lowercase and avoid inconsistencies
> +    // betweent different OSs.

spelling: between
Let's describe the inconsistency which we're seeing.

```
    // Always use lowercase to compare the key and avoid OS inconsistencies:
    // For Cmd/Ctrl+Shift+A, on Mac, key = "a" vs. on Windows, key = "A".
```

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +1396,5 @@
>          .getElementById("addressingWidgetLabels")
>          .querySelector('label:not([collapsed="true"])')
>      );
> +
> +    // Toggle the disable state of the main addressing fields menu items.

I think we should not disable the menu items, because the commands and the keyboard shortcuts are always available. Disabling falsely suggests that the shortcut wouldn't work when the field is already shown, but it does.
Also, disabling does not transport the notion "currently open" very well, if at all (so it doesn't add much informational value to disable).
As we're not showing a checkmark (because it's not a traditional toggle button), users cannot expect to hide the fields from the menu. The only legimate expectation for `View > Unchecked item` is to show and focus that item, and that's exactly what we're doing.

Some users may even find it convenient to use localized access keys to focus the field via the menu if for some reason they don't like or cannot remember the international shortcut.

::: mail/components/compose/content/messengercompose.xhtml
@@ +1072,5 @@
> +                    oncommand="menuShowAddressRowOnCommand('addr_cc', 'addressRowCc')"/>
> +          <menuitem id="menu_showBccField"
> +                    data-l10n-id="bcc-compose-show-address-row-menuitem"
> +                    data-l10n-attrs="acceltext"
> +                    oncommand="menuShowAddressRowOnCommand('addr_bcc', 'addressRowBcc')"/>

These commands are a bit different as they are not toggle buttons and will not show a checkmark.
Let's add another separator here to group them together, which makes visually parsing the menu easier.

@@ +2242,5 @@
>                    </hbox>
>                    <hbox class="address-label-container" align="top" pack="end"
>                          style="&headersSpace2.style;">
> +                    <label id="toAddrLabel"
> +                           data-l10n-id="to-compose-address-row-label"

Correct, thanks!

@@ +2281,5 @@
>                    </hbox>
>                    <hbox class="address-label-container" align="top" pack="end"
>                          style="&headersSpace2.style;">
> +                    <label id="ccAddrLabel"
> +                           data-l10n-id="cc-compose-show-address-row-label"

Wrong data-l10n-id (remove -show), see my last review (you overlooked this as you did change it for `To`)

@@ +2320,5 @@
>                    </hbox>
>                    <hbox class="address-label-container" align="top" pack="end"
>                          style="&headersSpace2.style;">
> +                    <label id="bccAddrLabel"
> +                           data-l10n-id="bcc-compose-show-address-row-label"

dito, wrong data-l10n-id (remove -show)

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +132,5 @@
> +    .acceltext = { ctrl-cmd-shift-pretty-prefix }{ to-compose-show-address-row-key }
> +
> +to-compose-show-address-row-label =
> +    .value = { to-compose-address-row-label }
> +    .tooltiptext = { to-compose-show-address-row-menuitem.label } ({ to-compose-show-address-row-menuitem.acceltext })

Interesting! I appreciate the modular idea which avoids repetitions and enforces consistency. Referencing the menuitem.acceltext is a smart move! However, I think we should only do that where we're quite sure that every localization MUST have that 100% consistency. For the tooltiptext's main part, let's not be more prescriptive than necessary... This might have odd results for other localizations like German:

German for "Bcc" is "Blindkopie (Bcc)", much longer. So for the menu, they might go for something shorter like "Blindkopie (Bcc) anzeigen" or even (Menü Anzeigen > ) "Blindkopie-Feld (BCC)" because "Blindkopie-Feld (Bcc) anzeigen" might be too long... Does that mean that the tooltip should have the same shorter variant without the word "field"? That sounds odd. Maybe they want "Eingabefeld Blindkopie (BCC) anzeigen" in the tooltip, and I think that's ok - tooltips are allowed to deviate from a menu entry so that they sound right and make sense for the local audience. Iow, menu and tooltip are different contexts.

That's a bit different when we reference { to-compose-address-row-label } and friends, because there we'd actually expect/encourage 100% consistency for the field labels where possible (even though the abbreviation issue might still occur, but that's localizer's decision and responsibility).

TL;DR: I think we should use
.tooltiptext = Show { to-compose-address-row-label } field ({ to-compose-show-address-row-menuitem.acceltext })

What do you think?

Btw: Do we really need the verb "Show ... " in the View menus? The menu sequence `View > Show...` is a bit of a duplicate expression... Anything wrong with `View > Bcc field`? "Show ..." stands out from all the other menu items which are noun phrases, for no apparent reason. Yes, we can't hide them from here, but we indicate that by not showing the checkmark, which suffices imo. That will reduce the complexity and length of the menuitem strings.
Attachment #9204945 - Flags: review?(bugzilla2007)

Here's a screenshot of my proposal to polish the menuitems.

Attached patch 1667692-compose-shortcuts.diff (obsolete) — Splinter Review

The suggested changes are sensible, thanks.

Attachment #9204945 - Attachment is obsolete: true
Attachment #9204945 - Flags: review?(mkmelin+mozilla)
Attachment #9205139 - Flags: review?(mkmelin+mozilla)
Attachment #9205139 - Flags: review?(bugzilla2007)
Comment on attachment 9205139 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

Awesome!
One nit which I overlooked in the previous iteration.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4103,2 @@
>      if (
> +      !accelKey ||

Didn't see last time that accelKey should be inlined:
```
    if (
      !(AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey) ||
```
Attachment #9205139 - Flags: review?(bugzilla2007) → review+
Attached patch 1667692-compose-shortcuts.diff (obsolete) — Splinter Review

Thomas already r+ this patch.
Magnus, what do you think about this implementation?

I personally find this great and very important from a UX point of view.

Attachment #9205139 - Attachment is obsolete: true
Attachment #9205139 - Flags: review?(mkmelin+mozilla)
Attachment #9205493 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9205493 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

I'm ok with adding the shortcuts, but some things to fix...

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +121,5 @@
> +#    Used for international shortcut keys Ctrl/Cmd+Shift+T/C/B for To, Cc and Bcc.
> +#    Do not translate.
> +to-compose-show-address-row-key = T
> +cc-compose-show-address-row-key = C
> +bcc-compose-show-address-row-key = B

We should not put them in the .ftl file then.

@@ +126,5 @@
> +
> +to-compose-address-row-label = To
> +
> +to-compose-show-address-row-menuitem =
> +    .label = { to-compose-address-row-label } field

These would all be capitalized Field.

But, I don't think you can add them as menu items. That part is very weird. If you'd make them checkbox menu items it would be slightly better, but then again, since you don't want to allow hiding with the hotkey, it doesn't really work out with that either.

I recently came across this:  https://github.com/WICG/accessible-loading-and-searching-of-content/issues/5
Something like that could be a better approach to make such keys known.
Attachment #9205493 - Flags: review?(mkmelin+mozilla) → review-

(In reply to Thomas D. (:thomas8) from comment #42)

Created attachment 9205101 [details]
Screenshot: Polished menuitems (simplified labels, always enabled, trailing separator)

Here's a screenshot of my proposal to polish the menuitems.

This looks really good. Users will find that information intuitive to locate under the 'View' menu.
It is important to have menu items because there are users who do not use shortcut keys for various reasons. There are plenty of people who need a selectable menu item. We need to consider all methods that people use.
The menu items will also mean easy selection for those who use the keyboard arrow keys and enter.
It also offers information on what shortcuts keys to use for those who may prefer the shortcut method.

This design follows the natural generic and intuitive design utilised throughout other menu items, so will be easy to learn and discover. This is equally important for those who need continuity such as those suffering from Autism or where age can make it difficult to learn new things.

It will be easy for those offering help in the Support Forum to display that menu when offering support. Often, it is important to provide an image of where to locate information and this design will facilitate it as we can show the full path on what to select.

Thankyou Thomas for producing a workable design that everyone can use.

(In reply to Anje from comment #47)

It is important to have menu items because there are users who do not use shortcut keys for various reasons.
...
This design follows the natural generic and intuitive design utilised throughout other menu items, so will be easy to learn and discover. This is equally important for those who need continuity such as those suffering from Autism or where age can make it difficult to learn new things.

It will be easy for those offering help in the Support Forum to display that menu when offering support.
...
Thank you Thomas for producing a workable design that everyone can use.

Oh, I think we have to thank Alex, our UX lead, for the fine implementation of my idea and the useful addition of the menu items. It's a teamwork thing!

Thank you Anje for chipping in from a support and accessibility POV.

(In reply to Magnus Melin [:mkmelin] from comment #46)

But, I don't think you can add them as menu items. That part is very weird.

I think we need to approach this in view of Anje's comment, which illustrates that menu items are very important to provide access for users with special access issues. It's not just about making the shortcut keys discoverable. It's about providing a generic, systematic route of access for every feature through menus, which, as Anje pointed out, can be crucial e.g. for users with Autism, blindness etc. Providing menu access hence looks less like a design choice and more like a requirement for fully accessible software.

If you'd make them checkbox menu items it would be slightly better, but then
again, since you don't want to allow hiding with the hotkey, it doesn't
really work out with that either.

You are right that checkboxes would not work out well here, but I think the menuitems are needed and useful.

  • I've tried to address the slight variance of the proposed menu items in my detailed review (comment 41), and from my careful analysis, which Alex found "sensible", we're not creating any UX problems here.
  • Most users will use the keyboard shortcuts to show and/or focus these fields (but for other users menu access will be crucial).
  • As we're not adding the checkmarks, it's obvious that you can only view/focus these fields via the menu, but not hide them (it's an always available one-way command, not a toggle). The logic seems fine: View > CC field - will always do just that.
  • Menus provide localized keyboard access.

I recently came across this:
https://github.com/WICG/accessible-loading-and-searching-of-content/issues/5
Something like that could be a better approach to make such keys known.

That's a worthwhile idea to explore for shortcut discovery, whereas the menus are also needed for special needs accessibility.
Alex and I are both very sensitive when it comes to "weird" UX, so if both of us are happy, it can't be all that bad ;-)

Summary: Explore implementing keyboard shortcuts to show and focus important address rows (To, CC, BCC): Ctrl+Shift+T, Ctrl+Shift+C, Ctrl+Shift+B (like gmail) → Implement keyboard shortcuts to show and focus important address rows (To, CC, BCC): Ctrl+Shift+T, Ctrl+Shift+C, Ctrl+Shift+B (like gmail)

We should not put them in the .ftl file then.

We already have some international shortcut keys in that file.
https://searchfox.org/comm-central/rev/0a5420357425c5fc45abf6d882e811ca79518113/mail/locales/en-US/messenger/messengercompose/messengercompose.ftl#56-57

Should we use the same approach and in a follow up but take out those keys from the fluent file?

We could create an array map for all those shortcuts.
That would also open up the gate for a future shortcut customization implementation.

Attached patch 1667692-compose-shortcuts.diff (obsolete) — Splinter Review

What do you think about this approach?

Attachment #9205493 - Attachment is obsolete: true
Attachment #9205818 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9205818 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

Where there's an applicable command the menu is a good place to also advertize the shortcut. 
But this is a "show, or focus, or both" command, which isn't really a command at all, so I don't want to add that in the menu like this for the sole purpose of advertising it.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +201,5 @@
>    },
>  };
>  
> +// Object to list all the non translatable international shortcuts.
> +const composeKeyboardShortcuts = {

Don't quit see how this mapping helps any. The code would just use "T", "C", "B" where needed.
Attachment #9205818 - Flags: review?(mkmelin+mozilla) → review-

Alex Arnaud (as a user depending on easy access), can you comment from an accessibility point of view?

To improve ux-efficiency in message composition, we're introducing 3 new commands, "Show and focus To field", "Show and focus CC field", and "Show and focus BCC field". They come with international keyboard shortcuts, Ctrl+Shift+T, Ctrl+Shift+CC, and Ctrl+Shift+BCC. On Mac, it's Command key instead of Control. When a field isn't shown, the command will show and and focus them. When a field is shown, it will just focus them, which is especially convenient via keyboard shortcut. So the net result is always "Field shown and focused". We're currently discussing ways of making these new commands discoverable and accessible.

  • In the main UI, the commands will be available from the address row disclosure buttons (CC/BCC) on top of the "To" address field. The "To" button will usually not be shown unless it's a News account. The buttons have a tooltip which mentions the respective keyboard shortcut.
  • Alex and I have proposed to make these commands and their shortcuts easily discoverable and accessible via the main menus: View > To field / Cc field / Bcc field. Using the menus (as explained above) will always show and/or focus the field. Hence the commands are never disabled, and do not have a checkmark as you cannot toggle them off like, for example, Contacts Side Bar.
  • Magnus doesn't want to have these commands included in the menu as he believes that they are "not really a command at all" and he finds them "weird" because they work a bit different from the classic toggle buttons in the View menu.

Here's our question:

  • How important is it for users depending on easy access (blind, autist and other special needs) to have these commands accessible in the View menu? Also considering that the shortcut to focus the "To" field will not be discoverable unless you've set up and are using a "News" identity.

For more background, you may want to read Anje's comment 47, my comment 48, and Magnus' comment 51.
Tia for assisting us with the accessibility evaluation.

Flags: needinfo?(aarnaud)

@Alex Arnaud, attachment 9205101 [details] has a screenshot of the proposed menu entries in the View menu, iirc you once mentioned that you're still able to see things. The menu entries would obviously advertise the shortcuts, too, and have localized access keys.

Just redirecting the n-i to Jean-Philippe and Valentin.

I am moving, I don't think I'll have the time to handle request this month.

Flags: needinfo?(valentin)
Flags: needinfo?(jpmengual)
Flags: needinfo?(aarnaud)
Attached patch 1667692-compose-shortcuts.diff (obsolete) — Splinter Review

All right, let's try again with this patch.

Menu items and Shortcut discoverability

  • The View > To Field etc, menu items are disabled if the field is already visible, since that command semantically doesn't make sense (View something that it's already visible).
  • The shortcuts are listed as tooltip text for the button labels in the addressing area, and as accelltext in the menu items to be easily discoverable by both mouse and keyboard users.

Adding accesskeys

  • I added accesskey attributes to the fields label because that's the correct thing to do. Since those fields are a representation of a Form, we should offer Alt+* quick access as we do for the other fields in the addressing area.
  • Those accesskeys are not connected to the shortcuts, making them easily translatable.
  • Users will know that they can quickly focus on those fields like a normal form, without making things complicated for us to teach them that Ctrl+Shift+* also focuses. That's a side effect of revealing a collapsed field, nothing more.
  • The To label accesskey interferes with the Tools menu item. I don't see this as a big problem because we can consider changing the Tools accekey, that menu item has objectively a lower priority in terms of access compared to the To field, this conflict wouldn't be constant and translators can handle it independently from the JavaScript code.

Shortcut keys listed in an array

  • This allows us to declared those keys once and reference them whenever we need (on keydown, when setting up menu item strings, etc).
  • In future iterations, it will be super easy to allow shortcut customization since we can simply replace that array in one place.

I consider this an optimal solution, offering discoverable international shortcuts on par with Gmail, and maintaining localized accesskey for all our form fields.

Attachment #9205818 - Attachment is obsolete: true
Attachment #9208830 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9208830 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

Where creativity meets competence! Thank you Alex for another awesome UX design patch. This looks like an optimum solution to make everyone happy. No objections from my side, just some thoughts.

It's a bit unfortunate that you're duplicating existing access keys from contacts sidebar "Add to Cc/Bcc" buttons, which can make this a bit error-prone for specific scenarios if users don't double-check their current focus. Technically it happens to work in spite of the duplication. We could prevent the most likely user error in a followup bug by deselecting addresses in contacts side bar when the search box gets focus via mouse (as it happens for keyboard, Alt+n).

The current shortcuts array may not be our ultimate solution for enabling fully customizable shortcuts lateron (esp. regarding modifier keys), but we can certainly run with this for now.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +210,5 @@
> +// Object to list all the non translatable international shortcuts.
> +const composeKeyboardShortcuts = {
> +  "show-to-address-row": "T",
> +  "show-cc-address-row": "C",
> +  "show-bcc-address-row": "B",

Great idea of keeping shortcut keys in a central array wrt future options of shortcut customization.

I'm a bit worried that this will not allow customizing the modifier part of the shortcut, as we're hardcoding that in the key events below. We have different modifier combinations, currently at least Ctrl/Cmd+*, Ctrl/Cmd+Shift+*, and unmodified shortcut keys. For full customization, we may want to allow more.

I understand we may not be able to develop the full shortcut customization system here, but then maybe we need to accept and be aware that while this is good because it's somehow modular, we can't be quite sure if this is the right type of modularity which will allow full shortcut customization later... Just saying.

@@ +4156,5 @@
> +  document.l10n.setAttributes(
> +    document.getElementById("menu_showToField"),
> +    "to-compose-show-address-row-menuitem",
> +    { key: composeKeyboardShortcuts["show-to-address-row"] }
> +  );

Would this benefit from a helper function?

setl10nAttributes(targetId, fluentId, shortcutId, shortcutsObj) {...}

@@ +4205,4 @@
>      if (
> +      !(AppConstants.platform == "macosx" ? event.metaKey : event.ctrlKey) ||
> +      !event.shiftKey ||
> +      ["Shift", "Control", "Meta"].includes(event.key)

See my comments wrt future full shortcut customization modularity above.

::: mail/components/compose/content/messengercompose.xhtml
@@ +1070,5 @@
> +                    data-l10n-attrs="acceltext"
> +                    oncommand="menuShowAddressRowOnCommand('addr_to', 'addressRowTo')"/>
> +          <menuitem id="menu_showCcField"
> +                    data-l10n-attrs="acceltext"
> +                    oncommand="menuShowAddressRowOnCommand('addr_cc', 'addressRowCc')"/>

Apart from easy discovery for these important shortcuts, these **menu items are highly valuable for *access*** (e.g. for the blind), because the row access keys cannot be used to show the rows when hidden, and even if they did that, it would be undiscoverable. There's pro's and cons for disabling (as I said before), but disabling seems to match the regular semantics of the `View` menu well and seamlessly.

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +119,5 @@
> +# Addressing Area
> +
> +to-compose-address-row-label =
> +    .value = To
> +    .accesskey = T

Yeah, as you said, this is hijacking Alt+T (pressed simultaneously) for Tools menu in en-US. However, tools menu is still accessible by first pressing Alt, let go, and then pressing T. So maybe that's acceptable; there is no other free accesskey in "Tools" which we could use (Alt+L = Remind me later button on inline attachment reminder, Alt+S = Subject). We should try to agree how we want to handle this in general; having Alt+* for main menu access sometimes work and sometimes fail isn't nice.

It's probably nice to have visible, localized access keys here, although localizers might have a hard time finding unique access keys.

@@ +133,5 @@
> +    .tooltiptext = Show { to-compose-address-row-label.value } Field ({ to-compose-show-address-row-menuitem.acceltext })
> +
> +cc-compose-address-row-label =
> +    .value = Cc
> +    .accesskey = C

Are you aware that the same access keys are used on "Add to Cc/Bcc" buttons in contacts sidebar (CSB)? This might be error-prone. Luckily, address row access keys take precedence over CSB button access keys if focus is not in contacts sidebar, although CSB button access keys normally work from everywhere (which is a design bug). However, users can still quite easily trick themselves into adding unwanted addresses:

STR
- select addresses in CSB
- focus CSB search with mouse (sic)
- change your mind somewhat (or return from a break) and press Alt+B with an intention of focusing Bcc address row.

Actual result:
- unwanted bcc recipients added

Expected:
- Not sure, but I think the best way out for now is this (perhaps in a followup bug):
  - clear CSB list selection also when CSB search (or AB selector, or AB context menu button) is mouse-focused (as we do for Alt+n access key for CSB search).
Attachment #9208830 - Flags: review+
Comment on attachment 9208830 [details] [diff] [review]
1667692-compose-shortcuts.diff

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

Coming back to this, and also looking at bug 1616514, I think we could do it like this.
Wontfix bug 1616514 as far as options are concerned. In this bug, add the View | To, Cc, Bcc menu items
* as checkbox menuitem items
* persist the menu item checkbox choice for cc and bcc (but always show To on new)
* through the menu when clicking: allow the fields to be toggled off unless they have content in them
* when using the hotkey, don't toggle closed, just focus

This way, the menu items behave (almost) as they menu items should.

I don't know about the accesskeys: you now have multiple collisions for all the ones you added: T and C from the compose sidebar. T for the _T_tools menu, and also View | _T_toolbar in the menu. There' probably just not enough free keys to do what you propose.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +208,5 @@
>  };
>  
> +// Object to list all the non translatable international shortcuts.
> +const composeKeyboardShortcuts = {
> +  "show-to-address-row": "T",

If you're using this only to have them declared once, simply declare a constant like

var SHOW_TO_KEY = "T". 

I don't see how an object would make things better. It's much worse since you don't even get linting warning you when you typoed something.

::: mail/locales/en-US/messenger/messengercompose/messengercompose.ftl
@@ +129,5 @@
> +    .acceltext = { ctrl-cmd-shift-pretty-prefix }{ $key }
> +
> +to-compose-show-address-row-label =
> +    .value = { to-compose-address-row-label.value }
> +    .tooltiptext = Show { to-compose-address-row-label.value } Field ({ to-compose-show-address-row-menuitem.acceltext })

I don't actually see where this tooltip is shown.
Attachment #9208830 - Flags: review?(mkmelin+mozilla) → review-

OT: Here's some anecdotal evidence for my claim that localization can easily break things if you give them a chance: Bug 1696623 Comment 0. That localization (Czech) gave the same access key (Alt+S) to File menu, Search contacts, and the BCC button (the last two conflicting in contacts sidebar).

Coming back to this, and also looking at bug 1616514, I think we could do it like this.
Wontfix bug 1616514 as far as options are concerned. In this bug, add the View | To, Cc, Bcc menu items

  • as checkbox menuitem items
  • persist the menu item checkbox choice for cc and bcc (but always show To on new)
  • through the menu when clicking: allow the fields to be toggled off unless they have content in them
  • when using the hotkey, don't toggle closed, just focus

Sorry, but I don't think this is a good UX approach.
Doing this creates some unpredictable inconsistencies that are hard to discover.
Reveal a field, but if it's visible hide it, but if it has content just focus it but never hide it, then make it persistent if shown.
It feels a bit too convoluted, I think we should step back and make it simple.

bug 1616514 is a thing on its own, which is "Allow users to make CC and BCC fields always visible per identity", and I think we found a good solution with the last patch from Thomas.

This bug is purely for keyboard accessibility, which is "Add an international keyboard shortcut to the CC and BCC field to allow quick opening".
The quick focusing is a consequential benefit, and showing those shortcuts in the View menu and tooltips is for total discoverability.
For this, I think we shouldn't disable the View menu items, nor make them checkboxes, but leave them as they are to guarantee that users can always select those items even if the field is already visible.

I don't know about the accesskeys...

Indeed, that's not great. I'll remove them.

I don't see how an object would make things better...

Replaced with simple variables, thanks.

I don't actually see where this tooltip is shown.

The "To" tooltip is shown when the user has a news identity selected, and the To label appears in the extra recipients list. That tooltip is also read by screen readers.

(In reply to Magnus Melin [:mkmelin] from comment #60)

This should use https://searchfox.org/mozilla-central/source/toolkit/modules/ShortcutUtils.jsm

That's neat, but if I'm not wrong those helper methods only work with a XUL <key> ID, which we don't use as we're doing everything with pure JS.

Patch updated and added a simple test to cover these scenarios.
Try-run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=1b36e35e79e44b6093be9acb3f697ab93cf1a5cf

Attachment #9208830 - Attachment is obsolete: true
Attachment #9220514 - Flags: review?(mkmelin+mozilla)
Attachment #9220514 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 90 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/24c20603478a
Implement keyboard shortcuts to show and focus importat addressing fields. r=mkmelin, r=thomas8

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Flags: needinfo?(valentin)
Flags: needinfo?(jpmengual)
Regressions: 1730454
You need to log in before you can comment on or make changes to this bug.