1106778 - Moving mouse out of autocomplete popup should clear selection, so TAB, RIGHT or ENTER will not confirm unexpected entry

Status: NEW

(Core :: XUL, defect)

Description

[Tomasz Ostrowski]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141113113219

Steps to reproduce:

Autocomplete popup selection follows mouse cursor. So moving mouse out of autocomplete popup should clear selection. Otherwise unexpected entry would be left selected - last entry on cursor way out. Using ENTER, TAB o RIGHT would then select this unexpected entry.

For example in Thunderbird:
1. Add "[e-mail]" and "[e-mail]" to addressbook.
2. Click "Write", write "addr" in "To" field, autocomplete should replace it to "[e-mail]" and show a popup to choose.
3. Move mouse over autocomplete popup entry [e-mail].
4. Move mouse over autocomplete popup entry [e-mail].
5. Move mouse down out of autocomplete popup entry [e-mail]. [e-mail] would stay selected, but "To" would stay autocompleted to "[e-mail]".
6. Press ENTER, TAB or RIGHT.



Actual results:

"To" would change to [e-mail] and confirm.
This can cause e-mail to be sent to a wrong person.


Expected results:

"To" would be confirmed to [e-mail].

Comment 1

[Tomasz Ostrowski]

Attachment: Proposed patch - clear autocomplete selection on mouseout

Proposed very small patch which would clear selection on moving mouse out of autocomplete popup.

Tested:
- would unselect on moving mouse cursor out;
- wrong entry would not be confirmed on ENTER, TAB, LEFT or RIGHT;
- UP/DOWN keys would behave normally after moving mouse out - DOWN will select first entry, UP will select last, just like when mouse was never moved;
- would not interfere with selecting with UP or DOWN until mouse moves;
- would select back on moving cursor in from up, down, left or right;
- would not select until cursor moves even when autocomplete popup will show under it.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8531239 [details] [diff] [review]
Proposed patch - clear autocomplete selection on mouseout

Sorry Tomasz -- I'm not the right reviewer for this, this is a XUL/Toolkit change.  Bouncing to Jan who I think might be better.

Comment 3

[Jan Varga [:janv]]

Comment on attachment 8531239 [details] [diff] [review]
Proposed patch - clear autocomplete selection on mouseout

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

Looks good to me, but I would also check with Neil Deakin.

