1536374 - Mailing lists: Enter to confirm recipient autocomplete prematurely closes the dialogue on TB 60.3 ESR and later

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Thomas D. (:thomas8)]

Ooops. Populating mailing lists has just become a no-op. They are always a PITA, but this makes it worse...
I think this came with the last update on release channel, haven't seen it before.

STR

1. create new mailing list (from AB or contacts side bar)
2. type "John"
3. press Enter to confirm john and to enter next recipient

Actual result

• Mailing List dialog closes prematurely
• cannot add more recipients via keyboard
• contacts side bar: current ab view changes, now wrongly shows the mailing list contacts

Expected result

• confirm address, allow adding more recipients
• do not close dialog
• do not mess up AB view

Console error:
09:04:26.191 ReferenceError: event is not defined 1 autocomplete.xml%20line%20422%20%3E%20Function:3:60
anonymous chrome://global/content/bindings/autocomplete.xml%20line%20422%20%3E%20Function:3:60
anonymous self-hosted:1020:17
onTextEntered chrome://global/content/bindings/autocomplete.xml:256:18
handleEnter chrome://global/content/bindings/autocomplete.xml:540:18
handleKeyPress chrome://global/content/bindings/autocomplete.xml:503:24
onKeyPress chrome://global/content/bindings/autocomplete.xml:434:18
onxblkeypress chrome://global/content/bindings/autocomplete.xml:617:8
goEditListDialog chrome://messenger/content/addressbook/abCommon.js:723:3
AbEditCard chrome://messenger/content/addressbook/abCommon.js:494:5
AbEditSelectedCard chrome://messenger/content/addressbook/abResultsPane.js:263:3
doCommand chrome://messenger/content/addressbook/abResultsPane.js:484:9
goDoCommand chrome://global/content/globalOverlay.js:84:7
oncommand chrome://messenger/content/addressbook/abContactsPanel.xul:1:1

Comment 1

[Jorg K (CEST = GMT+2)]

Grrr, that happens on TB 60.6 ESR as well, same error. How did that break, it was working in TB 60.3 after bug 1499410 was fixed, see uplift in bug 1499410 comment #25. I don't really understand what happened here, autocomplete.xml doesn't appear to have changed much: https://hg.mozilla.org/releases/mozilla-esr60/log/tip/toolkit/content/widgets/autocomplete.xml
Note that all the push dates are wrong.

Alice, can you please locate the regression on Daily, and if possible also on the ESR. As I said, 60.3 worked, 60.6 doesn't.

@Thomas: When you see a regression, please contact me. In general, bug triaging is done in a very patchy fashion, so it's easy for bugs to just slip through the cracks.

Comment 2

[Alice0775 White]

STR

1. create new mailing list (from AB or contacts side bar)

Please describe steps one by one.

Comment 3

[Jorg K (CEST = GMT+2)]

Alice, but you even provided a patch for bug 1499410, so I'm pretty sure you know what needs to be done:

• Open the TB address book, right-click on an address book, New List.
• On the Window that pops up, enter a name, XXX.
• Click below to add a mailing list member, so type any character that will cause
  auto-complete.
• Hit enter to accept, like you would when composing a message and entering an address.

Result: The auto-complete choice is accepted, the address is added to the mailing list, and the panel is closed.

Closing the panel is undesired, it should just go to the next line. I'm sure it worked in 60.3.

Comment 4

[Alice0775 White]

Regression window(comm-esr60 channel)
https://hg.mozilla.org/releases/mozilla-esr60/pushloghtml?fromchange=92515b2d8a74ec96119209101319a8b21e06c44d&tochange=ab014151d4c338562949c28aa140786b548856ca
https://hg.mozilla.org/releases/comm-esr60/pushloghtml?fromchange=ffd2a1a2de610930baa03a014e52980052b1834e&tochange=342a301ded26733882b7887343fd7f83d29093c4

puzzled..
a3f827381567 Masayuki Nakano — Bug 1434837 - Make autocomplete and satchel listen to keypress event at the system event group. r=mak (was c0b4ca69376c, backed out in 92515b2d8a74) a=jorgk THUNDERBIRD_60_VERBRANCH

ff097eea51d5 Jorg K — Bug 1499410 - fix mailing list enter handling (idea by Alice White). r=aceman a=jorgk

