1712425 - It's too easy to delete CC/BCC/Reply-To fields

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[ddl-github]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:88.0) Gecko/20100101 Firefox/88.0

Steps to reproduce:

• Added a CC field to the message compose window.
• Added several addresses to the CC field.
• Deleted addresses by backspacing over them.
• Pressed backspace again in the empty field.

Actual results:

The CC field is removed.

Expected results:

Two suggestions for alternate/better behaviour:

• Pressing backspace does nothing when the field is empty. Rationale: if field removal is truly desired by the user, there is already a removal "X" button to the left of the field.
• Pressing backspace in an empty field moves the focus to the "X" button; one more backspace removes the field.

Comment 1

[Thomas D. (:thomas8)]

(In reply to ddl-github from comment #0)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:88.0) Gecko/20100101 Firefox/88.0

Steps to reproduce:

• Added a CC field to the message compose window.
• Added several addresses to the CC field.
• Deleted addresses by backspacing over them.

More precisely, you backspaced over them one by one (at least for the last one), probably a bit fast and without much attention, hence your surplus key press:

• Pressed backspace again in the empty field.

Well, de facto that's a user error - there's no reason to press backspace again on an empty field after all recipient pills and all input text is already deleted. Personally, I've never had a problem with this; I find this shortcut extremely convenient and I'm using it every day.

That said, I can sympathize with reporter's scenario, as I guess we've all done staccato deletions sometimes without paying attention; then finding the entire row gone may involve an element of surprise. So we may look at this in terms of ux-error-prevention.

Here's a smart solution: On an already empty address row, require a dedicated(!) long keypress of Delete or Backspace to remove the row.

• Effective ux-error-prevention for reporter's scenario of surplus staccato deletions
• As before, deleting row contents with a long keypress will not remove the row ("safety brakes").
• Preserve the ux-efficiency and convenience of this long-standing feature: easy, hassle-free row removal via keyboard.

I've tried this and it works very well. I have a patch.

---

Expected results:
Two suggestions for alternate/better behaviour:

• Pressing backspace does nothing when the field is empty. Rationale: if field removal is truly desired by the user, there is already a removal "X" button to the left of the field.

Too clumsy via keyboard for this everyday action. Shift+Tab, Enter is pretty inconvenient and needs a lot of user reflection and attention to get that right.

• Pressing backspace in an empty field moves the focus to the "X" button; one more backspace removes the field.

Imho, that would be pretty unusual/unintuitive and would probably make the problem worse.

Comment 2

[Thomas D. (:thomas8)]

Attachment: 1712425_longKeypressAddressRowRemoval.diff

Patch: On an already empty address row, require another dedicated long keypress of Delete or Backspace to remove the row.

• Effective ux-error-prevention for reporter's scenario of surplus staccato deletions
• As before, deleting row contents with a long keypress will not remove the row ("safety brakes").
• Preserve the ux-efficiency and convenience of this long-standing feature: easy, hassle-free row removal via keyboard.

Comment 3

[Thomas D. (:thomas8)]

Comment on attachment 9224005 [details] [diff] [review]
1712425_longKeypressAddressRowRemoval.diff

Some flaws slipped in.

Comment 4

[Thomas D. (:thomas8)]

Attachment: 1712425_longKeypressAddressRowRemoval.diff

Rebased patch.

On an already empty address row, require another dedicated long keypress of Delete or Backspace to remove the row.

• Effective ux-error-prevention for reporter's scenario of surplus staccato deletions
• As before, deleting row contents with a long keypress will not remove the row ("safety brakes").
• Preserve the ux-efficiency and convenience of this long-standing feature: easy, hassle-free row removal via keyboard.

Comment 5

[Alessandro Castellani [:aleca]]

I'm not sure about this.
I stumbled upon the same issue the reporter wrote and it's kind of annoying.
I have this habit of hitting backspace multiple times, or keeping it pressed, to be sure the entire field is properly cleared, so the field closing happens fairly often for me.

I'm slightly questioning the "utility" of this feature.
What's the real advantage of being able to close an extra field with delete or backspace other than clearing the UI?
There are some scenarios that this feature might be useful, but the accidental closures might be more common than we expect and this might easily get annoying.

I need a bit more time to think about this and see if it makes sense keeping this or the benefits don't justify the potential user errors and checks we're putting in place.

Comment 6

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani [:aleca] from comment #5)

I'm not sure about this.
I stumbled upon the same issue the reporter wrote and it's kind of annoying.
I have this habit of hitting backspace multiple times, or keeping it pressed, to be sure the entire field is properly cleared, so the field closing happens fairly often for me.

Good news for you: That's exactly what we are fixing here!
Please note: "keeping backspace pressed to be sure the entire field is properly cleared" will not remove the row (neither before nor after this patch).

I'm slightly questioning the "utility" of this feature.

The utility is in the unbeatable keyboard efficiency and convenience. You'll start to know when you need to do this like 100 times per day.
The only keyboard alternative, Shift+Tab, Enter, is pretty inconvenient and needs a lot of user reflection and attention to get that right.

What's the real advantage of being able to close an extra field with delete or backspace other than clearing the UI?

Clearing the UI is a real advantage:

• Recover vertical space (the header area already uses a lot of that, minimum 25% with just 1 row each of To and Bcc).
• Declutter the UI from unneeded, distractive inputs (empty fields are inherently asking to be filled...)

There are some scenarios that this feature might be useful, but the accidental closures might be more common than we expect and this might easily get annoying.

Exactly! We are here to fix that annoyance...
Have you tried the patch in action? Accidental closures are no longer possible with this patch:

• surplus staccato deletions (multiple short keypresses) will no longer remove the row (which fixes the current accidents)
• long keypress from erasing text or pills will not remove the row ("safety brakes", just like before)
• only when you explicitly start(!) another long(!) keypress on an already empty(!) row, that will remove the row (this is new).

Put simple - you can safely delete your contents in whichever style you prefer (multiple, random short keypresses or a long keypress until field is cleared), then you need to let go(!), and explicitly do another long keypress on the empty field to remove it.
Imho that cannot happen by accident except with compulsive disorders or trying to send a Morse code... ;-)

