Bugzilla

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"] }
> +  );

In its current shape, I think this is screaming for a helper function:

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

@@ +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).

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).