::: toolkit/content/widgets/autocomplete.xml
@@ +1928,5 @@
>          }
>        ]]></handler>
> +
> +      <handler event="mouseout"><![CDATA[
> +         // Can't use clearSelection() because it does not reset currentindex

Nit: s/currentindex/currentIndex

Comment 4

[Tomasz Ostrowski]

Attachment: autocomplete-popup-mouseout-clear-selection.patch

Corrected "currentIndex" case in comment. Prepared for check-in according to guidelines in:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 5

[Neil Deakin]

Comment on attachment 8552996 [details] [diff] [review]
autocomplete-popup-mouseout-clear-selection.patch

Sorry to bump this around but Marco is the best person for autocomplete changes.

Comment 6

[Marco Bonardo [:mak]]

I'll look at this in the next days, sorry for the delay but I have many requests atm and autocomplete is always very regression-prone, so I need a bit of time to test this properly.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8552996 [details] [diff] [review]
autocomplete-popup-mouseout-clear-selection.patch

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

First of all sorry for the delay.
As I said, this kind of changes is sort of scary cause autocomplete code is rusty, things could have been made a certain way on purpose (but we don't have documentation) and users might be used to the current behavior.

I'd be willing to give this a try, since on paper it looks like an enhancement to avoid users hitting unexpected results and the patch is trivial (easy to backout in case of problems). The problem is that it will make the richlist autocomplete behavior different from treebody autocomplete behavior and different from the de-facto users habit.
Both autocomplete types differ from xpfe autocomplete, that makes me think someone made a decision in the past, but didn't document it in the code.

What I found so far, is bug 314386, that exactly removed the handler. We must be sure to not regress that and to document why we want a mouseout handler. instead.
We also need to find the bug that removed mouseout from the treebody implementation of autocomplete.
Finally, we need to check how the selection is managed in autocomplete lists of the various OS and the other major browsers, cause we don't want to surprise the user.

When we have this data, we can ask someone from UX or the browser owner, to sign off the behavior.

::: toolkit/content/widgets/autocomplete.xml
@@ +1932,5 @@
> +         // Can't use clearSelection() because it does not reset currentIndex
> +         // so using UP or DOWN keys after moving cursor out would start in
> +         // unexpected position
> +         this.parentNode.view.selection.select(-1);
> +      ]]></handler>

you can rather do this.parentNode.selectedIndex = -1

the problem is that I see some spurious mouseout events that make the selection flicker :(
So at a minimum you should filter events so that only mouseout from the richlistbox or the popup are considered.

Comment 8

[Tomasz Ostrowski]

(In reply to Marco Bonardo [::mak] from comment #7)
> What I found so far, is bug 314386, that exactly removed the handler. We
> must be sure to not regress that and to document why we want a mouseout
> handler. instead.

I've tested for this bug with this patch and it has not regressed.

> We also need to find the bug that removed mouseout from the treebody
> implementation of autocomplete.

I'm sorry but I don't even know what "treebody autocomplete" is. Where is it used?

I know that this bug makes users in my organization send emails to wrong address. And ask me to "unsend" it...

> Finally, we need to check how the selection is managed in autocomplete lists
> of the various OS and the other major browsers, cause we don't want to
> surprise the user.

Internet Explorer 11 - URL autocomplete:
- mouse out unselects (like with this patch);
- totally unrelated second selection when using keyboard arrows;
- keyboard does not confirm mouseover selection;

Internet Explorer 11 - form autocomplete:
- mouse out unselects (like with this patch);

Chrome on Windows 7 - URL autocomplete:
- mouse out unselects (like with this patch;
- totally unrelated second selection when using keyboard arrows;

Chrome on Windows 7 - form autocomplete:
- mouse out unselects (like with this patch;

> ::: toolkit/content/widgets/autocomplete.xml
> @@ +1932,5 @@
> > +         // Can't use clearSelection() because it does not reset currentIndex
> > +         // so using UP or DOWN keys after moving cursor out would start in
> > +         // unexpected position
> > +         this.parentNode.view.selection.select(-1);
> > +      ]]></handler>
> 
> you can rather do this.parentNode.selectedIndex = -1

Looks better. I'll retest and try to send updated patch.

> the problem is that I see some spurious mouseout events that make the
> selection flicker :(

I can't reproduce this flickering in today's trunk Thunderbird from Mercurial. Maybe it was caused by another bug - autocomplete popup window was behaving very erratically in nightly several days ago. Also it is broken in Thunderbird 38beta1 (see for example bug #1151764).

I've added to my build:
dump("mouseout\n");
and enabled preference "browser.dom.window.dump.enabled" and only see expected firings in console.

Comment 9

[Brian Norris]

This may qualify as a separate complaint, which may deserve a different bug/feature-request, but I'll post it here in case it is informative:

It seems like there is a usability issue here even when I haven't moused away from the second autocomplete entry ("address2@..." in the STR). Consider this:

0. type "address" in the "To:"
1. "[e-mail]" autocompletes in the main text box and remains highlighted
2. auto complete shows a dropdown with "[e-mail]" and "[e-mail]"
3. mouse over "[e-mail]"
4. press TAB or ENTER
5. "To:" changes to "[e-mail]"

The problem is that there is no definitive visual cue that in steps 3 to 5 we are about to change to a different autocomplete address, since the address from step 1 is still highlighted in the "To:" box. It kinda places equal emphasis on the original autocomplete suggestion and the mouseover suggestion, which is misleading, IMO.

So IMO, either:
(a) mouse-over should never be treated as an autocomplete suggestion (and it should require a "click") or else
(b) the 'To:' box should make a visual cue to show that the autocomplete is no longer considering "[e-mail]" as its primary choice

FWIW, a certain well-known proprietary mail client already does the mouseover-to-autocomplete, but it just doesn't do any autocomplete in the main address box until a choice has been made explicit by the user. I think this solves the confusion problem, in the vein of option (b).

Anyway, as I said, this might be a different feature request, but it seems related to the confusing behavior described in this bug. I can file a new ticket if that's deemed best.

Comment 10

[Tomasz Ostrowski]

(In reply to Tomasz Ostrowski from comment #8)

> > you can rather do this.parentNode.selectedIndex = -1
> 
> Looks better. I'll retest and try to send updated patch.

This does not work. Original patch works.

I'd be really grateful if this patch would land in TB38. Several days ago another user in my organization did not notice that he moved a cursor over the popup and has sent internal mail to several hundred addresses and without using BCC, because autocomplete changed to list name. This is very serious bug.

Comment 11

[custom.firefox.lady [:tawn]]

Related to Bug 1167479 ?

Comment 12

[Tomasz Ostrowski]

(In reply to custom.firefox.lady from comment #11)
> Related to Bug 1167479 ?

Almost the same.

Sometimes confirming with outside click isn't bad - when you choose selection with keyboard arrows and click at body of message.

I got so desperate that I tried to create an extension to correct this, but I did not manage to reference autocomplete popup in message compose window DOM:
http://stackoverflow.com/q/30328311/15862

It seems that developers are always using Enter or Tab to switch to message body - it's natural to them. Ordinary users often use mouse and get bitten.

Comment 13

[Thomas D. (:thomas8)]

This looks like a duplicate of Bug 1152517, which has just been fixed for the Toolkit//Autocomplete product//component, target milestone mozilla42. This bug here has different product/component: Core//XP Toolkit/Widgets: XUL

Comment 14

[K.J. Petrie]

I agree with Brian Norris.

It seems there is something seriously amiss with the design that patching for odd isolated scenarios won't fix.

There is a difference between highlighting with a mouse hover and highlighting with the arrow keys, in that in one case the user expects no selection to occur on pressing Enter and in the other they do.

However, this is surely not so much about autocomplete as about dropdown menu behaviour. I haven't looked at the code, but I wonder whether autocomplete is doing too much. Surely, its job should be just to generate the menu, with the menu selection code being a separate and standardised function.

Comment 15

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.