1569492 - After entering address not in contacts, can't hit enter to advance to next email address line (auto-complete)

Status: RESOLVED FIXED

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

Description

[Jonathan Kamens]

Open a compose window.
Type an address that isn't in your contacts, e.g., [e-mail], and wait for it to turn red.
Hit enter.
Note that nothing happens.
What should happen is you should advance to the next header line.

The first nightly this was broken in was 2019-07-26.

I regressed using mozregression and it narrowed the issue down to a specific build, but I looked at the changelog for the regression results and it looks completely irrelevant, so I suspect this is a mozilla-central issue.

Regression results:

 3:37.44 INFO: Running comm-central build built on 2019-07-25 22:59:46.614000, revision a519f975
 3:49.16 INFO: Launching /tmp/tmpMha6NM/thunderbird/thunderbird
 3:49.16 INFO: Application command: /tmp/tmpMha6NM/thunderbird/thunderbird --allow-downgrade -profile /tmp/tmpUS53OV
 3:49.16 INFO: application_buildid: 20190725221809
 3:49.16 INFO: application_changeset: a519f97538ac82e780061764b3889cde0ee452a9
 3:49.16 INFO: application_name: Thunderbird
 3:49.16 INFO: application_repository: https://hg.mozilla.org/comm-central
 3:49.16 INFO: application_version: 70.0a1
Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): bad
 4:03.71 INFO: Narrowed inbound regression window from [04799823, 3a098643] (3 builds) to [04799823, a519f975] (2 builds) (~1 steps left)
 4:03.71 INFO: No more inbound revisions, bisection finished.
 4:03.71 INFO: Last good revision: 047998237781d0e3c85c90e798bdba36e8bd7d92
 4:03.71 INFO: First bad revision: a519f97538ac82e780061764b3889cde0ee452a9
 4:03.71 INFO: Pushlog:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=047998237781d0e3c85c90e798bdba36e8bd7d92&tochange=a519f97538ac82e780061764b3889cde0ee452a9

I am happy to regress mozilla-central issues like this when I have the time to do it, but I don't know how. Can anyone offer any guidance for how it's done?

Comment 1

[WaltS48 [:walts48]]

Can't offer any guidance.

I can confirm it in my TB 70.0a1 on Ubuntu 18.04.2 LTS.

Comment 2

[Wayne Mery (:wsmwk)]

Given the push time of https://hg.mozilla.org/comm-central/rev/a519f97538ac82e780061764b3889cde0ee452a9 I put the mozilla pushlog range to be (perhaps untrustworthy) approx https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2019-07-25+17%3A00%3A00&enddate=2019-07-25+24%3A00%3A00

Comment 3

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=047998237781d0e3c85c90e798bdba36e8bd7d92&tochange=a519f97538ac82e780061764b3889cde0ee452a9 from comment #0.

https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=a519f97538ac82e780061764b3889cde0ee452a9 was built with
M-C 026812d05de4546d50b277499171050bea
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=047998237781d0e3c85c90e798bdba36e8bd7d92 was built with
M-C f30055b91e96481970506158162b5a44e9

You can see that by visiting the builds on treeherder and clicking on a B. It's easy since this was so recent. If it's further back, I usually ask Alice. I guess you need to download the binaries and inspect the build config in the troubleshooting information.

So the M-C range is:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f30055b91e96481970506158162b5a44e9&tochange=026812d05de4546d50b277499171050bea, wow, the same as Wayne came up with, but confirmed ;-)

So this looks like:
Bug 1498560 - Remove new Function from autocomplete.xml, r=mak
https://hg.mozilla.org/mozilla-central/rev/dc7cc0b7c0dfc5490b3bed862b6b67fe657d34fb