Comment 5

[Alice0775 White]

Btw, I cannot reproduce the issue on Daily68.0a1(BuildID=20190319090245).

Comment 6

[Jorg K (CEST = GMT+2)]

Hmm, I tried a TB 52 and there enter accepts the auto-complete suggestion and moves to the next line of the list.

In TB 60.2.1 it doesn't work at all, enter is ignored, bug 1499410. Then I tried 60.3.3 (which I had installed), and enter accepts the address and closes the panel. I'm puzzled too now.

So Masayuki-san landed bug 1434837on mozilla60 on 2018-02-20. So from that day, enter would have been ignored. But what happened the day before? Did enter close the panel, or move to the next line?

Maybe our fix to bug 1499410 was wrong? We were so happy that the enter didn't get ignored, so we didn't notice that the panel closed? What happened immediately after bug 1499410 landed on trunk, so 2018-10-25.

I think, we won't find anything on ESR. Let's see what happened on trunk, before 2018-02-20, between 2018-02-20 and 2018-10-25 and after 2018-10-25.

Comment 7

[Alice0775 White]

And also I cannot reproduce the issue on Beta66.0b3(BuildID=20190306001450).

Comment 8

[Jorg K (CEST = GMT+2)]

OK, but the ESR has a problem, and we will only locate that if we can check the three ranges I've given you.

Comment 9

[Alice0775 White]

1st Fixed window(comm-central):
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=97c5ad955dd4cc7e162cc418af4248aff595a&tochange=531dfcb2d29dcd8a6dab337faf4fd2685fcf9d40
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=09d302af0a59dadf990400f18e426766e2&tochange=ddadc29de671917f66478e62a6accd4764892d25

It difficult to find 2nd broken window after the above(BuildID=20181024223458)...

Comment 10

[Alice0775 White]

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

Hmm, I tried a TB 52 and there enter accepts the auto-complete suggestion
and moves to the next line of the list.

In TB 60.2.1 it doesn't work at all, enter is ignored, bug 1499410. Then I
tried 60.3.3 (which I had installed), and enter accepts the address and
closes the panel. I'm puzzled too now.

So Masayuki-san landed bug 1434837on mozilla60 on 2018-02-20. So from that
day, enter would have been ignored. But what happened the day before? Did
enter close the panel, or move to the next line?

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=5620690a17267e56e1407ae64294624747701ea1&tochange=91311b1d6aa3bddd427858cb7f8c149a81c71a92
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2c000486eac466da6623e4d7f7f1fd4e318f60e8&tochange=b03d9c87bbf6b9b73883758ce494dd971368b5c9

Before landing Bug 1434837: It works as expected, press ENTER moves to the next line.
After landing Bug 1434837: Press ENTER do nothing.

Maybe our fix to bug 1499410 was wrong? We were so happy that the enter
didn't get ignored, so we didn't notice that the panel closed? What happened
immediately after bug 1499410 landed on trunk, so 2018-10-25.

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=97c5ad955dd4cc7e162cc418af4248aff595a&tochange=531dfcb2d29dcd8a6dab337faf4fd2685fcf9d40
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=09d302af0a59dadf990400f18e426766e2&tochange=ddadc29de671917f66478e62a6accd4764892d25

Before landing Bug 1499410: Press ENTER do nothing.
After landing Bug 1499410: It works as expected, press ENTER moves to the next line.

I think, we won't find anything on ESR. Let's see what happened on trunk,
before 2018-02-20, between 2018-02-20 and 2018-10-25 and after 2018-10-25.

Comment 11

[Jorg K (CEST = GMT+2)]

OK, so this appears to be a backport error. After we fixed bug 1499410 it all was working. That fix got uplifted to ESR 60 and no one checked it there, so it most likely never worked there. I tried TB 60.3.0 ESR and it closes the window, so not working.

Let's check what landed:
Trunk: https://hg.mozilla.org/comm-central/rev/d3daa2594870
ESR: https://hg.mozilla.org/releases/comm-esr60/rev/ff097eea51d576adda0f75c2b28d29ee40100eaa

It closes the windows since "enter" will trigger the "OK" button unless we run event.preventDefault();. And exactly that seems to cause the error quoted in comment #0 and here again:

