1486178 - mouseup on scrollbar not ignored if auto-complete widget embedded in richlistbox - Was: Thunderbird - Partial manual contact entry, followed by clicking scrollbar, auto-selects top autocomplete contact

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Silekonn]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36

Steps to reproduce:

Open TB and Write. Select the To: field.  Type a letter that begins the contact name (e. g. "e").  Using the mouse, click the scrollbar's down arrow.


Actual results:

The first entry in the list appears in the To: field.


Expected results:

The list should have scrolled down.  Nothing should be finalized in the To: field.

Comment 1

[Wayne Mery (:wsmwk)]

confirmed. The key step is using scrollbar, either clicking arrow or dragging causes selection

Comment 2

[Jorg K (CEST = GMT+2)]

Sigh, most likely a regression from bug 1457085.

Comment 3

[Jorg K (CEST = GMT+2)]

Something CSS can fix? Broken on trunk? (Sorry, afk right now.)

Comment 4

[Richard Marti (:Paenglab)]

I see no chance for CSS. I tried different things without success. Something must be different between this autocomplete and the one from "Insert Link" dialog because this one doesn't close when clicking on the scrollbar. It seems the the onkeyup on the scrollbar is also used in the textbox.

Comment 5

[Jorg K (CEST = GMT+2)]

Thanks for looking into it, your comment helps. The difference is that address entry is forced:
https://searchfox.org/comm-central/rev/ed81287c9c6ef0a55561c639608cd525bf2ec3e3/mail/components/compose/content/messengercompose.xul#1962

I have to see where "key up" is processed. Yes, "key up" should select the row, I need to see whether we can recognise that it's coming from the scrollbar. 

I thought CSS could disallow some events, but anyway.

Comment 6

[Jorg K (CEST = GMT+2)]

We've done quite a few changes in recipient auto-complete over the last year, so an exact regression window might be helpful. Alice, can you do this for us.

STR:
Compose a message.
Start typing a recipient, wait for the autocomplete pop-up to appear.
(Make sure you have enough addresses in your address book to a scrollbar appears.)
Try using the scrollbar. Once you let of the mouse button (key up), the popup is dismissed
and the selected address is "committed".

Before letting go of the mouse button didn't do anything.

Please do the check on Daily.

Comment 7

[Jorg K (CEST = GMT+2)]

Oops, I meant to write "let *go* of the mouse button".

These bugs changed auto-complete on trunk:
Bug 1475817: Patch landed in bug 1475817 comment #83 (!!) on 2018-07-18 10:54 CEST.
Bug 1437222: 2018-02-16 11:46 CET
Bug 1427366: 2018-02-08 11:11 CET
Bug 1436764: 2018-02-10 01:14 CET

So I'd say February of July 2018, but I might be wrong ;-)

Comment 8

[Jorg K (CEST = GMT+2)]

I've backed out bug 1437222 locally, and that didn't break it.

