1302472 - Cursor-key-selected value in popup cannot be confirmed with <enter> (Thunderbird with completedefaultindex only)

Status: VERIFIED FIXED

(Toolkit :: Autocomplete, defect, P1)

Description

[Calum Mackay]

When composing a new message, using the keyboard to select a recipient from the auto-complete drop-down, the wrong recipient is filled into the field.

This problem first appeared in Daily 20160909 (and continues to e.g. 0913)

Daily 0908 does not have this problem.

I've marked it "major", due to the likelihood of the email then going to the wrong person, if the user does not spot the wrong fill (which has happened to me).


To reproduce:

• compose new mail, To: whoever

• in next recipient box below, type first 2 letters of a different recipient

• auto-complete drop-down appears; box is pre-filled with first entry from drop-down

• move down the drop-down list using keyboard cursor-down, to a different recipient (not the first pre-filled entry)

• enter/return to select that recipient: drop-down disappears

•notice that the wrong, originally pre-filled recipient remains (the first entry in the drop-down), not the one selected via keyboard cursor/enter from the list


It seems that if you select from the drop-down using the mouse, it is OK. It is wrong if the drop-down entry is selected via the keyboard.

Comment 1

[Wayne Mery (:wsmwk)]

Does it also happen if you have thunderbird started in safe mode?
(is typically good to have this test result in a bug report)

Comment 2

[Calum Mackay]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #1)
> Does it also happen if you have thunderbird started in safe mode?
> (is typically good to have this test result in a bug report)

Sorry Wayne, should have remembered that.

Yes, reproduced with 0913 in safe mode.

Comment 3

[Jorg K (CEST = GMT+2)]

Yes, reproducible in Daily of yesterday.

Alice, could you find us the regression. Look like some autocomplete change in M-C between 2016-09-08 and 2016-09-09 as per comment #0.

Comment 4

[Alice0775 White]

Last Good: http://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1473329073/
           c-c https://hg.mozilla.org/comm-central/rev/3b028c289baa00abbc8666d9873e3162af6f1bb0

First Bad: http://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1473415341/
           c-c https://hg.mozilla.org/comm-central/rev/3b028c289baa00abbc8666d9873e3162af6f1bb0



There are no m-c change set information.
I cannot find regression range in m-c until fix Bug 1297198

Comment 5

[Jorg K (CEST = GMT+2)]

Thanks!
Oops, you pasted the same C-C link twice. Can you paste the link to search the M-C pushlog by date, I'll have a look. The regression range appears to be 24 hours only.

Comment 6

[Alice0775 White]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #5)
> Thanks!
> Oops, you pasted the same C-C link twice.

good and bad build has same c-c.
and no m-c information in the builds.

> Can you paste the link to search
> the M-C pushlog by date, I'll have a look. The regression range appears to
> be 24 hours only.

Maybe.
https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2016-09-08&enddate=2016-09-09+04%3A0%3A00

I guess the culprit(nsAutoCompleteController.cpp) is
	c35d8f056f67	Marco Bonardo — Bug 1292310 - Enter may wrongly confirm a mouse selected entry in the urlbar. r=adw

Comment 7

[Jorg K (CEST = GMT+2)]

Marco, your changes in bug 1292310 seem to have affected the address entry in the Thunderbird addressing widget. We can no longer confirm an address with the enter key that is selected with the cursor keys or by hovering the mouse cursor over it. At least the former is an essential feature, we've had complaints about the latter, so no harm done if the mouse hovered one is no longer selected with enter.

Can you please advise.

Comment 8

[Marco Bonardo [:mak]]

interesting, could you please point me to the tb code that contains this widget? I mostly would like to know which attributes are set on the input field.