ReferenceError: event is not defined[Learn More] autocomplete.xml%20line%20422%20%3E%20Function:3:60
	anonymous chrome://global/content/bindings/autocomplete.xml%20line%20422%20%3E%20Function:3:60
	anonymous self-hosted:1020:17
	onTextEntered chrome://global/content/bindings/autocomplete.xml:256:18
	handleEnter chrome://global/content/bindings/autocomplete.xml:540:18
	handleKeyPress chrome://global/content/bindings/autocomplete.xml:503:24
	onKeyPress chrome://global/content/bindings/autocomplete.xml:434:18
	onxblkeypress chrome://global/content/bindings/autocomplete.xml:617:8
	goEditListDialog chrome://messenger/content/addressbook/abCommon.js:723:3
	AbEditSelectedDirectory chrome://messenger/content/addressbook/abCommon.js:220:5
	doCommand chrome://messenger/content/addressbook/abCommon.js:166:9
	goDoCommand chrome://global/content/globalOverlay.js:84:7
	oncommand chrome://messenger/content/addressbook/addressbook.xul:1:1

autocomplete.xml:256:18 is rv = this._textEnteredHandler(event);
https://hg.mozilla.org/releases/mozilla-esr60/file/tip/toolkit/content/widgets/autocomplete.xml#l255
and that runs initEventHandler("textentered"); here:
https://hg.mozilla.org/releases/mozilla-esr60/file/tip/toolkit/content/widgets/autocomplete.xml#l416

The cryptic error comes from line 422 of that file.

Marco, does this make any sense to you? Why would event not be available on the event handler in the XUL file?

Thomas, given that this is broken since 60.3 and before enter was ignored altogether, I'm afraid we can't invest too much effort into fixing this, and it's also very hard to track down. You might be able to do the tricks you always do by unpacking the omni.ja and manipulating the JS and XUL files. If you can come up with a fix, I'm happy to ship it on ESR 60.

Comment 12

[Jorg K (CEST = GMT+2)]

Thomas, could you try this. Instead of:

ontextentered="awRecipientTextCommand(param, this); if (this.value != '') event.preventDefault();"

make it

ontextentered="awRecipientTextCommand(param, this); if (this.value != '') param.preventDefault();"

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1536374-ml.patch

OK, let's try this. This also undoes the damage done here:
https://hg.mozilla.org/releases/comm-esr60/rev/ff097eea51d576adda0f75c2b28d29ee40100eaa#l4.12

Apparently SM have their own awRecipientTextCommand() which doesn't take an event:
https://searchfox.org/comm-central/rev/af1e9129c1cfbb72f73d346ee10c4f57a6887679/suite/mailnews/components/compose/content/addressingWidgetOverlay.js#704

Comment 14

[Jorg K (CEST = GMT+2)]

TB 60.6.1 or later:
https://hg.mozilla.org/releases/comm-esr60/rev/dd1713342fe3f723edb6da6ac8de2ed3c36d13a1

That fixed it.

FRG, my apologies, we messed with SM and Mailnews code here
https://hg.mozilla.org/releases/comm-esr60/rev/ff097eea51d576adda0f75c2b28d29ee40100eaa#l4.12
and here:
https://hg.mozilla.org/releases/comm-esr60/rev/dd1713342fe3f723edb6da6ac8de2ed3c36d13a1#l3.13

The latter changeset restores the original more or less. Can you please check it.

Comment 15

[Thomas D. (:thomas8)]

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

Thomas, could you try this. Instead of:

I tried in my flat version of 67.0a1 (2019-03-15) (64-bit) and strangely did not see the bug, with or without your change. Probably release only where it's harder to check...

Thanks for fixing this. Quick and dirty... Maybe I'll do the cleanup in the other bug. param is always an event, isn't it? The name should probably reflect that. Do we know what type of event? TB Code assumes that ontextentered can only be triggered by Enter key. I doubt that, but would need more checking in the internals. Normally we also try not to have any code in the XUL files except the caller (separate code vs. layout). What's easy and tempting to do in the layout makes it harder to understand the inner workings from reading the code in the JS file.

Comment 16

[Thomas D. (:thomas8)]

