Closed Bug 1437222 Opened 4 years ago Closed 4 years ago

Address auto-complete ignores click under certain circumstances

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(1 file, 1 obsolete file)

I've noticed this when reviewing the patch for bug 1436764.

Address auto-complete ignores click under certain circumstances.

STR:
Start a new message, type something into the To: field that will trigger auto-complete.
Use the down-arrow to select an entry. The value is copied into the field.
Now click another item. The click is ignored and the previously selected item stays.

If you remove the content of the field, then try again with the same steps, the click is obeyed.

This is pretty critical since users will send e-mail to the wrong person.

Marco, can you advise us here. This seems to be a regression from bug 1427366. Auto-complete has been pretty stable for a long time now.

I don't know whether you can reproduce anything like this in FF. I tried a Nightly with the same steps on the URL bar, and the click was always obeyed.
Flags: needinfo?(mak77)
Actually, when the click gets ignored, it doesn't really get ignored. In an opt build you can see a short flicker of the field, and in a slower debug version you can see that the arrow-key-selected value that was in the field gets overwritten by the clicked value, just to be overwritten again by the previous value.
I've backed out bug 1427363 (part 1 and part 2), and the behaviour didn't change, then bug 1427366 (and C-C port, bug 1436764) and then it went back to "normal".

So should this small change
https://hg.mozilla.org/mozilla-central/rev/63c152ad62b0#l4.12
cause this by upsetting out logic in
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/addressingWidgetOverlay.js
Blocks: 1427366
Yes, the
-            popup.setAttribute("type", "autocomplete");
+            popup.setAttribute("type", "autocomplete-richlistbox");
makes all the difference.
(In reply to Jorg K (GMT+1) from comment #3)
> Yes, the
> -            popup.setAttribute("type", "autocomplete");
> +            popup.setAttribute("type", "autocomplete-richlistbox");
> makes all the difference.

That is moving from the old tree binding to the richlistbox binding. It's not a small change, it's basically the core of the change.
The 2 bindings may act differently in certain circumstances, it could be a bug on either side (consumer or controller), it's hard to tell without some debugging.
Is addressingWidgetOverlay.js the the code that lies on top of your autocomplete? Which part of it is likely to handle these selection events?

You should probably follow the click handler and get a stack there, or write down notes about the followed logic, unfortunately I don't know TB code enough to tell how it's interacting with autocomplete and the autocomplete controller. If you can help me finding that hook point, that'd be useful.

Also, I suspect TB will shortly start having big troubles trying to follow XBL removal, probably it's just a gut feeling, but I feel like it would be much easier for you to fork before the large part of that hits you. It will be hard to find resources in the FX team to help, considered we're already overloaded. Don't take this too negatively, you're going a great job, it's just that I can imagine incoming troubles.
Marco, thanks for the comment. I've just extinguished another fire, so I'll look into this further tonight.

(In reply to Marco Bonardo [::mak] from comment #4)
> If you can help me finding that hook point, that'd be useful.
I have to find it myself first ;-( - I'll get back to you.
It is really strange that the problem only happens on the first auto-complete we run, the subsequent ones work.

> Also, I suspect TB will shortly start having big troubles trying to follow
> XBL removal, ...
We've been adopting removed bindings left, right and centre. The problem about a fork is dealing with security updates. We'll reconsider our position after we made it to TB 60 ESR, going to beta in about a month.
Attached patch autocomplete-debug-patch.patch (obsolete) — Splinter Review
I did debugging with the attached patch. The popup shows a list of recipients matching the letter "j".

Pam comes before Jeff in the list, so I down-arrow to Jeff, and then click on Pam.

The first time around, I see this debug:
=== >>> nsAutoCompleteController::HandleEnter
=== >>> nsAutoCompleteController::EnterMatch
===     nsAutoCompleteController::EnterMatch, inputValue is ||
===     nsAutoCompleteController::EnterMatch, (1) set |Pam Jordan <pamjordanpj@hotmail.com>|
===     nsAutoCompleteController::EnterMatch, (1) value is |Pam Jordan <pamjordanpj@hotmail.com>|
===     nsAutoCompleteController::EnterMatch, (2) value is |Pam Jordan <pamjordanpj@hotmail.com>|
=== <<< nsAutoCompleteController::EnterMatch
=== <<< nsAutoCompleteController::HandleEnter
=== >>> nsAutoCompleteController::HandleEnter
=== >>> nsAutoCompleteController::EnterMatch
===     nsAutoCompleteController::EnterMatch, inputValue is ||
===     nsAutoCompleteController::EnterMatch, (3) set |Jeff Laing <Jeff.Laing@synchronoss.com>|
===     nsAutoCompleteController::EnterMatch, (1) value is |Jeff Laing <Jeff.Laing@synchronoss.com>|
===     nsAutoCompleteController::EnterMatch, (2) value is |Jeff Laing <Jeff.Laing@synchronoss.com>|
=== <<< nsAutoCompleteController::EnterMatch
=== <<< nsAutoCompleteController::HandleEnter

So the clicked "Pam" is correctly selected at (1), then a second HandleEnter/EnterMatch is processed and "Jeff" is selected at (3).

Then I delete the input and type "j" again to trigger another auto-complete. This time I see:
=== >>> nsAutoCompleteController::HandleEnter
=== >>> nsAutoCompleteController::EnterMatch
===     nsAutoCompleteController::EnterMatch, inputValue is ||
===     nsAutoCompleteController::EnterMatch, (1) set |Pam Jordan <pamjordanpj@hotmail.com>|
===     nsAutoCompleteController::EnterMatch, (1) value is |Pam Jordan <pamjordanpj@hotmail.com>|
===     nsAutoCompleteController::EnterMatch, (2) value is |Pam Jordan <pamjordanpj@hotmail.com>|
=== >>> nsAutoCompleteController::HandleEnter
=== >>> nsAutoCompleteController::EnterMatch
===     nsAutoCompleteController::EnterMatch, (1) value is ||
===     nsAutoCompleteController::EnterMatch, (2) value is |Pam Jordan <pamjordanpj@hotmail.com>|
=== <<< nsAutoCompleteController::EnterMatch
=== <<< nsAutoCompleteController::HandleEnter
=== <<< nsAutoCompleteController::EnterMatch
=== <<< nsAutoCompleteController::HandleEnter

So "Pam" is correctly set at (1) and another HandleEnter/EnterMatch call happens *before* the first one has returned. In the second call, nothing new is selected.

So I set a breakpoint on the setting of Jeff at (3) and got this stack:
xul.dll!nsAutoCompleteController::EnterMatch(bool aIsPopupSelection, nsIDOMEvent * aEvent) Line 1499	C++
xul.dll!nsAutoCompleteController::HandleEnter(bool aIsPopupSelection, nsIDOMEvent * aEvent, bool * _retval) Line 342	C++
xul.dll!_NS_InvokeByIndex() Line 57	Unknown

And the JS stack at that point is:
0 onxblblur(event = [object FocusEvent]) ["chrome://global/content/bindings/autocomplete.xml":657]
    this = [object XULElement]
1 _awSetFocus() ["chrome://messenger/content/messengercompose/addressingWidgetOverlay.js":749]
    this = [object ChromeWindow]

So we seem to set focus here:
https://dxr.mozilla.org/comm-central/rev/8c7e7fdff34e61378d8e8cf1067f92bce66489cb/mail/components/compose/content/addressingWidgetOverlay.js#749

I'm not sure where to take if from here. As I said, it's weird that the first and the second use of the auto-complete field behave differently.
Some more stacks:
In the "good" case, the first invocation of HandleEnter/EnterMatch comes form JS onxblmouseup and onPopupClick, the second from this JS call stack, you can see the first call in there:
0 onxblblur(event = [object FocusEvent]) ["chrome://global/content/bindings/autocomplete.xml":657]
    this = [object XULElement]
1 select() ["chrome://global/content/bindings/textbox.xml":115]
    this = [object XULElement]
2 awReturnHit(inputElement = [object XULElement]) ["chrome://messenger/content/messengercompose/addressingWidgetOverlay.js":530]
    this = [object ChromeWindow]
3 awRecipientTextCommand(enterEvent = [object MouseEvent], element = [object XULElement]) ["chrome://messenger/content/messengercompose/addressingWidgetOverlay.js":850]
    this = [object ChromeWindow]
4 anonymous(eventType = "textentered", param = [object MouseEvent]) ["chrome://global/content/bindings/autocomplete.xml line 423 > Function":3]
    this = [object XULElement]
5 anonymous([object MouseEvent]) ["self-hosted":1035]
    this = [object XULElement]
6 onTextEntered(event = [object MouseEvent]) ["chrome://global/content/bindings/autocomplete.xml":257]
    this = [object XULElement]
7 onPopupClick(aEvent = [object MouseEvent]) ["chrome://global/content/bindings/autocomplete.xml":759]
    this = [object XULElement]
8 onxblmouseup(event = [object MouseEvent]) ["chrome://global/content/bindings/autocomplete.xml":2239]
    this = [object XULElement]

In the "bad" case, the first invocation equally comes from onxblmouseup and onPopupClick, the second one comes from onxblblur/_awSetFocus as mentioned in the previous comment.

So the difference here seems to be that in the "good" case, the click leads to an onTextEntered() being triggered, and it's good from there. In the "bad" case, that doesn't happen, so we later focus and pick up the previous selection and ignore the click.
Does this field set forcecomplete="true"?
there may be a bug there, probably forcecomplete should exclude other cases where it now invoked handleEnter when it shouldn't.
https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/toolkit/content/widgets/autocomplete.xml#638
forcecomplete is only used by comm-central, some of this code still relies on "mouseover changes the selected index" behavior, that was recently (a few versions ago) changed to "mouseover does not change the selectedIndex"
Since only comm-central uses this attribute, you can change it as you see fit for your use case.
It was modified in bug 1152517, maybe now that mouseover doesn't select all of this fix is pointless.
Flags: needinfo?(mak77)
Yes, we use forcecomplete="true" here. I'm still not sure why that would produce different outcome the first and second time auto-complete is used on the same field, but I can pursue it with the forcecomplete code in mind.

Don't the debugging results in comment #6 and comment #7 look fishy to you? First time around, the click doesn't appear to cause the onTextEntered() being triggered, so the click is just ignored (see the last paragraph of my comment #7).
When my local build finishes, I'll take the forcecomplete="true" out and see what happens.
Works with forcecomplete="true" removed. Thanks for the hint, I'll look into it further.
Just for the record, without forcecomplete there's only one call to HandleEnter/EnterMatch and that does the right thing:
=== >>> nsAutoCompleteController::HandleEnter
=== >>> nsAutoCompleteController::EnterMatch
===     nsAutoCompleteController::EnterMatch, inputValue is ||
===     nsAutoCompleteController::EnterMatch, (1) set |Pam Jordan <pamjordanpj@hotmail.com>|
===     nsAutoCompleteController::EnterMatch, (1) value is |Pam Jordan <pamjordanpj@hotmail.com>|
===     nsAutoCompleteController::EnterMatch, (2) value is |Pam Jordan <pamjordanpj@hotmail.com>|
=== <<< nsAutoCompleteController::EnterMatch
=== <<< nsAutoCompleteController::HandleEnter
Component: Message Compose Window → Autocomplete
Product: Thunderbird → Toolkit
Marco, thank you for the inspiration ;-)

This is the result of some experimentation. First I reverted
https://hg.mozilla.org/mozilla-central/rev/c36bb974792d

This didn't have any effect. Next I removed the
  if (this.forceComplete && this.mController.matchCount >= 1) {
    this.mController.handleEnter(false);
altogether. That fixed the problem, but also removed the forcing on blur, which is required.

Next I tried what I am presenting here, so
  if (this.forceComplete && this.mController.matchCount >= 1) {
    this.mController.handleEnter(true);
and that works.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8950747 - Flags: review?(mak77)
Comment on attachment 8950747 [details] [diff] [review]
1437222-autocomplete-forceComplete.patch (v1)

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

Cool!
You may want to add some kind of tests in the future for this, to better cover yourself from our changes.
Attachment #8950747 - Flags: review?(mak77) → review+
(In reply to Jorg K (GMT+1) from comment #9)
> Don't the debugging results in comment #6 and comment #7 look fishy to you?

It looked fishy because forcecomplete was manipulating the selection and then calling a second handleEnter on the new selection.
I honestly don't recall all the details around the functionality of forcecomplete and the expected outcome.
(In reply to Marco Bonardo [::mak] from comment #15)
> It looked fishy because forcecomplete was manipulating the selection and
> then calling a second handleEnter on the new selection.
Indeed, that what I found out, too.

> I honestly don't recall all the details around the functionality of
> forcecomplete and the expected outcome.
Well, I read here https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Textbox_(Toolkit_autocomplete)#a-forcecomplete:
===
If true, the textbox will be filled in with the best match when it loses the focus. If false, it will only be filled in when the user selects an item.
===

I guess that's what we want. If they type "marco" and the address book has "Marco", we want "Marco". Also, as the comment now explains, we need to use something on blur.

Looking through autocomplete.xml and nsAutoCompleteController.cpp, there is surprisingly little code for forcecomplete, and even less now, so I think the burden is not so big.

Thanks for the quick review at midnight, I saw that you have a long queue if things more important for Firefox.
Keywords: checkin-needed
Attachment #8950360 - Attachment is obsolete: true
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea3a251c920
fix forceComplete after switching to autocomplete-richlistbox. r=mak
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ea3a251c920
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.