(In reply to Jorg K (GMT+2, PTO during summer) from comment #7)
> We can no longer confirm an address
> with the enter key that is selected with the cursor keys or by hovering the
> mouse cursor over it.

The latter was a bug afaik, hovering an item with the mouse and pressing enter is not expected to confirm that entry, otherwise it's very easy to get the wrong result by touching a touchpad or having the cursor in the area where the popup appears.

The former is not expected clearly, selecting an entry with the keyboard and confirming it should always confirm that entry, and we should figure out why now it behaves wrongly in TB and not in FF.

Comment 9

[Marco Bonardo [:mak]]

I suspect your form has completeDefaultIndex but not completeSelectedIndex?
and then we don't enter this anymore:
if (aIsPopupSelection || (!completeSelection && !defaultCompleted))
correct?

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #7)
> We can no longer confirm an address
> with the enter key that is selected with the cursor keys or by hovering the
> mouse cursor over it. At least the former is an essential feature, we've had
> complaints about the latter, ...
Further reading on this: Bug 1192739. Repeating: No harm done when the last hovered address can't be confirmed with <tab> or <enter> any more, but using the arrow keys should allow to change the selection. The perfect solution would be to get the FF behaviour into TB, that is, if you use the arrow key, the field content is actually changed. Bug 1192739 says:
===
Firefox does the following:
- Hovering changes the "selected" value and does not update the "filled" value.
- Arrow keys and tab change the "selected" value *and* the "filled" value is updated.
- Enter confirms the "filled" value.
===
We'd like that!

Comment 11

[Marco Bonardo [:mak]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #10)
> (In reply to Jorg K (GMT+2, PTO during summer) from comment #7)
> > We can no longer confirm an address
> > with the enter key that is selected with the cursor keys or by hovering the
> > mouse cursor over it. At least the former is an essential feature, we've had
> > complaints about the latter, ...
> Further reading on this: Bug 1192739. Repeating: No harm done when the last
> hovered address can't be confirmed with <tab> or <enter> any more, but using
> the arrow keys should allow to change the selection. The perfect solution
> would be to get the FF behaviour into TB, that is, if you use the arrow key,
> the field content is actually changed.

for this you should only add completeSelectedIndex="true" attribute to the input field.

Btw, I can look into the regression and add a test case for this, that looks like we are missing.

Comment 12

[Jorg K (CEST = GMT+2)]

Marco, that's for the feedback. There was a mid-air collision.

(In reply to Marco Bonardo [::mak] from comment #8)
> interesting, could you please point me to the tb code that contains this
> widget? I mostly would like to know which attributes are set on the input
> field.
I'll have to attend to some other issues, I'll get back to you asap, also re. comment #9.

> The latter was a bug afaik, hovering an item with the mouse and pressing
> enter is not expected to confirm that entry, otherwise it's very easy to get
> the wrong result by touching a touchpad or having the cursor in the area
> where the popup appears.
YES, agreed 100%, see bug 1192739.

> ... selecting an entry with the keyboard and
> confirming it should always confirm that entry, and we should figure out why
> now it behaves wrongly in TB and not in FF.
YES, that would be excellent, indeed. See comment #10.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Marco Bonardo [::mak] from comment #9)
> I suspect your form has completeDefaultIndex but not completeSelectedIndex?
Sadly I know close to nothing about autocomplete, Magnus is our resident expert. (I'm just the poor guy who chased bug 1042561 with you.)
I looked for completeDefaultIndex and completeSelectedIndex and couldn't find either. Where would they need to go? Here?
https://dxr.mozilla.org/comm-central/rev/39761f94a020fa794f720670e873bb96c6df6b45/mail/components/compose/content/messengercompose.xul#955

Comment 14

[Marco Bonardo [:mak]]

yes:

completedefaultindex="true" forcecomplete="true"
minresultsforpopup="2" ignoreblurwhilesearching="true"

thanks

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: autocomplete.patch

This patch restores the previous behaviour, <tab> and <enter> confirm the hovered or cursor selected choice. Using the cursor keys does *not* populate the base field from the popup value.

Comment 16

[Jorg K (CEST = GMT+2)]

Wires crossed again, sorry, I'll try what you said in comment #14.

Comment 17

[Jorg K (CEST = GMT+2)]

Oops, silly me, we already had:
completedefaultindex="true" forcecomplete="true"
minresultsforpopup="2" ignoreblurwhilesearching="true"
Sorry, something is wrong here. My local build behaves differently to a Daily build (I think I had it refreshed recently. I'll have to look into it).

Comment 18

[Jorg K (CEST = GMT+2)]

OK, I got to the bottom of the confusion.

While the base field is not fully populated, that is, it still has the >> in it, a hover + <enter> *will still* select the hovered popup value or the one selected via the error keys.

If the base field has already been "fixed", no >> in it, then enter will no longer confirm the hovered popup value or the one selected via the arrow keys. That's changed. I guess FF doesn't do the >> business.

I'll try with completeSelectedIndex="true" instead of completedefaultindex="true".

Comment 19

[Jorg K (CEST = GMT+2)]

OK, I'm trying with completeselectedindex="true" *only*.

The effect is that using the cursor keys, the base field is now updated from the popup value. That's how we like it. But shock horror, enter still confirms another value, the first one in the popup list, which is neither the base value nor the hovered one. Even clicking in the popup confirms the first one in the list.

So that's absolutely not working. BTW, with this setting, there is none of this >> business.

I think Magnus needs to look into it.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: autocomplete.patch

This doesn't work at all, see previous comment.

Comment 21

[Jorg K (CEST = GMT+2)]

Just repeating (sorry): We'd love to use completeselectedindex="true" to populate the base field from the popup when the arrow keys are used, but enter and click of course need to work.

BTW, that >> business, that's an M-C feature, right?

Comment 22

[:aceman]

I don't know if we still need that >> feature. We have places in code that have to take care to remove it first if found.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Video showing that the previous behaviour hasn't changed in some cases.

While the >> was visible, I hovered over Laurence and hit enter and she got selected. That's the previous behaviour.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: Video showing that completeselectedindex doesn't select the right value.

Further to comment #19, as you can see, not the base value, neither the selected value from the popup is used, but the first value in the popup.

Comment 25

[Jorg K (CEST = GMT+2)]

Firefox claims that the videos are corrupt, but they play fine in VLC when downloaded.

Comment 26

[Jorg K (CEST = GMT+2)]

Sorry about all the noise, but let me summarise the issues seen in Thunderbird:

completedefaultindex="true" (current state):
1) If no >> is showing: Cursor-key selected popup value cannot be confirmed with <enter> or <tab>.
   (original report in this bug, comment #0).
2) If >> is showing: Hovered popup value is still selected with <enter> or <tab>.
   (video: attachment 8791678 [details]). Accepted TB behaviour but apparently a bug.
   TB complaints in bug 1192739.

completeselectedindex="true" (no >> seem to occur) (desirable future state):
3a) Cursor-key selected popup value is not confirmed with enter or tab (1st in popup is used)
    (video: attachment 8791806 [details]).
3b) Clicked popup value is not used (1st in popup is used).

Marco, please let me know if you want to split off the various issues into separate bugs.

Comment 27

[Marco Bonardo [:mak]]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #26)
> Marco, please let me know if you want to split off the various issues into
> separate bugs.

Here I'm just going to take care of the regression, so will fix up case where defaultcompleteindex="true" and there's no completeselectedindex. This is what broke and what thunderbird uses.

Other issues or improvements should be moved elsewhere. comments inline:

> completedefaultindex="true" (current state):
> 1) If no >> is showing: Cursor-key selected popup value cannot be confirmed
> with <enter> or <tab>.
>    (original report in this bug, comment #0).

this is what I can fix here.

> completeselectedindex="true" (no >> seem to occur) (desirable future state):

autocomplete-in-the-middle (the ">>" results) is something that exists only in TB, removing it could allow simplifying some code. I'm all-in for that, but it will require someone to evict that functionality from nsAutoCompleteController and eventual tests. I'm available to review changes, but it will require a separate bug and a decision on your side.

> 3b) Clicked popup value is not used (1st in popup is used).

This is strange, does it happen always, only for autocomplete-in-the-middle? May need a new bug, since it's quite unexpected.

Comment 28

[Jorg K (CEST = GMT+2)]

Filed bug 1303303 for problem 2) and bug 1303304 for problem 3).

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8791678 [details]
Video showing that the previous behaviour hasn't changed in some cases.

