1647654 - Show keyboard shortcuts on recipient pill(s) context menu

Status: ASSIGNED

(Thunderbird :: Message Compose Window, enhancement, P3)

Description

[Richard Leger]

As part of the Pill(s) system improvement in Compose Message windows in TB, I would suggest to add keyboard shortcuts to the context menu of Pill(s) and create related shortcuts for those that may not exist already...

Edit Address  F2
Delete        BACKSPACE/DEL
---
Cut           CTRL+X
Copy          CTRL+C
---
Move to To    SHIFT+T <-- those could be created for the occasion :-)
Move to Cc    SHIFT+C
Move to Bcc   SHIFT+B

Comment 1

[Alessandro Castellani [:aleca]]

I would love to add these as I think they're more intuitive and accessible than the current accesskey we use (underscore a letter for ALT+* action).
I'm not sure if the platform actually support these as I've never seen them used.

Magnus, do you know if it's possible to add shortcuts inline a context menu item? Like we have in the main AppMenu?

Comment 2

[Thomas D. (:thomas8)]

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

I would love to add these as I think they're more intuitive and accessible than the current accesskey we use (underscore a letter for ALT+* action).

There's a misunderstanding here. [EDIT: Yes, I misunderstood Alex, sorry... as we both agree that:] Even if you add shortcut keys to the menu labels, you'd still have to keep the accesskeys to make the menu itself accessible. Shortcut keys can only be used after closing the menu (so the menu is just providing this info for later use), access keys are for navigating the menu itself.

I'm not sure if the platform actually support these as I've never seen them used.

What do you mean here? Access keys? They work on Windows, yes. I think Linux is supporting them, too. MAC does not support access keys from what I have read.

Magnus, do you know if it's possible to add shortcuts inline a context menu item? Like we have in the main AppMenu?

Let me snatch this answer: Yes, definitely possible, we already display some access keys in the context menu of contacts in contacts side bar.
It's easy: Just add key="keyId" attribute to the menu item, as seen here:

https://searchfox.org/comm-central/rev/93db6b3c1403b18025e1ad81466979c5b4cd2772/mail/components/addrbook/content/abContactsPanel.xhtml#73-76

    <menuitem label="&deleteAddrBookCard.label;"
              accesskey="&deleteAddrBookCard.accesskey;"
              key="key_delete"
              command="cmd_delete"/>

Comment 3

[Alessandro Castellani [:aleca]]

I wasn't suggesting to remove the accesskeys, I was simply stating a personal preference.

As for "platform" I was referring to TB/FF itself.
I'm glad the key attribute allows us to do that, thanks for the info.

Comment 4

[Thomas D. (:thomas8)]

(In reply to Richard Leger from comment #0)

As part of the Pill(s) system improvement in Compose Message windows in TB, I would suggest to add keyboard shortcuts to the context menu of Pill(s) and create related shortcuts for those that may not exist already...

I agree with Alex that displaying shortcut keys on context menu items is wanted and helpful:

• to advertise them in-place (exactly at that time where you hustle with your mouse... ;-))
• and, especially in this case, to ease the transition and inform users about keyboard methods of dealing with pills.

In an ideal world, we should probably have a pref for that, on by default, but that's for another bug.

Edit Address  F2
Delete        BACKSPACE/DEL

Displaying more than one keyboard shortcut on one label requires assembling that info in code and then setting the acceltext attribute on <menuitem>s. The "/" must also be localizable, but that's easy with Fluent.
Technically we are not using <keys> here (as we listen directly to keydown/keypress), so we cannot immediately use key="key-ID", but there's a way of getting the pretty display name for certain keys, I've done that before [EDIT: ShortcutUtils.prettifyShortcut(keyElement), which unfortunately requires <key>, so we have to create <key>s without oncommand/command attribute for display purposes anyway]. For reorderAttachmentsPanel, we used a custom attribute prettykey (which should be data-prettykey) which was then attached and formatted with GrayText color via css, probably because acceltext attribute isn't available on <toolbarbutton>s).

