1694502 - Thunderbird exits recipient address editing and pillifies prematurely when switching focus to a different program/window

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Igor R.]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

In the message composition window, enter a new address in the "To:" (or any other recipient field)
(Alternatively, with the already entered address, Right-click -> "Edit address")
Switch to a different window (or program, e.g. browser)

Actual results:

When you return to the original window, the address is no longer in the "edit" mode, "tokenized" again, so that you have to "Edit address" again every time you switch back to this window.

Expected results:

It should remain in the "edit" mode until a certain number of actions: click away, some key pressed (Esc, Enter, etc.) ..

Use case:
Very often, when you are writing to a new recipient, and you want to insert the name and the address of that person, you need to copy that information from an e-mail message (not necessarily a header, where you can just choose "Compose Message To", of if there are more than one person). Often, you have to copy the address and the name separately, sometimes rearranging the first and the last name, when copying from a directory, where they are posted as "Last; First" ->
First Last <[e-mail]>
So, you have to copy and paste more than one time, going between the windows.
This spontaneous "tokenization" of the address introduced in one of the recent versions make this process extremely painful.

I am fine with the address being converted to a token (that helps to prevent from accidentally erasing parts of the address, I guess..) - but the edit mode should not be closed until certain actions occurs, - such as switching to a different field in the same window, pressing keyboard keys, clicking with the mouse outside of that field. But switching focus to a different windows/tab shouldn't exit the edit mode.

Comment 1

[Wayne Mery (:wsmwk)]

confirmed

Comment 2

[Alessandro Castellani [:aleca]]

Thanks for the report.

This is tricky because the "click away" action we have in place is done by listening to the blur event.
A blur even happens also when you move the focus to another window.

I'm not sure we can prevent pills from exiting the edit mode if the focus moves to another window.

Thomas, any thoughts?

Comment 3

[Igor R.]

Just from the behavior, - I would suggest to compare how it is done, say, in Adobe Photoshop. Let's say, you are doing a transformation (say, resizing) of an object/layer when you click and resize/reposition that object with the mouse. You can switch to a different program window and come back, and you will remain in that "transformation" mode, until you hit "Esc", or click away, or switch to a different mode/tool.
I hope that example might inspire some thoughts about how to do it here.

Is it possible to listen to a different, more narrowly defined event (or a set of events)?
I am not an expert on those, but looking at the API, I'd think, maybe Keyboard event (by analyzing KeyboardEvent.code or KeyboardEvent.key for a set of keys pressed, such as Esc, Enter) and Mouse.Event (MouseEvent.button + something like MouseEvent.region - for a click away)?
I understand that it makes it a bit more involved than just waiting for the blur event, but this way, it will do "The Tight Thing ™". :-)

Comment 4

[Thomas D. (:thomas8)]

Attachment: 1694502_tame-address-input-blur.diff

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

Thomas, any thoughts?

I'm on record for disliking autocomplete/pillify on blur, imo it's a wrong concept especially for autocomplete, which should always require affirmative user action (Enter).

This bug is even worse, because - as in reporter's scenario - when focus shifts to another window that does not imply at all that the user has finished editing the recipient.

So here's a hack which prevents autocomplete and pillification when focus went to another window.
Ironically, when focus goes out to another window, the input remains document.activeElement (iow, we've just lost focus but we still have it - don't ask me why or how this works), so we can test for that.

This fixes the problem for all unfinished addresses which do not trigger autocomplete matches.
For those who trigger autocomplete, we need to do something similar in the blur handler of toolkit autocomplete, which needs a separate patch and more thought.

https://searchfox.org/comm-central/rev/77a876e480a7d3367efc168652c91f5323feb32f/mozilla/toolkit/content/widgets/autocomplete-input.js#84-103

I tested the following locally and it works. Inline autocompletion will remain highlighted/selected, and I couldn't find a way to prevent the popup from closing, but I guess both of that is ok.

