1667614 - Implement Shift+Home keyboard shortcut from address input for selecting all pills of the address row.

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

With cursor in first position of an empty or whitespace-only address input, Shift+Home should select all pills contained in the address row. I keep trying this keyboard shortcut all the time, and it's so boring when it doesn't work. Time to implement it! :-)

Comment 1

[Alessandro Castellani [:aleca]]

Is this a commonly used keyboard shortcut?
We already have the CTRL+A implemented that does the same exact thing when pressed twice (or does not?).
I don't think we should implement and support multiple shortcuts that do the same exact thing as it adds to the burden of maintaining.

On a related note, we should start implementing tests to cover all the accessibility feature we currently have for the pills. That's a more impending priority IMHO.

Comment 2

[Thomas D. (:thomas8)]

Attachment: 1667614_addressInputShiftPos1.diff

Implement Shift+HOME from address input to select all pills within the addressing row. Shift+Ctrl+HOME will work, too.
Deliberately not yet handling Shift+HOME starting out from a pill, that's for another day.

While we are here, nitfix a hard-coded ID reference to the addressing area and nitfix that Alt+Backspace from address input should not delete pills.

Comment 3

[Thomas D. (:thomas8)]

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

Is this a commonly used keyboard shortcut?

Yes. Plain vanilla standard keyboard shortcut both for text and items, and pills have both notions.

We already have the CTRL+A implemented that does the same exact thing when pressed twice (or does not?).
I don't think we should implement and support multiple shortcuts that do the same exact thing as it adds to the burden of maintaining.

