1663583 - Raw header fields created via mail.compose.other.header no longer handle keyboard events for navigation, row deletion etc.

Status: RESOLVED FIXED

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

Description

[Thomas D. (:thomas8)]

Unfortunately, bug 1658890, by removing autocomplete from other header fields (raw input), has also unwittingly removed all of our own keyboard event handling which only works via toolkit autocomplete:
input.onBeforeHandleKeyDown() --> addressInputOnBeforeHandleKeyDown().
Without autocomplete, this will no longer get called for other header inputs.

Here are some failures which regressed:

• Navigating from an other header row to any next row by pressing Enter fails - not nice getting stuck half way down and being forced to use Tab key
• Deleting an empty, focused other header row by pressing Backspace or Del fails

I have started to clean up the mess:
Bug 1663526 - Cleanup buildRecipientRows() to disentangle row input creation for address autocomplete vs. other/raw header inputs [Bug 1658890 followup]

As we can no longer go through autocomplete channels for other header fields, they will need their own dedicated otherHeaderOnKeyDown() function. I think I can fix that.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1663583_restoreOtherHeaderKeyboardHandling.diff

With bug 1663526 landed, let's fix this!

• restore navigating from an other header row to any next row or subject by pressing Enter
• restore deleting an empty or whitespace-only, focused other header row by explicitly pressing Backspace or Del.

Note:

• For existing whitespace-only text, cursor must be on position 0 for the trick to work, so that deleting whitespace say from right to left will first work normally without surprises (same behaviour which we have for autocomplete inputs).
• After deleting input text by holding Backspace or Del, you must first let go, then press the key again. Again, we want text deletion to first work normally without surprises of a disappearing row (same behaviour which I implemented for autocomplete inputs).

Comment 2

[Thomas D. (:thomas8)]

Yeah, there's one tiny logical error because "Del" with cursor in position 0 of whitespace-only should first eat up the whitespace before removing the row. Overlooked that when conflating "Backspace" and "Del" which are handled separately in the other function.
Edit: This is how it also works for autocomplete headers. Selected whitespace will get deleted normally with DEL, but if your cursor is in position 0 (meaning you won't even see the following whitespace), we just remove the row. I guess that's ok, because it might feel unresponsive if we first delete the invisible whitespace, and you wouldn't typically position your cursor at position 0 to delete it from front (but rather select it first, or use backspace). It's very hard and unlikely to end up with whitespace only anyway...
We could also copy the trick that starting with a space isn't possible...

Comment 3

[Thomas D. (:thomas8)]

Attachment: 1663583_restoreOtherHeaderKeyboardHandling.diff

Polish: Prevent typing space as first character when input is empty or whitespace-only.

See comment 1 for the behaviour restored by this patch.

Comment 4

[Alessandro Castellani [:aleca]]

Comment on attachment 9176586 [details] [diff] [review]
1663583_restoreOtherHeaderKeyboardHandling.diff

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

This is good, thanks for taking care of this.
Just a couple of small fixes.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +549,5 @@
> +    case " ":
> +      // Prevent the typing of a blank space as a first character.
> +      if (!input.value.trim()) {
> +        event.preventDefault();
> +      }

This doesn't work as the white space was already typed as we're not listening to a "before" keydown, but after the keydown.
We should clear the field instead of preventDefault which doesn't do anything.

@@ +554,5 @@
> +      break;
> +    case "Enter":
> +      // Move focus to the next available element,
> +      // but not for Ctrl/Cmd+[Shift]+Enter keyboard shortcuts for sending.
> +      if (!event.ctrlKey && !event.metaKey) {

this is good, but we should add the macos metaKey modifier also here since we forgot: https://searchfox.org/comm-central/rev/27cb9951c097a9b15f8952a79617921fdcc568da/mail/components/compose/content/addressingWidgetOverlay.js#649

Comment 5

[Thomas D. (:thomas8)]

Attachment: 1663583_restoreOtherHeaderKeyboardHandling.diff

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

Review of attachment 9176586 [details] [diff] [review] ⧉[details] [diff] [review]:
::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +549,5 @@

> > +    case " ":
> > +      // Prevent the typing of a blank space as a first character.
> > +      if (!input.value.trim()) {
> > +        event.preventDefault();
> > +      }

This doesn't work as the white space was already typed as we're not
listening to a "before" keydown, but after the keydown.

Ummm, no sir. At the time of keydown event, the new character isn't visible yet in the input, and input.value has the previous (unchanged) value. console.log (input.value); can tell. So this works exactly as expected (and as we already have it in the other function).

We should clear the field instead of preventDefault which doesn't do
anything.

preventDefault() is needed to prevent the space from being added in this event.

In addition, yes, we can clear any pre-existing whitespace, which might temporarily occur in the rare event that

• whitespace-only like "   " was pasted and focus still in the input, or
• whitespace-only like "  " is a volatile leftover from having " foo ", deleting or cutting "foo" and then remaining with " " for as long as focus remains in the input.

We only prevent entering a space and clear the previous value IF the previous input.value is whitespace-only, because we have to allow changing "foo" to " foo" or "foo ". I rewrote the comment to be more accurate.

this is good, but we should add the macos metaKey modifier also here since
we forgot

Yes.

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9176727 [details] [diff] [review]
1663583_restoreOtherHeaderKeyboardHandling.diff

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

Looks good, thanks

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/261076d7205e
Restore keydown event handling for rows from mail.compose.other.header. r=aleca

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9176727 [details] [diff] [review]
1663583_restoreOtherHeaderKeyboardHandling.diff

[Approval Request Comment]
More address pills polish

Comment 9

[Wayne Mery (:wsmwk)]

Comment on attachment 9176727 [details] [diff] [review]
1663583_restoreOtherHeaderKeyboardHandling.diff

[Triage Comment]
Approved for beta

Comment 10

[Rob Lemley [:rjl]]

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

Comment 11

[Rob Lemley [:rjl]]

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

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9176727 [details] [diff] [review]
1663583_restoreOtherHeaderKeyboardHandling.diff

[Triage Comment]
Approved for esr78

Comment 13

[Rob Lemley [:rjl]]

This doesn't apply on esr78, could you rebase please?

Comment 14

[Thomas D. (:thomas8)]

(In reply to Rob Lemley [:rjl] from comment #13)

This doesn't apply on esr78, could you rebase please?

This is marked to depend on bug 1663526, which needs to be uplifted first.

Comment 15

[Rob Lemley [:rjl]]

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