---
Cut           CTRL+X
Copy          CTRL+C

Those should have commands, so we can use key-ID.

Move to To    SHIFT+T <-- those could be created for the occasion :-)
Move to Cc    SHIFT+C
Move to Bcc   SHIFT+B

I like the idea of having real keyboard shortcuts for these functions, so you can select pills, press the shortcut, and pills move. Selecting the shortcuts needs a bit more thought:

• We might want to use the same keyboard shortcut for just displaying the address row when no pills are selected, so Shift+* isn't ideal, because we'd want the shortcut to work from anywhere in composition, which in turn requires unique and unused shortcuts. As shortcuts, these will not be localised, which is also great for support and cross-national use cases (as opposed to access keys, which are always localized).
• I think I a saw Ctrl+Shift+* on Gmail for our use case here (To/CC/BCC), so we might want to use that.
• We'd also want the same shortcut to work from contacts side bar with contacts selected (which will be perfect for the UI and behaviour refactoring which I envision for bug 271917, which is much more exposed now with grouped recipients).

Comment 5

[Thomas D. (:thomas8)]

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

I wasn't suggesting to remove the accesskeys, I was simply stating a personal preference.

Ah sorry, so the misunderstanding was on my side (I've edited my comment accordingly). +1 for aleca's personal preference!
Yes, definitely having real keyboard shortcuts is much more convenient then having to trigger the context menu and then use access keys, plus keyboard shortcuts will be the same internationally.

Comment 6

[Thomas D. (:thomas8)]

Attachment: 1647654_pillContextShortcutsDisplay.diff

@Geoff, could you review this? Tia.

@Alex, I'll file a screenshot in my next comment.

• I used GrayText color for shortcut display to reduce the noise of the shortcut info on the menuitems.
• I also added a bit more whitespace in front of the shortcut texts to disentangle them from the menuitem labels. Whitespace rocks! ;-)

I think we should do the same for all shortcut displays also in main menus (color and distance). What do you think?

• I have also changed the IDs of cut/copy/paste on this context menu, because we cannot have the same IDs multiple times in the same document. Could you check if rectifying that error has any adverse consequences?

@Richard, could you pls double-check the css and effects on other OS. Strangely this only changes some of the shortcut keys on main menu items, but not all. E.g. on composition's Edit menu, cut/copy/paste still have black shortcut text instead of GrayText.

Getting two shortcuts to display on one menuitem was entirely non-trivial, mixing XUL and Fluent, but after tweaking my getPrettyKey() function a bit and adding some <keys> for display purposes, I was able to continue exploiting ShortcutUtils.prettifyShortcut(keyElement) for correct localized display of shortcut keys.

Combining rest parameters and destructuring assignment came in handy, allowing for lean callers:

getPrettyKey("key_ID1");
getPrettyKey("key_ID1", "key_ID2");
function getPrettyKey(...[keyId1, keyId2]) {...}

Comment 7

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: Shortcuts displayed on pill context menu (with patch applied)

Richard Leger (reporter), how do you like this?

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 9159248 [details] [diff] [review]
1647654_pillContextShortcutsDisplay.diff

+/* Override toolkit menu.css style for shortcut keys displayed on menuitems. */
+.menu-accel {
+	color: GrayText !important;
+	margin-inline-start: 2em !important;
+}

Please don't do this globally. Applying this in messengercompoe.css as
#emailAddressPillPopup menu-accel {} is enough.

Also don't set a colour as this isn't system standard and doesn't work well on Mac. We also don't use tabs, two spaces is our standard. And at least, the `!important` shouldn't be needed.

Comment 9

[Richard Leger]

(In reply to Thomas D. from comment #7)

Created attachment 9159258 [details]
Screenshot 1: Shortcuts displayed on pill context menu (with patch applied)

Richard Leger (reporter), how do you like this?

I am touched! ...and me who thought Alessandro was the UX specialist ;-)
Probably a question for him :-)
That said, I will take up on the challenge and provide you with my feedback...

