1499410 - Pressing enter to accept new entry in address book mailing list is ignored

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[M Lopez-Ibanez]

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

Steps to reproduce:

1. In address book, New List, go to field to enter email addresses
2. Start typing, an overlay with a match candidate appears.
3. Press enter


Actual results:

Nothing, the key is ignored. Tab is accepted


Expected results:

The selection is accepted, cursor moves to next entry field.

Comment 1

[Jorg K (CEST = GMT+2)]

Confirmed. I need to look into that.

Comment 2

[Jorg K (CEST = GMT+2)]

Alice, perhaps a regression range would be helpful here. Can you find it for us please.
Note bug 1500411. Posting on BMO is somewhat broken, you need to repeat until it works.

STR:
Edit a mailing list.
Start typing a new member and wait for the auto-complete to happen.
Press enter. Nothing happens, auto-complete should have accepted the item.

Comment 3

[Jorg K (CEST = GMT+2)]

I'm utterly puzzled by this bug. Here's what I've done.

For address entry, we use autocomplete there:
https://searchfox.org/comm-central/rev/f9a9b8d71d4898fef36bc6eea2dfde0e0e56ff35/mail/components/compose/content/messengercompose.xul#1962
I've added debug to <method name="handleEnter"> in M-C's autocomplete.xml and to our functions awRecipientKeyDown, awRecipientKeyPress and awRecipientTextCommand.
When I start address entry and navigate the popup with the arrow keys and then hit enter, I get this output:
=== awRecipientKeyDown
=== awRecipientKeyPress
=== handleEnter
=== awRecipientTextCommand
=== awRecipientTextCommand
All working.

However, on the mailing list edit here
https://searchfox.org/comm-central/rev/f9a9b8d71d4898fef36bc6eea2dfde0e0e56ff35/mail/components/addrbook/content/abEditListDialog.xul#59
I get this debug, and I can hit enter as many times as I like, it doesn't register at all:
=== awRecipientKeyDown
Well, hitting enter does give another one of those debug lines.

So who eats my enter? The XUL for both autocomplete setups is vaguely the same, so I really don't see why one works and the other one doesn't. Marco, do you see anything I don't see?

Comment 4

[Alice0775 White]

