Closed Bug 1663583 Opened 9 months ago Closed 9 months ago

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

Categories

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

Tracking

(thunderbird_esr78+ fixed, thunderbird82 fixed)

RESOLVED FIXED
83 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird82 --- fixed

People

(Reporter: thomas8, Assigned: thomas8)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.

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).
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Attachment #9176579 - Flags: review?(alessandro)

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

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

See comment 1 for the behaviour restored by this patch.

Attachment #9176579 - Attachment is obsolete: true
Attachment #9176579 - Flags: review?(alessandro)
Attachment #9176586 - Flags: review?(alessandro)
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
Attachment #9176586 - Flags: review?(alessandro) → feedback+

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

Attachment #9176727 - Flags: review?(alessandro)
Attachment #9176586 - Attachment is obsolete: true
Comment on attachment 9176727 [details] [diff] [review]
1663583_restoreOtherHeaderKeyboardHandling.diff

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

Looks good, thanks
Attachment #9176727 - Flags: review?(alessandro) → review+
Target Milestone: --- → 82 Branch
Target Milestone: 82 Branch → 83 Branch

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

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED

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

[Approval Request Comment]
More address pills polish

Attachment #9176727 - Flags: approval-comm-esr78?
Attachment #9176727 - Flags: approval-comm-beta?

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

[Triage Comment]
Approved for beta

Attachment #9176727 - Flags: approval-comm-beta? → approval-comm-beta+

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

[Triage Comment]
Approved for esr78

Attachment #9176727 - Flags: approval-comm-esr78? → approval-comm-esr78+

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

Flags: needinfo?(bugzilla2007)
Flags: needinfo?(alessandro)

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

Flags: needinfo?(bugzilla2007)
Flags: needinfo?(alessandro)
Depends on: 1629144
No longer depends on: 1629144
See Also: → 1629144
Regressions: 1682147
No longer regressions: 1682147
You need to log in before you can comment on or make changes to this bug.