Looks good to me.

Perhaps few suggestions if I may:

• change "Backspace" into "Bksp" ? To make it less looking like an invader in the menu...
• use " | " to replace " / " might be better... "|" is often being interpreted as "OR" keeping space around it... and it may look better...

Those are details really up to you guys...

Noticed though that shortcut are missing for Move options...
Those may be the most handy ones considering that drag/drop feature may be delayed to after ESR version possibly... if at least the shortcut could be created and enabled for Move option in ESR version it would be a must! It would ease life :-)

Comment 10

[Richard Leger]

Also a detail but why not just to simplify and say "Edit" instead of "Edit Address" in the context it is understandable what "Edit" refers to...

Comment 11

[Thomas D. (:thomas8)]

Attachment: Screenshot 2: Shortcuts on pill context (with `|` as a separator)

Alex, what do you think of using | as shortcut separator? I like it (see below for the details)...

(In reply to Richard Leger from comment #9)

I am touched! ...and me who thought Alessandro was the UX specialist ;-)

Undisputedly so! No worries, I asked for Alex' ui-review on the patch so that will easily override reporter's feedback ;-)
That said, we like listening to our users, and while user feedback may have its caveats, it's most important nonetheless!

Looks good to me.

Your idea - thanks! :-)

Perhaps few suggestions if I may:

• change "Backspace" into "Bksp" ? To make it less looking like an invader in the menu...

Yeah, I see what you mean, but "Bksp" would be cryptic and meaningless to most users, so we can't do that. These are localized key translations which I'm retrieving automatically from the system, so we can't even reasonably change that.

• use " | " to replace " / " might be better... "|" is often being interpreted as "OR" keeping space around it... and it may look better...

Interesting! Nice and neat. I like it, maybe even better than the slash /, because | looks calm and less noisy. Users may not know about the OR meaning of |, but even if they just take it as a vertical separator, we're still good.
Alex, what do you think?

Noticed though that shortcut are missing for Move options...

You fox!! Getting those alternative shortcuts to display was a lot harder than expected (if it looks easy in the patch code, that's because it's the n-th iteration after navigating all the problems and optimizing the code), so I figured that I've invested enough of my volunteer time for now ;-)

Those may be the most handy ones considering that drag/drop feature may be delayed to after ESR version possibly... if at least the shortcut could be created and enabled for Move option in ESR version it would be a must! It would ease life :-)

Good point! Efficiency is part of our brand core. I'd love that, and I understand from comment 1 that Alex also endorses the idea of adding real keyboard shortcuts for Move-to-* actions. I've suggested some implementation details for consideration at the end of my comment 4. I guess we could do that in a followup bug after first landing what we already have here.

Comment 12

[Thomas D. (:thomas8)]