(In reply to Jorg K (GMT+2) from comment #2)
> Alice, perhaps a regression range would be helpful here. Can you find it for
> us please.
> Note bug 1500411. Posting on BMO is somewhat broken, you need to repeat
> until it works.
> 
> STR:
> Edit a mailing list.
> Start typing a new member and wait for the auto-complete to happen.
> Press enter. Nothing happens, auto-complete should have accepted the item.

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

Suspect: c0b4ca69376c	Masayuki Nakano — Bug 1434837 - Make autocomplete and satchel listen to keypress event at the system event group r=mak

Comment 5

[Jorg K (CEST = GMT+2)]

Wow, that broke in February 2018 and no one noticed :-( - Thank you so much, Alice!

Let's add Masayuki-san here.

Comment 6

[Jorg K (CEST = GMT+2)]

I tried a local backout of c0b4ca69376c	and that doesn't restore the function.

There were no Windows Daily builds on 21st and 22nd of Feb 2018. 20th still works for me, 23rd doesn't.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1499410-autocomplete-enter.patch

I noticed that in all other working cases in C-C we have oninput= as well, so I added that and things started working. Of course it's a big hack and to work around some bug in the autocomplete code.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9018833 [details] [diff] [review]
1499410-autocomplete-enter.patch

Ignore that. I'll explain in the next comment.

Comment 9

[Jorg K (CEST = GMT+2)]

I must have don't something wrong, local backout of rev c0b4ca69376c *does* restore the function.

Strangely enough TB uses autocomplete in a few places which are all accepting <enter>, only two usages don't work. These two don't have the oninput= that the other call sites have.

Comment 10

[Jorg K (CEST = GMT+2)]

Actually, the two panels where it doesn't work are modal, all the others aren't.

Comment 11

[Jorg K (CEST = GMT+2)]

Fixed for TB 60 ESR via backout:
https://hg.mozilla.org/releases/mozilla-esr60/rev/92515b2d8a74ec96119209101319a8b21e06c44d on THUNDERBIRD_60_VERBRANCH
Backed out changeset c0b4ca69376c (bug 1434837 for causing bug 1499410) to build Thunderbird 60.3. a=jorgk

Comment 12

[Marco Bonardo [:mak]]

I fear I'm not of help here, this seems to require specific debugging, you could maybe try to build a simple testcase of an autocomplete in a modal dialog to see if it can be used to reproduce the problem.

Comment 13

[Jorg K (CEST = GMT+2)]

Well, I launched the panel non-modal and the autocomplete still doesn't work. As I said, I'm utterly puzzled that very similar XUL configuration works in all other cases. Looks like Masayuki-san needs to shed some light on the event processing here.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, backing out bug 1434837 fixes this bug means that there are some Thunderbird specific "keypress" event listeners in the default group and they are added after autocomplete/satchel adds "keypress" event listener. So, previously, autocomplete/satchel consumed "keypress" events before them. However, now, the other listeners handle "keypress" event first, then, autocomplete/satchel's listener handles it later. You need to look for the "keypress" event listener and make it into the system group (use addSystemEventListener or addEventListener with {mozSystemGroup: true}).

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1499410-fix-mailinglist-enter.patch - WIP, experiment, not working

Hmm, so if I understand you correctly, there is a conflict between a listener we register and the autocomplete listener and our listener runs first and messes up the event.

I thought I had found the issue in abMailListDialog.js we indeed we add listeners to the now failing panel. So I changed them to
  document.addEventListener("keypress", awDocumentKeyPress, {capture: true, mozSystemGroup: true});
but that didn't help.

Next I took out various listeners I could see in DXR, which I admit doesn't make much sense and also has no effect. So how can I find the offending listener? We have heaps of hits on "keypress" in C-C:
https://dxr.mozilla.org/comm-central/search?q=keypress+path%3Acomm%2F+-path%3Acomm%2Fcalendar%2F&redirect=false
https://dxr.mozilla.org/comm-central/search?q=regexp%3Aaddeventlistener.*keypress+path%3Acomm%2F+-path%3Acomm%2Fcalendar%2F&redirect=false

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jorg K (GMT+2) from comment #15)
> Created attachment 9019031 [details] [diff] [review]
> 1499410-fix-mailinglist-enter.patch - WIP, experiment, not working
> 
> Hmm, so if I understand you correctly, there is a conflict between a
> listener we register and the autocomplete listener and our listener runs
> first and messes up the event.

Yes, I think so. Or, another possible case is, another keypress event listener is added by toolkit or something but the combination is not used in m-c.

> I thought I had found the issue in abMailListDialog.js we indeed we add
> listeners to the now failing panel. So I changed them to
>   document.addEventListener("keypress", awDocumentKeyPress, {capture: true,
> mozSystemGroup: true});
> but that didn't help.

Hmm, it's anyway odd. It listens "Enter" keypress *before* autocomplete module since it adds the event listener to capturing phase of the document node, but autocomplete attaches the editable element. I must not realize something...

> Next I took out various listeners I could see in DXR, which I admit doesn't
> make much sense and also has no effect. So how can I find the offending
> listener?

I also want to know... I always read implementation a lot when I meet unexpected orange at working on such changes...

If the consumer is in C++, you can get the stack with MOZ_ASSERT(); in dom::Event::PreventDefault() and/or dom::Event::StopProparation(). E.g.,

MOZ_ASSERT(!mEvent || !mEvent->AsKeyboardEvent() || mEvent->AsKeyboardEvent()->mKeyNameIndex != KEY_NAME_INDEX_Enter);

but I guess it's in JS...

> We have heaps of hits on "keypress" in C-C:
> https://dxr.mozilla.org/comm-central/search?q=keypress+path%3Acomm%2F+-
> path%3Acomm%2Fcalendar%2F&redirect=false
> https://dxr.mozilla.org/comm-central/search?q=regexp%3Aaddeventlistener.
> *keypress+path%3Acomm%2F+-path%3Acomm%2Fcalendar%2F&redirect=false

Or, I wonder, "accept" handler of XUL dialog might not work with autocomplete.

Comment 17

[Alice0775 White]

I do not understand the code, but If I change abMailListDialog.xul as follows, then the mailing list autocomplete works as expected.


           <textbox id="addressCol1#1" class="plain textbox-addressingWidget uri-element"
                    type="autocomplete" flex="1"
                    autocompletesearch="addrbook ldap"
                    autocompletesearchparam="{}" timeout="300" maxrows="4"
                    completedefaultindex="true" forcecomplete="true"
                    completeselectedindex="true"
                    minresultsforpopup="3"
-                   ontextentered="awRecipientTextCommand(param, this)"
+                   ontextentered="awRecipientTextCommand(param, this);event.preventDefault();"
-                   onkeypress="awHandleKeyPress(this, event);"
                    onkeydown="awRecipientKeyDown(event, this);"
                    onclick="awNotAnEmptyArea(event);">
             <image onclick="this.parentNode.select();" class="person-icon"/>

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> Hmm, it's anyway odd. It listens "Enter" keypress *before* autocomplete
> module since it adds the event listener to capturing phase of the document
> node, but autocomplete attaches the editable element. I must not realize
> something...
As you see in the patch, even removing the listener completely doesn't fix the issue.

(In reply to Alice0775 White from comment #17)
> -                   ontextentered="awRecipientTextCommand(param, this)"
> +                   ontextentered="awRecipientTextCommand(param, this);event.preventDefault();"
> -                   onkeypress="awHandleKeyPress(this, event);"
Thanks Alice, I'll play with this. Note that his pattern is used elsewhere and it works:
https://searchfox.org/comm-central/rev/f9a9b8d71d4898fef36bc6eea2dfde0e0e56ff35/mail/components/compose/content/messengercompose.xul#1962
So there must be other circumstances that lead to this malfunction.

Comment 19

[Jorg K (CEST = GMT+2)]

Alice's suggestion
-                   onkeypress="awHandleKeyPress(this, event);"
gets us closer to the issue.

With just that change, hitting enter accepts the autocomplete choice, and closes the panel. To avoid panel closing, that is, passing the event to the OK button, the function does this:

https://searchfox.org/comm-central/rev/ed8a923565e8de31d976ad58f0dc95850c0da275/mailnews/addrbook/content/abMailListDialog.js#28

function awHandleKeyPress(element, event)
{
  // allow dialog to close on enter if focused textbox has no value
  if (element.value != "" && event.keyCode == KeyEvent.DOM_VK_RETURN) {
    event.stopPropagation();
    event.preventDefault();
  }
}

So now this function is eating the DOM_VK_RETURN and it doesn't get to the auto-complete any more.

So it appears to be XUL's own onkeypress handler that now gets the event first before the autocomplete gets it. Masayuki-san, what can we do about that? That doesn't appear right. Shouldn't autocomplete as the top most "action" get the event first? Or I need to change awHandleKeyPress() to pass the event onto the autocomplete widget somehow.

I think I also understand why Alice's further suggestion works:
-                   ontextentered="awRecipientTextCommand(param, this)"
+                   ontextentered="awRecipientTextCommand(param, this);event.preventDefault();"

So once we remove the call to awHandleKeyPress(), the autocomplete gets the event, and eventually ontextentered gets run. With preventDefault() added, it will prevent the closing of the panel.

So with Alice's solution we can no longer close the panel with enter now when nothing got added.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1499410-fix-ml-enter.patch (v1)

This works. It's basically Alice's idea with an improvement. I killed the doubtful and badly named awHandleKeyPress() that cancelled DOM_VK_RETURN but assumed that auto-complete had already processed it (which is no longer true).

Instead, if a value was entered, I follow Alice's idea to prevent the default so the panel isn't closed. Is no value was entered, the event will go to the OK button and close the panel.

Maybe we can take this to the ESR as well and backout the backout of the Mozilla patch ;-)

Comment 21

[:aceman]

Comment on attachment 9019496 [details] [diff] [review]
1499410-fix-ml-enter.patch (v1)

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

Thanks, seems to work for me.

::: mail/components/addrbook/content/abMailListDialog.xul
@@ +78,1 @@
>                     onkeypress="awHandleKeyPress(this, event);"

Forgot to remove this one?

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1499410-fix-ml-enter.patch (v1b)

(In reply to :aceman from comment #21)
> >                     onkeypress="awHandleKeyPress(this, event);"
> Forgot to remove this one?
Oops. It still works since I removed the function :-)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d3daa2594870
fix mailing list enter handling (idea by Alice White). r=aceman

Comment 24

[Jorg K (CEST = GMT+2)]

Beta (TB 64):
https://hg.mozilla.org/releases/comm-beta/rev/dfaefe83ca91

Comment 25

[Jorg K (CEST = GMT+2)]

TB 60.3:
https://hg.mozilla.org/releases/comm-esr60/rev/ff097eea51d576adda0f75c2b28d29ee40100eaa
Undid the backout from comment #11 here:
https://hg.mozilla.org/releases/mozilla-esr60/rev/a3f82738156799bf9bf3948f54f40a1b0a1fb4f2

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jorg K (GMT+2) from comment #20)
> This works. It's basically Alice's idea with an improvement. I killed the
> doubtful and badly named awHandleKeyPress() that cancelled DOM_VK_RETURN but
> assumed that auto-complete had already processed it (which is no longer
> true).

Makes sense.

(In reply to Jorg K (GMT+2) from comment #19)
> So it appears to be XUL's own onkeypress handler that now gets the event
> first before the autocomplete gets it. Masayuki-san, what can we do about
> that? That doesn't appear right. Shouldn't autocomplete as the top most
> "action" get the event first? Or I need to change awHandleKeyPress() to pass
> the event onto the autocomplete widget somehow.

Really good point. The reason why autocomplete should handle keyboard events in the system group is, autocomplete is one of default action. Therefore, it should handle keyboard events after any event listeners in web content. And another reason is, keypress events won't be fired for non-printable keys unless the document is in chrome document.  So, in other words, in chrome XUL document, we could make autocomplete handle keyboard events in the default group for backward compatibility. However, on the other hand, we don't have any similar bugs for now. If we'd get similar reports, it could be better to change the group (I think that we're just lucky in this bug since you succeeded to fix this bug without too big/risky changes).

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(And really sorry for the delay. Currently, my queue is full. I'll reply each your ni request later.)