Marco, we meet again over another auto-complete problem :-( - As you know, address entry is the first and foremost function of an e-mail client and it just has to work.

I did a bit of debugging in autocomplete.xml. This code:
      <handler event="blur" phase="capturing"><![CDATA[
        if (!this._dontBlur) {
          if (this.forceComplete && this.mController.matchCount >= 1) {
            // If forceComplete is requested, we need to call the enter processing
            // on blur so the input will be forced to the closest match.
            // Thunderbird is the only consumer of forceComplete and this is used
            // to force an recipient's email to the exact address book entry.
            dump("======= we're in blur here\n");
            this.mController.handleEnter(true);
          }
          if (!this.ignoreBlurWhileSearching)
            this.detachController();
        }
      ]]></handler>
gets executed on "mouse up" from the scrollbar. So of course we then accept the selection.
How can we detect and avoid this? Oops, can't NI :mak right now, but he should be back tomorrow.

Comment 9

[Jorg K (CEST = GMT+2)]

OK, found the cause. It's a bug in auto-complete if the auto-complete is embedded into another richlist list.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1486178-mouseup.patch (v1)

Paolo, can you take this review or should I wait for Marco?

In the loop I see these elements when letting go of the mouse on the scrollbar:

scrollbar
scrollbox
richlistbox
panel
popupset
---
textbox
hbox
richlistitem

The ones below the line belong to Thunderbird's XUL, so detecting the outer TB richlistitem is a bug.

Should I exit the loop on finding the richlistbox or panel or the popupset?

Comment 11

[:Paolo Amadini]

Thanks for the fix! Based on the comment, it's correct for this to stop when reaching the "richlistbox". I can review this, but I'm wondering if we can now use this simpler pattern here:

https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/browser/components/places/content/editBookmark.js#856-859

There should always be a "richlistbox" so the result of "closest" can't be null. Gijs, do you see any potential performance issues with the selector? I actually guess using "closest" might faster since we don't have iterate all the parent nodes in JavaScript.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1486178-mouseup.patch (v2)

Thanks, Paolo. Based on your comment, here is a more elegant solution which appears to be working as well.

Comment 13

[:Paolo Amadini]

Comment on attachment 9004139 [details] [diff] [review]
1486178-mouseup.patch (v2)

Thanks! I'll let Gijs make the final call, this may have to wait until tomorrow.

nits: add braces around single-line statement for new code. You may also check for == "richlistitem" and have onPopupClick as the body of the statement.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1486178-mouseup.patch (v3)

I found another call site using the same incorrect parent search. Can you please take another look.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1486178-mouseup.patch (v3b)

Sorry, trailing spaces removed.

Comment 16

[:Paolo Amadini]

Comment on attachment 9004184 [details] [diff] [review]
1486178-mouseup.patch (v3b)

Isn't it guaranteed that "item" is not null?

Comment 17

[Jorg K (CEST = GMT+2)]

Good question. Better safe than sorry, no? The original code tested 'item' in the loop, but that was there to cater for the parent eventually becoming null.

Looking at the elements listed in comment #10, how can I be sure not to hit the panel or popupset outside the richlistbox? Clicking outside the widget altogether would not deliver the mouse event to those methods, I suppose.

Comment 18

[:Paolo Amadini]

(In reply to Jorg K (GMT+2) from comment #17)
> Good question. Better safe than sorry, no? The original code tested 'item'
> in the loop, but that was there to cater for the parent eventually becoming
> null.

That's the reason the original null check was there, yes. Now that we changed the code, we should remove it, because we wouldn't have added it if we were writing this new code from scratch. If in the future someone changed other code and broke this invariant, it would be better to see an exception rather than failing silently. It would make it obvious why the event handler doesn't work anymore.

> Looking at the elements listed in comment #10, how can I be sure not to hit
> the panel or popupset outside the richlistbox? Clicking outside the widget
> altogether would not deliver the mouse event to those methods, I suppose.

Yes, <handler> defines listeners only for the XBL element itself or its children.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1486178-mouseup.patch (v4)

Now without the null check.

Comment 20

[:Paolo Amadini]

Comment on attachment 9004283 [details] [diff] [review]
1486178-mouseup.patch (v4)

Thanks! The condition in "mouseup" could still be inverted, but it mirrors the "mousemove" one, so either way works.

Comment 21

[Jorg K (CEST = GMT+2)]

Indeed ;-)

(In reply to :Paolo Amadini from comment #13)
> Thanks! I'll let Gijs make the final call, this may have to wait until
> tomorrow.
Not sure why you chose Gijs and what the "tomorrow" (now today) is about. Now that Marco is back, should he review it?

Comment 22

[:Paolo Amadini]

(In reply to Jorg K (GMT+2) from comment #21)
> Not sure why you chose Gijs and what the "tomorrow" (now today) is about.

I asked Gijs because his main focus is currently on browser performance and we discussed a similar use of CSS selectors before. As for the delay, yesterday was a holiday so I didn't expect him to be around.

> Now that Marco is back, should he review it?

I'd just like to make sure we're not introducing a problematic pattern by using "closest", since I expect that over time we'll convert other occurrences of these loops. Unless I'm missing something, I think it'll be fine, so if we don't hear from Gijs we can land this later today and file a follow-up afterwards if needed.

Comment 23

[Jorg K (CEST = GMT+2)]

Thanks for the information. Just some timing information: As you'll understand, address entry is one of the most important actions in an e-mail client. I intend to take that fix to TB's release branch on m-esr60 before our next ESR release at the beginning of September. So we have a few days ;-) - I doubt the change would be accepted on m-esr60.

Comment 24

[:Gijs (he/him)]

Comment on attachment 9004283 [details] [diff] [review]
1486178-mouseup.patch (v4)

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

LGTM, thanks!

Comment 25

[Jorg K (CEST = GMT+2)]

Thanks, Gijs!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d7cbceaa1a
fix mouse-up and mouse-move handling in auto-complete if widget embedded in richlistbox. r=Gijs

Comment 27

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f6d7cbceaa1a

Comment 28

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/8d09365b956a2cebc8dc4d959b533c7da9ecf655 (THUNDERBIRD_60_VERBRANCH)