Jonas and Marco, I think you broke the special "force complete" case TB is using :-(

Comment 4

[Marco Bonardo [:mak]]

Jonas was going to file a bug against comm-central, to notify you about the change. where you are currently using ontextentered or ontextreverted, you must add a notifylegacyevents="true" attribute and use the Custom Events (textentered and textreverted).

Comment 5

[Marco Bonardo [:mak]]

otherwise, you can clearly override the binding and onTextEntered, onTextReverted

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks, Marco. "Jonas was going to file a bug against comm-central ...", hasn't happened, or have I missed something?

Looks like an issue for the de-XBL team.

Comment 7

[Marco Bonardo [:mak]]

probably the bug would have been filed today, I didn't consider it a critical blocker for landing.

Comment 8

[Calum Mackay]

[just for duplicate identification] note that this seems also to affect addresses that are in Contacts, at least for me.

Side-effect is that you're now limited to just a few recipients, since there's no way to get a new field to add another, with ENTER or TAB.

Comment 9

[Jorg K (CEST = GMT+2)]

Hmm, Magnus is not answering, Alex, you've looked at auto-complete stuff, so maybe this one if for you.

Comment 10

[Alessandro Castellani [:aleca]]

Hey, thanks for pulling me into the this.
There's a patch waiting to land on bug 1565075, which takes care of converting those textbox elements to html:input elements, and removes the ontextentered attribute.
As you can see the patch is blocked by bug 1534455 from m-c.

We could do 2 things here:

• Add a notifylegacyevents="true" attribute and use the Custom Events (textentered and textreverted).
• Wait for bug 1534455 to land, and then land right after our bug 1565075.

Comment 11

[Marco Bonardo [:mak]]

if, at a certain point, you don't need anymore notifylegacyevents, please file a bug to remove it.

Comment 13

[Jorg K (CEST = GMT+2)]

Alex, there are two weeks left before TB 70 goes to beta. Can we fix this bug here with some intermediate patch. Is there an ETA for bug 1534455? I think we should go for option 1 in comment #10.

Comment 14

[Alessandro Castellani [:aleca]]

It doesn't seem to be an ETA for bug 1534455.
I'll try to have a temp patch ready for today based on option 1 in comment #10.

Comment 15

[Alessandro Castellani [:aleca]]

Attachment: 1569492-message-compose.patch

I ended up updating the onkeypress method to handle the Enter keystroke.
Now it works exactly like 68.
Here's the link to a try-run I just pushed: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d7f8c6b36403f6435336adb31c6d0cfd2aebdc92

Comment 16

[Jorg K (CEST = GMT+2)]

Please try adding an address to a new mailing list and editing a mailing list by adding an address. Also, try adding an invitee to an event. All those use address auto-complete.

Comment 17

[Jorg K (CEST = GMT+2)]

I think I should review this or Magnus.

Comment 18

[Richard Marti (:Paenglab)]

Comment on attachment 9086136 [details] [diff] [review]
1569492-message-compose.patch

As requested.

Comment 19

[Jorg K (CEST = GMT+2)]

As I said, I think it's incomplete:

calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
84 type="autocomplete"
mail/base/content/messenger.xul
249 <panel type="autocomplete"
mail/components/addrbook/content/abEditListDialog.xul
63 type="autocomplete" flex="1"
mail/components/addrbook/content/abMailListDialog.xul
74 type="autocomplete" flex="1"
mail/components/compose/content/messengercompose.xul
2045 type="autocomplete" flex="1"

I forgot, the gloda search box also uses autocomplete, so there are 5 consumers: Attendees, Gloda, compose, 2x mailing lists.

Comment 20

[Alessandro Castellani [:aleca]]

Jorg, the issue is not with type="autocomplete" attribute but with the ontextentered attribute.
I'm removing it in those other locations.

Comment 21

[Alessandro Castellani [:aleca]]

Attachment: 1569492-ontextentered-attribute.patch

OK, this should cover everything.
I manually tested all those fields and they behave like in 68.
I had to add an event.preventDefault() to avoid the closing of the mailing list dialog if the user is adding contacts to the list.

Let me know if you find any issue or quirky behaviour.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086164 [details] [diff] [review]
1569492-ontextentered-attribute.patch

Looks like this needs a calendar reviewer for the calendar parts.

Comment 23

[Paul Morris [:pmorris]]

Comment on attachment 9086164 [details] [diff] [review]
1569492-ontextentered-attribute.patch

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

Looking good, but I found a few things to address, so r- for now.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
@@ -90,5 @@
>                       completedefaultindex="true"
>                       forcecomplete="true"
>                       completeselectedindex="true"
>                       minresultsforpopup="1"
> -                     onblur="if (this.localName == 'textbox') { this.closest("calendar-event-attendees-list").returnHit(this, true) }"

Do we no longer want to do the check for |this.localName == 'textbox'| in the new code?  Might not be needed now, but I tend to err on the side of keeping such checks, given the way events can bubble all over the DOM.

::: mail/components/addrbook/content/abMailListDialog.xul
@@ -78,3 @@
>                     completeselectedindex="true"
>                     minresultsforpopup="3"
> -                   ontextentered="awRecipientTextCommand(param, this); if (param && this.value != '') param.preventDefault();"

Shouldn't we delete awRecipientTextCommand?  Looks like it is no longer called.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +784,5 @@
>    switch (event.key) {
>      case "Enter":
> +      event.preventDefault();
> +      awReturnHit(element);
> +      break;

By breaking here you prevent falling through to the "Tab" case, so entering multiple addresses (comma-separated) no longer works when hitting "Enter" (see comment below for the tab case).  I tested the patch and confirm that this is broken here.  (Would be nice to have test coverage for this functionality. -- EDIT: not saying you should do that in this patch/bug, just a general lament.)

Comment 24

[Alessandro Castellani [:aleca]]

Attachment: 1569492-ontextentered-attribute.patch

Thanks for the review.
All the things should be addressed.

I ended up renaming the awRecipientTextCommand to awAbAddRecipient in order to handle the AddressBook recipients with a dedicated method and not having to create multiple conditions to fix issues with the Tab keystroke.

Comment 25

[Jorg K (CEST = GMT+2)]

Thanks, Paul, for looking at the non-Calendar parts as well. I'll have a look once you're done.

Comment 26

[Paul Morris [:pmorris]]

Comment on attachment 9086451 [details] [diff] [review]
1569492-ontextentered-attribute.patch

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

r+ with a couple of suggestions to address.  

When testing I found that we don't quite fully support the ability to paste in a list of (comma-separated) addresses into a mailing list via the address book, like we do elsewhere.  It actually works, after you click the "OK" button to save the changes, but it doesn't look like it works because the addresses are not separated out onto their own lines when you hit "Enter".  Might be worth fixing while we are here?

Sending it back over to Jorgk.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +773,5 @@
>  function awRecipientOnFocus(inputElement) {
>    inputElement.select();
>  }
>  
> +function awAbAddRecipient(event, element) {

A separate function to simplify things makes sense.  For the name, what about something that expresses the parallel with `awRecipientKeyPress`?  Something like `awAbRecipientKeyPress` or `awAddressBookRecipientKeyPress`?

And while we're at it, lets add short doc strings on each function to say that one is for use with the address book and the other is used for email compose.

Comment 27

[Jorg K (CEST = GMT+2)]

When testing I found that we don't quite fully support the ability to paste in a list of (comma-separated) addresses into a mailing list via the address book, like we do elsewhere. ... Might be worth fixing while we are here?

Oh no. Thanks for the "boy-scout" spirit, but let's not fix everything at once. What you describe doesn't work in TB 68, and it even doesn't work when you press OK. I added "[e-mail], [e-mail], [e-mail]" into one line and with OK I only got a and c. By all means, file a new bug and fix it :-)

I prefer awAbRecipientKeyPress to the longer version.

Comment 28

[Alessandro Castellani [:aleca]]

Thanks for the review.
Let's go with awAbRecipientKeyPress and I'll add missing doc strings.
Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=581563965098f85f1d141f22e59327789f0f317e
Are those Z2 and bct the usual intermittent issues?

Comment 29

[Alessandro Castellani [:aleca]]

Attachment: 1569492-ontextentered-attribute.patch

Doc strings created.
Does this make sense?

Comment 30

[Jorg K (CEST = GMT+2)]

Are those Z2 and bct the usual intermittent issues?

Yes, they are. Z2 is bug 1568789 and Mochitest/bct fails all the time. I filed a bug 1575036 now.

Comment 31

[Paul Morris [:pmorris]]

Comment on attachment 9086487 [details] [diff] [review]
1569492-ontextentered-attribute.patch

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

Some nit stuff, looks good overall.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +774,5 @@
>    inputElement.select();
>  }
>  
> +// Used to catch the onkeypress attribute in the Address Book
> +// Mailing List dialogs.

Pretty good!  I have my insufferably nit-picky editor hat on today, so here's my suggestion: 

"Handles keypress events for the email address inputs (that auto-fill) in the Address Book Mailing List dialogs."

And lets use the JSDOC style formatting for them:

/**
 * Doc string here.
 */
function blah() {

Comment 32

[Paul Morris [:pmorris]]

I've filed bug 1575046 for pasting lists of email addresses into mailing list dialogsf

Comment 33

[Alessandro Castellani [:aleca]]

Attachment: 1569492-ontextentered-attribute.patch

Comment 34

[Jorg K (CEST = GMT+2)]

Since I'm not reviewing it, I thought I'd try it at least. Compose, Gloda (unchanged) and AB stuff work fine, but I can't add in invitee at all. I type "John", some John appears and I can hammer the keyboard with enter and nothing happens. Am I missing something?

Comment 35

[Paul Morris [:pmorris]]

Hm, I just double-checked and the calendar invitees are auto-filling just fine for me as they did when I checked it before. I'm on Linux. Maybe it's a platform thing?

Comment 36

[Jorg K (CEST = GMT+2)]

Sorry, it needed a "partial" clobber to get back on track. Sorry about the noise. I'll fix the not-so-elegant

+function onAttendeesInputBlur(element) {
+    if (this.localName != "textbox") {
+        return;
+    }
+
+    element.closest("calendar-event-attendees-list").returnHit(element, true);
+}

Comment 37

[Alessandro Castellani [:aleca]]

No problem at all, and thanks for cleaning up that condition.

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: 1569492-ontextentered-attribute.patch

Sorry to be a pain. If fixed the commit message and onAttendeesInputBlur().
EDIT: https://bugzilla.mozilla.org/attachment.cgi?oldid=9086497&action=interdiff&newid=9086529&headers=1

There's still an issue. In TB 68, when you add an invitee by clicking in the auto complete pop-up, that person is added and you get a new row. Not so with the patch. The person is added, but you need to hit enter. If it's too hard to fix, we can leave it.

Comment 39

[Alessandro Castellani [:aleca]]

Good finding, I only tested with Enter and Tab.
I'll take a look as see if I can find the issue.

Comment 40

[Alessandro Castellani [:aleca]]

Attachment: 1569492-ontextentered-attribute.patch

Mhhh, this is a bit tricky since without the textentered attribute I don't know how to determine when text is added to the input from the autocomplete popup.
I'll keep investigating and see if I ca find a workaround.

Meanwhile, I updated the patch to fix the localName condition, which had a little mistake.

Comment 41

[Alessandro Castellani [:aleca]]

Attachment: 1569492-ontextentered-attribute.patch

I think I fixed it.
The problem wasn't related to the ontextentered but rather to this condition which it doesn't apply anymore after the CE conversion:
https://searchfox.org/comm-central/rev/d55fb7f2405fd4e07cb267851340626a63a8c270/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js#611

Comment 42

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086559 [details] [diff] [review]
1569492-ontextentered-attribute.patch

Yes, this works, thanks for tracking it down. I'll land this now. I didn't realise how badly broken this all was on Daily, so it's really time to fix it.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1f2b56d3ba48
Replace deprecated ontextentered attribute to fix recipient/mailing list/invitee auto-complete. r=pmorris

Comment 44

[Jorg K (CEST = GMT+2)]

Jonathan, sorry about the delay here. I didn't realise how bad it was.

Comment 45

[Paul Morris [:pmorris]]

Comment on attachment 9086559 [details] [diff] [review]
1569492-ontextentered-attribute.patch

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

Last set of changes look good to me.