(In reply to Richard Leger from comment #10)

Also a detail but why not just to simplify and say "Edit" instead of "Edit Address" in the context it is understandable what "Edit" refers to...

If you're trying to be more nitpicking than me, I'm ready for the challenge ;-p
Jokes aside, having "Edit Address" was a deliberate disambiguation because it's also possible to take the pill as a representation of the underlying contact, so it may not be clear if "Edit" would edit just the recipient pill or the contact itself. We might even want to add "Edit contact" to this menu at a later stage if feasible, similar to what we have in message reader.
Fair enough, and thanks, lots of good details make up good software!

Comment 13

[Magnus Melin [:mkmelin]]

We've been pretty consistent with NOT putting shortcuts in context menus. I think we don't really want to go there...

Comment 14

[Alessandro Castellani [:aleca]]

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

We've been pretty consistent with NOT putting shortcuts in context menus. I think we don't really want to go there...

I think we should reconsider and open a little bit the conversation to experiment and explore the idea.

Visible shortcuts in context menus are great for the discoverability of those shortcuts that otherwise would be relegated only to the menubar, which offers a disconnected experience from interacting with the current element.

We need find a good balance and define tight guidelines to not bloat every context menu with unnecessary shortcuts, but in some cases where these type of actions are commonly repeated daily, we should definitely consider it.

Comment 15

[Thomas D. (:thomas8)]

Some thoughts:

• UX-efficiency is key for the brand core of Thunderbird: "Make email easier".
• Efficient keyboard access is key for UX-efficiency, especially for enterprise use cases where large message volumes must be handled every day. If you lose just 20 seconds on each one of 100 messages per day by navigating menus and searching for click targets, that's 2000 seconds = 33 minutes lost per day, that's almost 7% of 8 daily working hours wasted. For repetitive tasks, keyboard can be quite literally a thousand times faster.
• Advanced keyboard control is one thing that makes Thunderbird stand out out as a desktop email client - any webmailer can offer some clickable UI.
• UX-discovery is key for keyboard efficiency - Obviously, users can only benefit from keyboard shortcuts if they know about them.
• Contextual disclosure of keyboard shortcuts is key for UX-discovery - we can safely assume most users will never try to edit a pill going through composition's main menu, which in this case is not even possible:
  • Pill commands have not been linked with the main command controller, so they are not accessible from main menu (select a pill, open edit menu and try: copy/cut/paste/delete/select-all etc. - nothing! That's a bug.)
  • We don't even have entries for all pill commands in main menus (that's an omission).

In a nutshell:

• Contextual UX-discovery of keyboard shortcuts --> Keyboard efficiency --> UX-efficiency --> Thunderbird brand core: "Make email easier"
• ...which is key especially for enterprise use cases / large message volumes
• In view of the pending release, pill context menu is currently the only feasible way of disclosing these keyboard shortcuts (nothing in main menu).
• Context menu is a great way of making keyboard shortcuts discoverable exactly when they are needed.

Hints for implementation:

• Reduce noise by visually de-emphasizing keyboard shortcut display on menu items (e.g. grey color) and sufficient horizontal distance (as seen in screenshot of attachment 9159414 [details]).
• Experiment with disclosure variations like tooltips, or only show the keyboard shortcut for the hovered item.
• Long-term: consider pref to display keyboard shortcuts in context menu or not (e.g. pref toggles attribute, CSS shows/hides the shortcuts)

Comment 16

[Richard Leger]

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

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

We've been pretty consistent with NOT putting shortcuts in context menus. I think we don't really want to go there...

I think we should reconsider and open a little bit the conversation to experiment and explore the idea.

It does not have to be genetalised to all context menus but for pills management in compose windows it make sense. Hope you would considere enhancement... help discoverability/accessibility... even for advance user that may need a "reminder"

Visible shortcuts in context menus are great for the discoverability of those shortcuts that otherwise would be relegated only to the menubar...

The menu bar is disappearing meaning shortcuts hints with it at the moment... context menu(s) become good contender to hold them instead...

We need find a good balance and define tight guidelines to not bloat every context menu with unnecessary shortcuts, but in some cases where these type of actions are commonly repeated daily, we should definitely consider it.

Exactly... please consider... ;-)'

Comment 17

[Thomas D. (:thomas8)]

Attachment: 1647654_pillContextShortcutsDisplay.diff (Variation 1: Display shortcut only for hovered/focused menuitem)

Shortcut disclosure, Variation 1 (see screenshot in my next comment):
Display shortcuts only for the currently hovered or focused menuitem.

• keeps the context menu UI clean ("whitespace")
• reveals one shortcut at a time, exactly when you're touching it / focused on it
• inline display (less noisy than tooltips)

Not bad. I think I like it.

Comment 18

[Thomas D. (:thomas8)]

Attachment: Screenshot 3: Variation 1: Display shortcuts only for hovered/focused menuitem

Comment 19

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #8)

Also don't set a colour as this isn't system standard and doesn't work well
on Mac.

Could you attach a screenshot of "doesn't work well on MAC"?
Works perfectly on Windows, including GrayText color for the shortcut display.