I wouldn't know if this breaks other consumers (although it's hard to imagine that any consumer would want autocomplete to fill stuff in just because the window lost focus, which is pretty uncontrolled and error-prone). To be on the safe side, we could add our own n-th parameter to the autocomplete and check for that.

      this.addEventListener(
        "blur",
        event => {
          // Ignore blur event if the input is still our active element, e.g.
          // when switching to another application.
          if (document.activeElement == this) {
            return;
          }
          if (!this._dontBlur) {

If Alex could handle the toolkit part, that would be great.

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1694502_tame-address-input-blur.diff

Ah, this is awesome!

I'm bouncing the review back to you because I was able to stop the autocomplete from kicking in on blur independently by setting the _dontBlur = true; attribute.

I think this is great and works as expected, and the dontBlur it's exactly what we need.
I'm a bit worried that this attribute might be removed by m-c since they use it purely to prevent a blur event when focusing, but I guess we gotta roll with it.

Comment 6

[Thomas D. (:thomas8)]

Comment on attachment 9205584 [details] [diff] [review]
1694502_tame-address-input-blur.diff

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

Excellent, thanks for adding the missing piece!
There's something else I want to improve, will offer a new patch.

::: mail/base/content/mailWidgets.js
@@ +1943,5 @@
>      }
>  
>      _setupEventListeners() {
> +      // Disable the autocomplete on blur since we will handle it independently.
> +      this.emailInput._dontBlur = true;

Excellent!

Comment 7

[Thomas D. (:thomas8)]

Attachment: 1694502_tame-autocomplete-blur.diff

New patch with 2 changes.

1. I think buildRecipientRow() isn't covered by the callback as it might run later, so I think it needs its own input._dontBlur = true;. We're currently not using that for building rows with autocomplete inputs, but that option is available in the function, so let's err on the safe side and keep all the autocomplete settings together in one place.

2. I would like to reduce the potential for user errors as far as possible. When user focuses another window while he's entering a new address / editing an existing address (without accepting any autocomplete suggestions), it's unlikely that he wants the autocomplete inline suggestion after coming back. The autocomplete input is pretty thick, or maybe I'm missing something, but I found eliminating the inline autocomplete suggestion (highlighted selection after what you've typed) rather non-trivial.

   I succeeded to safely remove the inline suggestion starting with " >> " from indirect matches (it's 99% unlikely that you'd manually type an email address containing " >> " and have a selection starting with exactly that and defocus your composition...).
   If user clicks into body for returning focus to composition, the suggestion (including the >>) can easily end up as a "correct" pill (which is wrong of course), and will actually send (which requires ux-error-prevention).

I wish we could do the same for regular inline suggestions (without >>), but alas, user has several ways of manually deselecting the inline suggestion without the autocomplet widget actually realizing that (via cursor right or mouse click in selection), which is de facto accepting it - nethertheless, autocomplete input claims .valueIsTyped = false, so there's no way to know if perhaps the user selected something after that and not autocomplete. So here we need to err in favor of preserving user's data.
Ideally, users should remove the inline suggestion before they switch windows, but no problem if they don't, it will still be focused and selected and hence have pretty good visibility.

With this patch, it's much better because now at least the on-blur conversion of the suggestion to a pill can only happen while the user is watching, whereas before it would happen covertly.

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9205906 [details] [diff] [review]
1694502_tame-autocomplete-blur.diff

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

Good intention, but unfortunately it doesn't work consistently and it introduces a regression.
If the user focuses on another element in the window by clicking it, the autocomplete doesn't properly kick in and the generated pill comes with the wrong string. (attachment incoming)
Hitting Enter to accept the suggested autocomplete works.

I don't think we should touch the autocomplete suggestion here as we risk to mess stuff up and introduce regressions.
Better keep the "focus" on the focus issue (excuse the pun :P)

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +734,5 @@
>        }
>        break;
>  
>      case "Tab":
> +      // Return for OS shortcuts Alt/Cmd+Tab to switch between active windows.

This is not super clear.
Let's write something like "Return if the Alt or Cmd modifiers were pressed, meaning the user is switching between windows and not tabbing out of writing an address."

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2021-03-01 10-47-47.png

This is the result if you click on another element after the autocomplete suggestion appears.

Comment 10

[Thomas D. (:thomas8)]

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

Created attachment 9206147 [details]
Screenshot from 2021-03-01 10-47-47.png

This is the result if you click on another element after the autocomplete suggestion appears.

That regression was introduced because with input._dontBlur, we're now always bypassing toolkit's entire blur handler which used to force completion before... wasn't that your idea? ;-p

Comment 11

[Thomas D. (:thomas8)]

Attachment: 1694502_tame-autocomplete-blur.diff

Alright. This patch is no longer trying to do any tricks on the inline suggestions, and fixes the regression.
Autocomplete is a messy thing, so messing with it is the only cure...
Afasics, it's now working exactly as expected for row inputs and pill inputs.

Comment 12

[Alessandro Castellani [:aleca]]

Comment on attachment 9208920 [details] [diff] [review]
1694502_tame-autocomplete-blur.diff

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

I think this works great!
Just a little linting nit to fix and this should be good to go.

I launched a try-run just to be sure we're not affecting the sending mechanism, which covers a little bit the creation of pills.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a370be4a8021e25807a45e832396be4cd260e560

Let's wait for this try-run to complete before marking this for check-in.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +867,5 @@
> +  }
> +
> +  // For other headers aka raw input, trim and we are done.
> +  if (
> +    input.getAttribute("is") != "autocomplete-input"

tiny linting error: this can be all in one line.

Comment 13

[Thomas D. (:thomas8)]

Attachment: 1694502_tame-autocomplete-blur.diff

Thank you for review and try build. Nit of comment 12 fixed, otherwise unchanged.

Comment 14

[Alessandro Castellani [:aleca]]

Comment on attachment 9209530 [details] [diff] [review]
1694502_tame-autocomplete-blur.diff

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

Try run is clean, good stuff.
I suggest to not back port this to 78 and let it run the train normally.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/24503e06b97d
Don't autocomplete and itemize recipients on blur if the window lost focus while editing them. r=aleca