1580950 - Address auto-complete click no longer accepts the address and moves to the next line

Status: RESOLVED FIXED

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

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1569492 +++

I've just landed a snipped of bug 1569492 onto TB 68 here:
https://hg.mozilla.org/releases/comm--esr68/rev/1f74906fee2f0980ec552146596f5bee261fb4d2

In the resulting version, clicking onto an auto-complete result no longer accepts that address.

So that's broken on TB 70, trunk and now TB 68.

Comment 1

[Jorg K (CEST = GMT+2)]

BTW, clicking on a auto-complete suggestion also doesn't work for mailing lists. I have't tried invitees, but it's most likely the same.

Comment 2

[Jorg K (CEST = GMT+2)]

Looking at bug 1569492 again, couldn't that have been fixed much simpler using what bug 1569492 comment #4 suggested?

Comment 3

[Alessandro Castellani [:aleca]]

(In reply to Jorg K (GMT+2) from comment #2)

Looking at bug 1569492 again, couldn't that have been fixed much simpler using what bug 1569492 comment #4 suggested?

The notifylegacyevents="true" attribute is the first thing that I tried but never worked. It was completely ignored.
The decision to move to native attributes like oninput and onkeypress was also due to the upcoming conversion of those fields to native html input.

I'm compiling 68 and taking a look at why the snipped patch is not working properly.
How come is broken on trunk and 70?
We tested it and it was working properly when it landed.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: Screencast from 2019-09-12 19.39.59.webm

This seems to work fine for me.

Comment 5

[Jorg K (CEST = GMT+2)]

This seems to work fine for me.

Well, I believe I see this in the video. You click on an auto-complete suggestion, and that is accepted into the input field. Then you hit return to move to the next field. But that's wrong. Clicking should automatically move to the next field, no hitting return required.

As I said on IRC: "20:08:15 - jorgk: that auto-complete is terribly tricky and sensitive".

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: 1580950-address-autocomplete-68.patch

What do you guys think about this solution?
Detecting the click event and forcing the return only if the target is not the textbox itself.

It's currently applied only to the messenger compose.
If you think it's a good solution I'll applied it everywhere where needed.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9092719 [details] [diff] [review]
1580950-address-autocomplete-68.patch

Seems to work. We need if for trunk/beta first, for 68 we can use this or the ontextentered hack.

Maybe this is better. Also, please add it to the two mailing list XUL files (add/edit).

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1580950-address-autocomplete.patch

Patch for trunk.

I just noticed that this issue doesn't happen for the Attendees invite in the calendar.
That textbox handles this problem by catching the blur event.
A similar solution to mine, but different.

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: 1580950-address-autocomplete-68.patch

Patch updated for 68 with the Mailing List dialogs.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9092723 [details] [diff] [review]
1580950-address-autocomplete-68.patch

It's OK to review one patch ;-) - But thanks for the rebased version.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9092722 [details] [diff] [review]
1580950-address-autocomplete.patch

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

This works. I've added some debug and clicking on auto-complete suggestions I saw span, image, description and richlistitem. If one day that throws up a div, we're hosed. Anyway, traditionally we had `ontextentered="awRecipientTextCommand(param, this); if (param && this.value != '') param.preventDefault();"` which looks much worse, do you agree? Also, I don't understand why clicking onto an auto-complete suggestion triggers this, well, maybe "text entered", but enter, which is what we processed:
https://dxr.mozilla.org/comm-esr60/source/mail/components/compose/content/addressingWidgetOverlay.js#846

Aceman and I have suffered through this quite a bit, for example coming up with the line quoted above, so I'll let him check it, too.

::: mailnews/addrbook/content/abMailListDialog.js
@@ -339,5 @@
>      input[0].setAttribute("id", "addressCol1#" + top.MAX_RECIPIENTS);
>    }
>  }
>  
> -function awNotAnEmptyArea(event) {

What about this stuff, this can just go?

Comment 12

[Alessandro Castellani [:aleca]]

I don't understand why clicking onto an auto-complete suggestion triggers this, well, maybe "text entered", but enter, which is what we processed:
https://dxr.mozilla.org/comm-esr60/source/mail/components/compose/content/addressingWidgetOverlay.js#846

The ontextentered method was used to deal with selecting a result from the autocomplete.
From the documentation: "This event handler is called when a result is selected for the textbox."

What about this stuff, this can just go?

That method was used to create an empty row on click in the Mailing List dialog if the last row had any value in it.
I replaced it with my method, which it also creates an extra empty row and also moves the focus on it.

If one day that throws up a div, we're hosed

I'm sure I will have to rework this whole thing once bug 1534455 lands and I'll have to convert those elements to html input.

Comment 13

[Paul Morris [:pmorris]]

Comment on attachment 9092722 [details] [diff] [review]
1580950-address-autocomplete.patch

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

This makes sense to me.  Just a few suggestions.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +909,5 @@
>  
> +function onRecipientClicked(event, inputElement) {
> +  if (
> +    event.originalTarget.localName != "div" &&
> +    event.originalTarget.localName != "textbox"

Would it work to check for a class rather than the localName?  If so, that might give us a little more future-proofing.  But maybe we don't have access to the originalTarget elements, and can't add our own class there, because they come from m-c code.  In that case we're relying on them not changing one thing or the other, and it's a coin toss as to which is more likely to change, so whatever works.

In any case, would be good to add a docstring explaining what this is here for.

Comment 14

[:aceman]

Attachment: 1580950.patch v2

What about this? Check for a <panel> element to see if a suggestion from the autocomplete list was clicked.

Yes, it will fail again if m-c rewrites autocomplete, but this is probably more indicative. If we see they touched <panel> we grep c-c for occurrences of panel and find this. With just "textbox" or "div" checks, we never find this place.

Comment 15

[:aceman]

(In reply to Jorg K (GMT+2) (reduced availability 14-19 of Sept.) from comment #11)

-function awNotAnEmptyArea(event) {

What about this stuff, this can just go?

In the mailing list editor, if you delete the last empty row of recipients and then click ANY of the existing ones (whether the first or last), a new empty row appears on the END. That looks weird and this patch drops this if we remove awNotAnEmptyArea().

Comment 16

[Alessandro Castellani [:aleca]]

Looking for the panel parent might be a bit more solid than my solution.
I'm good with it.

Do you want me to update the 2 patches with the suggested solution?

Comment 17

[Jorg K (CEST = GMT+2)]

We'll handle it.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1580950.patch v2b

Discussed with Aceman over IRC. Let's return early if we're certainly not on the popup.

Comment 19

[:aceman]

Comment on attachment 9092790 [details] [diff] [review]
1580950.patch v2b

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

Yes, good to abort if we see we are in the recipient box already to avoid climbing 10 more levels of elements and find no panel.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/43f3705dfb3b
Make click onto auto-complete results popup accept suggestion and move to the next line again. r=jorgk DONTBUILD

Comment 21

[Jorg K (CEST = GMT+2)]

TB 68.1.1 or TB 68.2:
https://hg.mozilla.org/releases/comm-esr68/rev/736b5db8c01e6c8fae1ca001ce459532253b4bb8

Comment 22

[Jorg K (CEST = GMT+2)]

TB 70 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/12980836e3bce0e5a1636680784339d582382d7b

Comment 23

[Alessandro Castellani [:aleca]]

Does anyone know why this patch seems to not be present on trunk, but the click works?

Comment 24

[Jorg K (CEST = GMT+2)]

You know :-) - The trunk version was ripped out when auto-complete was de-XBL'ed and you actually implemented the new version here: Bug 1565075 comment #57.

Comment 25

[Alessandro Castellani [:aleca]]

Oh man, I completely forgot about it...uff, thanks for the reminder.