We also don't use tabs, two spaces is our standard.

Copied the entire CSS rule from TB Developer Tools - too bad it comes with tabs... can that be changed by way of settings?

And at least, the !important shouldn't be needed.

Yes, not needed for the color, but needed for margin-inline-start.

Comment 20

[Richard Leger]

(In reply to Thomas D. from comment #17)

Created attachment 9159566 [details]
1647654_pillContextShortcutsDisplay.diff (Variation 1: Display shortcut only for hovered/focused menuitem)

Shortcut disclosure, Variation 1 (see screenshot in my next comment):
Display shortcuts only for the currently hovered or focused menuitem.

• keeps the context menu UI clean ("whitespace")
• reveals one shortcut at a time, exactly when you're touching it / focused on it
• inline display (less noisy than tooltips)

Not bad. I think I like it.

Nice compromise/idea... me like it! Should make everyone happy...
Then the invader can remain whole without cluttering area :-)

Questions:
What happens when context menu is opened and no item selected/mouse hover?
Is there always an item selected by default or does menu width adjust to size of item list to avoid just empty space after text in case all shortcuts hidden? At the same time too much animation/flickering may be annoying over time... just wondering...

Comment 21

[Thomas D. (:thomas8)]

Attachment: Screenshot 3b: Variation 1: (Initial state) Display shortcuts only for hovered/focused menuitem

(In reply to Richard Leger from comment #20)

Nice compromise/idea... me like it! Should make everyone happy...

Thanks! Indeed.

Then the invader can remain whole without cluttering area :-)

Yes, longer Del | Backspace shortcut will only show when Delete item is hovered/focused.

Questions:
What happens when context menu is opened and no item selected/mouse hover?
Is there always an item selected by default or does menu width adjust to size of item list to avoid just empty space after text in case all shortcuts hidden? At the same time too much animation/flickering may be annoying over time... just wondering...

See screenshot of initial menu state (not hovered / not item focused).
If context menu is opened with keyboard (context menu key, or Shift+F10 on Windows), first item should be selected, but we haven't finetuned that yet.
Yes, we need to reserve a bit of menu whitespace for the shortcuts to appear without the menu suddenly resizing. But imho it still looks good, not disproportionate. We can't have our cake and eat it. Principled compromise is part and parcel of UX design.

Over to Alex.

Comment 22

[Richard Marti (:Paenglab)]

As I wrote before, don't use GrayText, let the default colour for consistency with the other shortcuts in the other menus..

The popup with the big empty space looks a bit weird.

Comment 23

[Alessandro Castellani [:aleca]]

Lots of things to unpack here, thanks for working on this, but let's be careful of the type of implementation we want to have.

Revealing shortcuts on hover is a big NO for me.
Context menus should show all the available elements, alongside their shortcuts, and not hide them behind an hover effect. That's extremely strange and defeats the purpose of easy discoverability offered by the context menu.

We shouldn't list double shortcuts
Other than looking visually wrong and cluttered, they don't work the same for all the platform.
F2 is a Windows specific shortcut, not natural for other platform, and also not working on default settings on macOS where functions keys are handled by the OS differently.
Also, Backspace is not a thing on mac as well due to different keyboard organization.
Same thing for all the other shortcuts which use a modifier. CTRL is not a thing on macOS, which uses CMD.

We should have platform specific shortcuts thanks to the built-in Fluent feature.

Shortcut color
Instead of forcibly applying the textGrey color, let's keep the default color inherited from the menu and decrease importance with a 0.8 opacity value.

Comment 24

[Thomas D. (:thomas8)]

Comment on attachment 9159258 [details]
Screenshot 1: Shortcuts displayed on pill context menu (with patch applied)

f+ Richard Leger (reporter) from comment 9

Comment 25

[Thomas D. (:thomas8)]

Comment on attachment 9159568 [details]
Screenshot 3: Variation 1: Display shortcuts only for hovered/focused menuitem

f+ Richard Leger (reporter) from comment 20