Attachment not relevant for this bug.

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8791806 [details]
Video showing that completeselectedindex doesn't select the right value.

Attachment not relevant for this bug.

Comment 31

[Jorg K (CEST = GMT+2)]

Reporter, please note: We have landed bug 1303304 today which changes the configuration of autocomplete used in Thunderbird. Whilst this bug here isn't fixed using the previous configuration, a side effect of the new configuration is that the problem described in comment #0 no longer occurs. Please read bug 1303304 comment #16 for more details. Try it in tomorrow's Daily.

Comment 32

[Calum Mackay]

thanks very much Jorg; yes, I've been following the other bugs and saw the checkin.

Will test when next Daily available.

Comment 33

[Calum Mackay]

looks good to me.

Just tested Daily 0919; the issue I originally reported is no longer reproducible.

Also, nice to see the fill on cursor movement.

Great work, thanks again Jörg.

Comment 34

[Calum Mackay]

hope this is the right resolution state.

Comment 35

[Jorg K (CEST = GMT+2)]

You misunderstood comment #31. This bug is now about fixing the Mozilla core toolkit autocomplete.

We made it work for Thunderbird in bug 1303304 by changing from a configuration that doesn't work and still needs to be fixed, to one that works and has other benefits.

Comment 36

[Calum Mackay]

sorry Jörg; I didn't misunderstand it, I forgot it :)

apols for the bug spam.

Comment 37

[Marco Bonardo [:mak]]