Comment 7

[Thomas D. (:thomas8)]

Please note: Regarding ux-efficiency, this is the reverse case of:

• Bug 1667692 - Implement keyboard shortcuts to show and focus important address rows (To, CC, BCC): Ctrl+Shift+T, Ctrl+Shift+C, Ctrl+Shift+B (like gmail)
• Bug 1616514 - Provide a way of showing Cc and Bcc addressing rows automatically when starting to write a message (esp. for enterprise)

In those bugs, we've learned from high-volume professional email users that even the smallest extra click in frequent scenarios can be extremely annoying and time-consuming as it interrupts high efficiency keyboard workflows. Users were explicitly requesting an easier keyboard-accessible way to open their Cc or Bcc fields either case-to-case (bug 1667692) or permanently (bug 1616514).

In this bug, it's about ux-efficiency for the reverse case: easy, hassle-free keyboard access to close the Cc or Bcc fields case to case. I guess we allow closing the fields for a reason... This is a daily scenario for me, and the shortcut is a perfect no-brainer time-saver.

Comment 8

[Thomas D. (:thomas8)]

(In reply to Thomas D. (:thomas8) from comment #6)

The utility is in the unbeatable keyboard efficiency and convenience. You'll start to know when you need to do this like 100 times per day.
The only keyboard alternative, Shift+Tab, Enter, is pretty inconvenient and needs a lot of user reflection and attention to get that right.

Fwiw: Bug 1713426 - [x] Close button to remove Cc/Bcc address rows is not keyboard-accessible (90.0a1 (2021-05-28) (64-bit))

Comment 9

[Alessandro Castellani [:aleca]]

Comment on attachment 9224074 [details] [diff] [review]
1712425_longKeypressAddressRowRemoval.diff

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

Overall this is a good implementation and I think it'll benefit the users.
There's a small linting issue to fix.

I'm sorry if I'm being a bit picky with the comments, but I think we should avoid being overly verbose and using strange structures.
Comments should be simple and straightforward to make them easy to read and understand.
Even if I think my English is decent, I had to read these comments 2 or 3 times to properly understand what was happening, and reading the code turned out to be easier to understand :D

::: mail/components/compose/content/MsgComposeCommands.js
@@ +7861,5 @@
>   * @param {Event} event - The DOM Event.
>   */
>  function subjectKeyPress(event) {
> +  // If long (repeated) Delete keypress which ended up here from deletion of
> +  // previous row, cancel the event and do nothing: prevent unwanted deletion.

What about: "Prevent the event if a repeated Delete keypress event is registered here and is coming from a continuous keypress from a previous row."
Not sure if that sounds good, but I'm trying to simplify a bit the comments to make them a bit more natural to read and void parenthesis, colons, semi colons, etc. Let's make them more colloquial and easy to digest.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +16,5 @@
>  
>  // Keep track of the height of the addressing header if the user manually
>  // resizes it.
>  var kAddressingHeaderHeight;
> +var gPreventRowDeletionKeysRepeat = false;

nit: we should have a comment to explain this boolean variable.

@@ +691,5 @@
> +      ) {
> +        // If repeated Backspace keydown event and flag is set, cancel the event.
> +        // Flag will be set for surplus repeated keydown events from removing
> +        // subsequent address row via long Backspace keydown, or for input
> +        // event which removes all input text except whitespace.

This comment is a bit too much.
The explanation of the flag should be written when we declare it as a var at the top of the document, not here.
This comment should be more streamlined, like: "Prevent the event if the it's a repeated backspace keydown previously triggered from another field" (or something like that).

@@ +696,5 @@
> +        event.preventDefault();
> +        break;
> +      }
> +      // For every first keydown (not repeated), enable repeated deletion.
> +      // Otherwise it's already enabled; then we're not changing it here.

This comment sounds a bit weird and confusing.
What about: "Enable repeated deletion if we're registering a non repeated first keydown event."
The second line is a bit confusing since it refers to another area of the code.

@@ +705,5 @@
>          input.selectionStart + input.selectionEnd ||
>          event.altKey
>        ) {
> +        // Adress row with content, or cursor not at position 0, or Alt modifier
> +        // key was used: break to proceed with default behaviour of the key.

"Break the proceed but still allow default behavior if the row has content, or the cursor is not at position, or the Alt modifier is pressed."

@@ +724,5 @@
>          );
>        if (targetPill) {
> +        if (event.repeat) {
> +          // Prevent navigating into pills for repeated keydown from the middle
> +          // of whitespace. You first need to let go of the key and re-press it.

The last part of this comment is a bit redundant. We shouldn't write instructions in the comment but only explain the reason for the condition.

@@ +737,5 @@
>          break;
>        }
>  
> +      // No pill found: address row is empty except whitespace - check for long
> +      // keydown of Backspace to remove the row.

Let's be more conversational and avoid colon and dash.

@@ +773,4 @@
>        if (
> +        !event.repeat ||
> +        input.value.trim() ||
> +        (input.selectionStart + input.selectionEnd) ||

nit linting error: since this is not falsy anymore the parenthesis are not necessary.

Comment 10

[Thomas D. (:thomas8)]

Attachment: 1712425_longKeypressAddressRowRemoval.diff

Thanks for the review! All code comments improved along comment 9.
Two more nits fixed around subject field to prevent unwarranted ongoing deletion when cursor was at position 0 of subject field before and a previous row gets deleted via the shortcut.

Comment 11

[Thomas D. (:thomas8)]

Attachment: 1712425_longKeypressAddressRowRemoval.new.diff

For consistency, this patch applies the improved behavior also to address rows generated via mail.compose.other.header pref.

In the process, unify and relocate handling of the input event for all types of address rows to avoid code duplication and keep similar and related event handlers conveniently in one place.

Comment 12

[Alessandro Castellani [:aleca]]

Comment on attachment 9225450 [details] [diff] [review]
1712425_longKeypressAddressRowRemoval.new.diff

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

Looks good, thanks.
Just a couple of nits for the comments.

I'll set this to `leave-open` as I want to implement a couple of tests to cover this feature.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +579,5 @@
>      case "Delete":
> +      if (event.repeat && gPreventRowDeletionKeysRepeat) {
> +        // Prevent repeated deletion keydown event if the flag is set (after
> +        // removing adjacent address row via long Backspace or Delete keyboard
> +        // shortcut, or after an input event which has removed all input text).

No need to repeat the conditions that set the flag to true as you already described this thoroughly above the variable at the beginning of the file.
Here a simple: "Prevent repeated deletion keydown event if the flag is set." is enough.

@@ +606,5 @@
>        }
> +      // Hide the address row if it is empty except whitespace, repeated
> +      // deletion keydown event occured, and it has an [x] button for removal.
> +      // Prevent further unwarranted deletion in the adjacent row which will
> +      // receive focus while the key is still down.

Maybe we should move things around to have the comments above the code they're referencing:
```
event.preventDefault();

// Prevent further unwarranted deletion in the adjacent row which will
// receive focus while the key is still down.
gPreventRowDeletionKeysRepeat = true;

// Hide the address row if it is empty except whitespace, repeated
// deletion keydown event occured, and it has an [x] button for removal.
hideAddressRow(input, event.key == "Backspace" ? "previous" : "next");
```

Comment 13

[Thomas D. (:thomas8)]

Attachment: 1712425_longKeypressAddressRowRemoval.new.diff

OK. All comment nits of comment 12 addressed. So we're done :-)

(In reply to Alessandro Castellani [:aleca] from comment #12)

Review of attachment 9225450 [details] [diff] [review] ⧉[details] [diff] [review]:
I'll set this to leave-open as I want to implement a couple of tests to
cover this feature.

That's great!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5d088e70a358
Improve ux-error-prevention of removing empty address row with Delete or Backspace. r=aleca

Comment 15

[Alessandro Castellani [:aleca]]

Grabbing this so I can implement a few tests to cover the feature.

Comment 16

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani [:aleca] from comment #15)

Grabbing this so I can implement a few tests to cover the feature.

Yeah, but if you want your name permanently on the bug, then maybe it's better to add tests in a new bug.
Otherwise I'll just grab this back after you're done with the test, because I've done all the feature work and I do want credit for that... ;-)

Comment 17

[Alessandro Castellani [:aleca]]

I don't care about my name on the bug :P
I just assigned it to me so I have it on my list for ease.
I'll reassign it to you once the tests land, probably later today.

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: Bug 1712425 - Implement tests to cover the press and hold Backspace key event in the compose addressing fields. r=mkmelin

Comment 19

[Alessandro Castellani [:aleca]]

Try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e1b92f7329a4ff538a07bcf197103b5d89fca577

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3a986f6343b7
Implement tests to cover the press and hold Backspace key event in the compose addressing fields. r=darktrojan

Comment 21

[Alessandro Castellani [:aleca]]

Attachment: Bug 1712425 - Implement tests to cover Delete and Backspace key event in the compose addressing fields. r=darktrojan

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/53d788748ed6
Implement tests to cover Delete and Backspace key event in the compose addressing fields. r=darktrojan