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)
People
(Reporter: thomas8, Assigned: thomas8)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
9.04 KB,
patch
|
aleca
:
review+
wsmwk
:
approval-comm-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•4 years ago
•
|
||
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
orDel
.
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
orDel
, 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 | ||
Comment 2•4 years ago
•
|
||
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...
Assignee | ||
Comment 3•4 years ago
|
||
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•4 years ago
|
||
Assignee | ||
Comment 5•4 years ago
•
|
||
(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.
Assignee | ||
Updated•4 years ago
|
Comment 6•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
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
Updated•4 years ago
|
Comment 8•4 years ago
|
||
Comment on attachment 9176727 [details] [diff] [review]
1663583_restoreOtherHeaderKeyboardHandling.diff
[Approval Request Comment]
More address pills polish
Comment 9•4 years ago
|
||
Comment on attachment 9176727 [details] [diff] [review]
1663583_restoreOtherHeaderKeyboardHandling.diff
[Triage Comment]
Approved for beta
Comment 10•4 years ago
|
||
bugherder uplift |
Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/a97887e1fb01
Comment 11•4 years ago
|
||
bugherder uplift |
Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/a97887e1fb01
Comment 12•4 years ago
|
||
Comment on attachment 9176727 [details] [diff] [review]
1663583_restoreOtherHeaderKeyboardHandling.diff
[Triage Comment]
Approved for esr78
Comment 13•4 years ago
|
||
This doesn't apply on esr78, could you rebase please?
Assignee | ||
Comment 14•4 years ago
|
||
(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.
Assignee | ||
Updated•4 years ago
|
Comment 15•4 years ago
|
||
bugherder uplift |
Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/39047ba4abd0
Assignee | ||
Updated•4 years ago
|
Description
•