On 67.0a1 (2019-03-15) (64-bit), I do however see the other half of this bug, that suddenly the mailing list populates contacts side bar under the currently selected AB, which is very irritating.
Jörg, you removed this from summary. Has this been fixed, too? Otherwise please file a new bug.

Comment 17

[Jorg K (CEST = GMT+2)]

That seems completely unrelated and you should have filed a separate bug to start with. So please do it now.

Comment 18

[:aceman]

Do we need to do this same on 68?

Comment 19

[Thomas D. (:thomas8)]

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

That seems completely unrelated and you should have filed a separate bug to start with. So please do it now.

With benefit of hindsight, that's easy to say. What I have seen is that pressing Enter in mailing lists triggers two wrongs, the closing of the dialogue AND the wrong update of contacts side bar. Without knowing the internals, all reason to assume that the two may be related, so there was no reason for me to file two separate bugs. Plus they also appeared at the same time, don't remember seeing this before in the recent past. Anyway, we'll find out in the other bug to be filed.

Comment 20

[:aceman]

Comment on attachment 9052196 [details] [diff] [review]
1536374-ml.patch

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

This seems right. Ontextentered is a different beast and is handled via https://hg.mozilla.org/releases/mozilla-esr60/file/tip/toolkit/content/widgets/autocomplete.xml#l250 and https://hg.mozilla.org/releases/mozilla-esr60/file/tip/toolkit/content/widgets/autocomplete.xml#l416, which seems to rename the 'event' argument into 'param' and then passing it to the code in the ontextentered attribute.

Comment 21

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #20)

Comment on attachment 9052196 [details] [diff] [review] 

This seems right. Ontextentered is a different beast and is handled via
https://hg.mozilla.org/releases/mozilla-esr60/file/tip/toolkit/content/
widgets/autocomplete.xml#l250 and
https://hg.mozilla.org/releases/mozilla-esr60/file/tip/toolkit/content/
widgets/autocomplete.xml#l416, which seems to rename the 'event' argument
into 'param' and then passing it to the code in the ontextentered attribute.

1. renaming event to param does not make much sense, wonder why?
2. while we have to use their exact name to get the param event in the ontextentered caller, in our own functions we should indicate that this is an event if it actually is. So we could use eventParam, event, or enterEvent in our function (if we know that it only gets triggered by Enter, as our code was claiming, untested). Also, imo we shouldn't keep code in the XUL file, ever. Alas, as long as it works, it's better... ;-)

Comment 22

[Jorg K (CEST = GMT+2)]

Well, the code I changed was:

ontextentered="awRecipientTextCommand(param, this); if (this.value != '') event.preventDefault();"

event was undefined, param was not and it was carrying the event to awRecipientTextCommand(). To the fix is obvious and works. I don't know why event works on TB 66 and later. Please investigate that and we can also change param to event in another bug. I'm done here. According to https://mail.mozilla.org/pipermail/tb-planning/2019-March/006451.html I'm the "code manager" (no longer a developer), so I managed the backport error.

Comment 23

[:aceman]

I played with this on TB trunk 68 and indeed both of 'param' and 'event' are initialized there.
Usually they contain the same value (event object).

The change of behaviour since TB60 may be caused by parts of autocomplete.xml already were converted to custom elements.

So we wonder if it would be right to also do the change on trunk:
awRecipientTextCommand(param, this); if (this.value != '') param.preventDefault();

But I found a case where param != event and that is when you type some characters, let autocomplete propose results and you choose one. Then the selected string is put in the field and cursor moved to new empty line (inputbox).
At that event the ontextentered is called with param==null but 'event' is "blur" event. The 'this' is the previous line (the one where autocomplete value was accepted) so this.value is not empty, but param is null and we get an error :(

Notice awRecipientTextCommand() handles 'param' being undefined. When it is defined, it is assumed to be Enter being hit. Strangely the case of 'param==null' I noticed is actually when Enter was pressed on the autocomplete result.

Comment 24

[Jorg K (CEST = GMT+2)]

Let's have this discussion in bug 1539295.

Comment 25

[Frank-Rainer Grahl (:frg)]

wrt Comment 14. Thanks Jorg. Put a testing note for it in the SeaMonkey 2.57 bug. Will check it when mailnews is in better shape.