??? Come on... Convenience and ux-efficiency matters, we don't want to fail users' muscle memory for nothing. We offer several ways of killing the cat all over in Thunderbird, that's how we serve different users with different needs. No maintenance burden. 2 lines of code which will just continue to sit where we put them now. If anyone is going to touch that, it's most likely me: For the same reasons, we also want Shift+Pos1 starting out from any pill (where Ctrl+A won't do the trick).

On a related note, we should start implementing tests to cover all the accessibility feature we currently have for the pills. That's a more impending priority IMHO.

I agree that tests are great, but stuff that just works as users can expect it from widely known standards since time immemorial is even greater.

Comment 4

[Thomas D. (:thomas8)]

Btw, @aleca, pls remember your comment on meta bug 1602397, where you actually asked me to implement Shift+HOME, only that you accidentally mistyped that as Shift+END.

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

It'd be nice to focus on the SHIFT implementation here with the following:

• Shift + Click on a pill will focus all the pills between a previously focused pill and the currently clicked pill.
• Shift + END key should select all pills between a previously focused pill and the last available pill.
• Shift + END HOME key should select all pills between a previously focused pill and the first available pill.

Thomas, would you be able to handle this?

Comment 5

[Thomas D. (:thomas8)]

Of course whereever I said POS1 (Position1, the German key label for HOME), I really mean HOME (I've fixed that in my comments).

Comment 6

[Thomas D. (:thomas8)]

https://defkey.com/what-means/shift-home lists 49 programs using this shortcut, and they forgot Windows Explorer and probably more, because all navigation keys, if combined with SHIFT, work for continuous text/item selection, everywhere.

Comment 7

[Thomas D. (:thomas8)]

Fyi, https://en.wikipedia.org/wiki/Table_of_keyboard_shortcuts seems to have a pretty comprehensive cross-OS overview (found just now, haven't checked for quality yet).

They list HOME/END for "Select/move to first/last item in selected widget", whereby selection always requires Shift modifier, so that entry happens to be inaccurate and incomplete in their table.

Comment 8

[Thomas D. (:thomas8)]

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

We already have the CTRL+A implemented that does the same exact thing when pressed twice (or does not?).

Shift+Home: Select all pills from focus point (cursor in address input OR a focused pill) to the beginning (which might be all pills, or less).
Ctrl+A: Select all pills of focused address row.
Ctrl+A, Ctrl+A: Select all pills of all addressing rows (i.e. all pills of the entire addressing area).

Comment 9

[Alessandro Castellani [:aleca]]

Thanks for the details and analysis of the implementation.
I don't know why but my initial understanding was that you wanted to implement this for those extra headers that don't use the pills, my bad.
This makes sense.

Comment 10

[Alessandro Castellani [:aleca]]

Comment on attachment 9178054 [details] [diff] [review]
1667614_addressInputShiftPos1.diff

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

Thanks for taking care of this.
Let's update the commit message with Shift+Home instead of Shift+Pos1.

A little comment nit in the code, and I'm gonna ping Magnus for a tiny coding style feedback.

::: mail/base/content/mailWidgets.js
@@ +2754,5 @@
>          if (sourcePill) {
>            sourcePill.setAttribute("selected", "selected");
>          }
> +        if (event.key == "Home" && !sourcePill) {
> +          // Shift+Home from address input

nit: period at the end.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +604,5 @@
>          event.preventDefault();
>  
>          let previous = input.previousElementSibling;
>          if (previous && previous.tagName == "mail-address-pill") {
> +          input.closest("mail-recipients-area").selectPills(previous);

I always knew that getElementById() was usually faster than any other selector, and since we're using that ID only here I'm not sure changing this brings any benefit other than styling.
Let's ask Magnus since he has a better overview of the code styling we should aim for.

Comment 11

[Thomas D. (:thomas8)]

Comment on attachment 9178054 [details] [diff] [review]
1667614_addressInputShiftPos1.diff

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

Let me explain the motivation of removing an absolute ID from what is essentially the inner code of the <mail-recipient-area> custom element...

```
::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +604,5 @@
>          event.preventDefault();
>  
>          let previous = input.previousElementSibling;
>          if (previous && previous.tagName == "mail-address-pill") {
> +          input.closest("mail-recipients-area").selectPills(previous);
```

My change isn't motivated by micro-performance, it's motivated by avoiding hard-coded IDs and having self-contained functionality in the <mail-recipient-area> custom element. Please note:
* addressingWidgetOverlay.js should probably not even exist any more. The corresponding addressingWidgetOverlay.xul is gone, and it's just totally confusing to have addressing area code scattered all over the place. Alex and I certainly know...
* addressInputOnBeforeHandleKeyDown() and other handlers are inherently part of the <mail-recipients-area> implementation, so they should live in mailWidgets.js, just like the rest of the functionality which is already there.
* The code of the <mail-recipients-area> custom element should be self-contained and using relative references to its own sub-elements. Which ID the <mail-recipient-area> happens to have in our main composition document should not matter for the internal workings of the custom element. Iow, code inside the element should never depend on the outside ID of the element which it cannot control.
* So this is all about having self-contained inner code for the functionality of a custom element which won't break or need unnecessary code adjustments just because someone decides that the ID in the layout document should change. That's a matter of code styling principles indeed.
* Iirc, Magnus actually recommended avoiding hard-coded references when we were initially working on the pills implementation.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9178054 [details] [diff] [review]
1667614_addressInputShiftPos1.diff

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

Yep, getElementById is faster, but I don't think it matters here and it's more clear and self confined to do as proposed

Comment 13

[Thomas D. (:thomas8)]

Attachment: 1667614_addressInputShiftPos1.diff

Add a dot in comment.

Comment 14

[Thomas D. (:thomas8)]

We should also land this on TB78, to polish the addressing area transition experience from TB 68.
In TB 68, it was possible to type multiple comma-separated addresses into a single input and select them with Shift+Home (only before pressing Enter).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c5e1348f8afb
Implement Shift+Home keyboard shortcut for address inputs to select all pills. r=aleca

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9178758 [details] [diff] [review]
1667614_addressInputShiftPos1.diff

Pills polish

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9178758 [details] [diff] [review]
1667614_addressInputShiftPos1.diff

[Triage Comment]
Approved for beta

Comment 18

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/a31bebc3fbd2

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/a31bebc3fbd2

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9178758 [details] [diff] [review]
1667614_addressInputShiftPos1.diff

[Triage Comment]
Approved for esr78

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/f5d08cc96ad8