Closed Bug 1486178 Opened Last year Closed Last year

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

Categories

(Toolkit :: Autocomplete, defect)

60 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: silekonn, Assigned: jorgk-bmo)

References

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

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.
confirmed. The key step is using scrollbar, either clicking arrow or dragging causes selection
Status: UNCONFIRMED → NEW
Component: Untriaged → Message Compose Window
Ever confirmed: true
Keywords: regression
Summary: Partial manual contact entry followed by clicking scrollbar auto-selects top contact → Partial manual contact entry, followed by clicking scrollbar, auto-selects top autocomplete contact
Sigh, most likely a regression from bug 1457085.
Something CSS can fix? Broken on trunk? (Sorry, afk right now.)
Flags: needinfo?(richard.marti)
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.
Flags: needinfo?(richard.marti)
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.
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.
Flags: needinfo?(alice0775)
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 ;-)
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.
OK, found the cause. It's a bug in auto-complete if the auto-complete is embedded into another richlist list.
Flags: needinfo?(alice0775)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Component: Message Compose Window → Autocomplete
Product: Thunderbird → Toolkit
Summary: Partial manual contact entry, followed by clicking scrollbar, auto-selects top autocomplete contact → 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
Version: 60 → 60 Branch
Attached patch 1486178-mouseup.patch (v1) (obsolete) — Splinter Review
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?
Attachment #9004094 - Flags: review?(paolo.mozmail)
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.
Flags: needinfo?(gijskruitbosch+bugs)
Attached patch 1486178-mouseup.patch (v2) (obsolete) — Splinter Review
Thanks, Paolo. Based on your comment, here is a more elegant solution which appears to be working as well.
Attachment #9004139 - Flags: review?(paolo.mozmail)
Attachment #9004139 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #9004094 - Attachment is obsolete: true
Attachment #9004094 - Flags: review?(paolo.mozmail)
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.
Attachment #9004139 - Flags: review?(paolo.mozmail)
Attachment #9004139 - Flags: review?(gijskruitbosch+bugs)
Attachment #9004139 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #9004139 - Flags: feedback+
Attached patch 1486178-mouseup.patch (v3) (obsolete) — Splinter Review
I found another call site using the same incorrect parent search. Can you please take another look.
Attachment #9004139 - Attachment is obsolete: true
Attachment #9004139 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #9004183 - Flags: review?(gijskruitbosch+bugs)
Attachment #9004183 - Flags: feedback?(paolo.mozmail)
Attached patch 1486178-mouseup.patch (v3b) (obsolete) — Splinter Review
Sorry, trailing spaces removed.
Attachment #9004183 - Attachment is obsolete: true
Attachment #9004183 - Flags: review?(gijskruitbosch+bugs)
Attachment #9004183 - Flags: feedback?(paolo.mozmail)
Attachment #9004184 - Flags: review?(gijskruitbosch+bugs)
Attachment #9004184 - Flags: feedback?(paolo.mozmail)
Comment on attachment 9004184 [details] [diff] [review]
1486178-mouseup.patch (v3b)

Isn't it guaranteed that "item" is not null?
Attachment #9004184 - Flags: feedback?(paolo.mozmail)
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.
(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.
Attachment #9004184 - Flags: review?(gijskruitbosch+bugs)
Now without the null check.
Attachment #9004184 - Attachment is obsolete: true
Attachment #9004283 - Flags: review?(gijskruitbosch+bugs)
Attachment #9004283 - Flags: feedback?(paolo.mozmail)
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.
Attachment #9004283 - Flags: feedback?(paolo.mozmail) → feedback+
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?
(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.
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 on attachment 9004283 [details] [diff] [review]
1486178-mouseup.patch (v4)

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

LGTM, thanks!
Attachment #9004283 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by dluca@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f6d7cbceaa1a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.