Attachment: Bug 1302472 - keyboard selected value in autocomplete popup cannot be confirmed with <enter> if completedefaultindex is true but completeselectedindex is false.

Review commit: https://reviewboard.mozilla.org/r/79714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79714/

Comment 38

[Jorg K (CEST = GMT+2)]

Sorry, I'm really confused now:

Attachment:
Bug 1302472 - keyboard selected value in autocomplete popup cannot be confirmed with <enter> if completedefaultindex is false but completeselectedindex is true.

This bug here is about completedefaultindex="true" only (see summary), which was the previous TB usage.

Bug 1303304 was about the experiment to have completeselectedindex="true" before we now have both equal true.

Comment 39

[Jorg K (CEST = GMT+2)]

I gave this patch a quick test:
Current TB config:
completedefaultindex="true" forcecomplete="true"
completeselectedindex="true"
Unaffected.

Previous TB config:
completedefaultindex="true" forcecomplete="true"
Cursor-key-selected entry can now be confirmed again with <enter>. As noted as TODO in the patch, mouse-over choice can also be confirmed with <enter>, which is subject to bug 1303303. In fact, this patch here seems to aggravate bug 1303303, since without the patch, the mouse-hovered popup item can only be confirmed when autocomplete-in-the-middle is active, now it can be confirmed regardless.

Comment 40

[Marco Bonardo [:mak]]

(In reply to Jorg K (GMT+2) from comment #39)
> completedefaultindex="true" forcecomplete="true"
> Cursor-key-selected entry can now be confirmed again with <enter>. As noted
> as TODO in the patch, mouse-over choice can also be confirmed with <enter>,
> which is subject to bug 1303303. In fact, this patch here seems to aggravate
> bug 1303303, since without the patch, the mouse-hovered popup item can only
> be confirmed when autocomplete-in-the-middle is active, now it can be
> confirmed regardless.

but this is the same that was happening before bug 1292310, right? Cause I'm just undoing the fix in that bug, I figured that the backend can't really distinguish a mouse selection from a keyboard selection if completeselectedindex is false. So we can't fix that without first changing the widget. That is something undergoing a different approach elsewhere.

Comment 41

[Jorg K (CEST = GMT+2)]

(In reply to Marco Bonardo [::mak] from comment #40)
> but this is the same that was happening before bug 1292310, right?
Sure. You reinstated the pre-bug 1292310 behaviour in TB.

> Cause I'm just undoing the fix in that bug, ...
Completely? You're not bringing the FF problem back you tried to fix in bug 1292310?

> I figured that the backend can't really
> distinguish a mouse selection from a keyboard selection if
> completeselectedindex is false. So we can't fix that without first changing
> the widget. That is something undergoing a different approach elsewhere.
Well, I'm glad we added completeselectedindex="true" to TB in bug 1303304. That has fixed a lot of problems for us (and since this is the combination FF is using hopefully avoids further problems). 

Re. comment #38: That patch really as a wrong name, doesn't it?

Comment 42

[Marco Bonardo [:mak]]

(In reply to Jorg K (GMT+2) from comment #41)
> > Cause I'm just undoing the fix in that bug, ...
> Completely? You're not bringing the FF problem back you tried to fix in bug
> 1292310?

The urlbar takes another code path due to completeselectedindex. It's a partial revert that still fixes the problem on fields having completeselectedindex but reverts to the previous on fields not having it.

> Re. comment #38: That patch really as a wrong name, doesn't it?

Ah yes, I inverted the two! will fix the patch name. thanks.

Comment 43

[Marco Bonardo [:mak]]

Comment on attachment 8792802 [details]
Bug 1302472 - keyboard selected value in autocomplete popup cannot be confirmed with <enter> if completedefaultindex is true but completeselectedindex is false.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79714/diff/1-2/

Comment 44

[Drew Willcoxon :adw]

Comment on attachment 8792802 [details]
Bug 1302472 - keyboard selected value in autocomplete popup cannot be confirmed with <enter> if completedefaultindex is true but completeselectedindex is false.

https://reviewboard.mozilla.org/r/79714/#review78884

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27435af2becb
keyboard selected value in autocomplete popup cannot be confirmed with <enter> if completedefaultindex is true but completeselectedindex is false. r=adw

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/27435af2becb

Comment 47

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 51.0a1, build ID: 20160909104831, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 50.0b5, build ID: 20161006105459 and Fx 52.0a1, build ID: 20161006030208, on Windows 10 x64, mac OS X 10.10.5 and Ubuntu 14.04 LTS.

Cheers!