1388123 - Press enter key to input suggestion be canceled for google login form

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P2)

Description

[Clemens Eisserer]

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

Steps to reproduce:

1. loaded gmail.com
2. google login form appeared
3. started typing -> list with suggestions appears
4. navigated using the arrow-keys to the entry I wished to select + pressed <ENTER>


Actual results:

the text-field stayed empty after confirming the selected suggestion via the <ENTER> key: https://youtu.be/c6URS34SWh8

Only after using the mouse, the selected entry was copied into the username textfield


Expected results:

the selected list-entry should have been transferred into the text field

Comment 1

[YF (Yang)]

Looks the page to catch the Enter key and cancel the event.

It also happens for old versions, like Firefox 36, 44,48, etc.

Comment 2

[:Gijs (he/him)]

Based on bug 286933 comment 60 this actually sounds like a regression since bug 1121040 got fixed. Matt, is that right?

Comment 6

[Matthew N. [:MattN]]

Yeah, probably an e10s regression.

Comment 7

[Clemens Eisserer]

is this annoying bug really that hard to fix?

It distrurbs the login to all google services, so I guess a really large number of users is bothered by this.

Comment 8

[Clemens Eisserer]

I wonder how much longer auto completion on the internet's #1 traffic site will have to be broken.

Comment 9

[Andreea Cupsa [:acupsa], Experiments QA]

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180326100107

I have tested your issue on the latest Nightly build (61.0a1) and latest Release build (59.0.1) on win 10 x64, Arch Linux, mac 10.11.6 and managed to reproduce it.

The bug is not reproducible on Nightly 34, and is starting to reproduce from Nightly 35.
I have used MozRegression and I've reached this pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=532b5fb77ba1&tochange=372ce1f36116

Given that these are very old version MozRegression could only give me a full day pushlog.

I was able to reproduce this issue on Nightly 39, before and after bug 1121040 was fixed, therefore it isn't a regression of bug 1121040.
I was also was able to reproduce this issue with e10s disabled. 

@gijs @Matthew N Could you please take a look at the pushlog and see if anything stands out?

Comment 10

[:Gijs (he/him)]

I don't see anything in the regression window that looks related, except bug 1052343 which was backed out immediately, so that's out...

As far as I can tell, when running Firefox 34, the devtools don't show any key event listeners on the inputbox, whereas when loading the page in later versions, they do. The debugger on current versions of Firefox also shows way more script files than on 34. So I suspect that the regression window will just point to when some script started loading in newer versions.

A workaround for users is to just use tab to select the account.

(In reply to YF (Yang) from comment #1)
> Looks the page to catch the Enter key and cancel the event.

Maybe, but it's impossible to tell what exactly gmail is doing from just looking at their script, as it's all obfuscated into oblivion.

Comment 11

[:Gijs (he/him)]

I'm really not familiar enough with the autocomplete code to understand what's going on here. On my windows machine, we hit nsAutoCompleteController::EnterMatch just fine, but when fetching the selectedIndex on the popup ( https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1205-1206 ) it returns -1 . When adding logging inside listbox.xml, it believes there are no selected items. This is odd because if using MSVS to debug, up to the point where I step over the GetSelectedIndex call, the popup remains open and an item is clearly visually selected. Not pausing there and just setting breakpoints later on doesn't seem to work, so presumably this isn't a debugging artifact but we just actually get -1 as the selectedIndex there. I don't really understand why that'd be the case, except if we somehow close the popup? I don't see what code would do that in this particular codepath, though. Maybe Marco, Matt or Mike can make more sense of what's going on here.

Comment 12

[Marco Bonardo [:mak]]

What I found so far (I didn't finish looking at it yet).
Just before the Enter keypress event reaches nsFormFillController::KeyPress something causes a call to StopControllingInput, that sets mFocusedInput to null. KeyPress bails out early if mFocusedInput is null.

Comment 13

[Marco Bonardo [:mak]]

And, it's the "blur" event, basically we blur the field before the Enter keypress event arrives

Comment 14

[Mike Conley (:mconley) (:⚙️)]

I'm reminded vaguely of bug 1311301.

Hey daleharvey, got any of this swapped into your head still?

Comment 15

[Marco Bonardo [:mak]]

Some more thoughts: the page has both keypress and keydown event listeners (no idea what they do, as Gijs said), FormFilleController has a keypress listener.
It looks like the page is causing the blur by submitting the form, and that's wrong, our keypress listener should come first and stop propagation. See
https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/toolkit/components/satchel/nsFormFillController.cpp#1172

My suspect is that the page submits on keydown. Indeed, if I change the formfill code to use keydown instead of keypress, it seems to work properly. Could this be a solution, maybe, but it's hard to tell if it may cause regressions elsewhere.

Comment 16

[Marco Bonardo [:mak]]

Attachment: hack

Comment 17

[Marco Bonardo [:mak]]

obviously using keydown breaks quite some tests https://treeherder.mozilla.org/#/jobs?repo=try&revision=79ee6bf8e939501fd5f722d7684e02fd9ab3235c
As I said, it's unclear how many of these would just be tests to update VS real problems.
Ideas are welcome, I am not sure how we could proceed from here.

Comment 18

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #17)
> obviously using keydown breaks quite some tests
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=79ee6bf8e939501fd5f722d7684e02fd9ab3235c
> As I said, it's unclear how many of these would just be tests to update VS
> real problems.
> Ideas are welcome, I am not sure how we could proceed from here.

We could probably minimize impact by continuing to use keypress for all the other key navigation stuff, but add a keydown listener that only cares about the VK_RETURN/ENTER case (and remove that from the keypress thing).

That seems like a reasonable fix to me, though as the trypush shows we'd probably still need to fix a number of automated tests, I'd expect that to be OK. As a bonus, I guess this will make a small positive difference in perceived performance. :-)

Comment 19

[Marco Bonardo [:mak]]

I think Masayuki-san said it would be better in general to use keydown, because keypress requires using the system group to observe all the non printable keys. It's just risky.
But yes, I don't see a way out of using keydown for at least this bug.

Comment 20

[Dale Harvey (:daleharvey)]

Yup sorry for the delay, it doesnt seem from reading the bug that my patch would have affected this in anyway and seems like there is a idea for how to best fix so clearing needinfo

Comment 21

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #19)
> I think Masayuki-san said it would be better in general to use keydown,
> because keypress requires using the system group to observe all the non
> printable keys. It's just risky.
> But yes, I don't see a way out of using keydown for at least this bug.

Could we implement it behind a pref? Or should we just make the jump? Do we know what other browsers with login fill do?

Comment 22

[Marco Bonardo [:mak]]

(In reply to :Gijs (he/him) from comment #21)
> Could we implement it behind a pref? Or should we just make the jump? Do we
> know what other browsers with login fill do?

The possibilities are:
1) only change Enter to act on keydown
2) only change Enter to act on keydown and also add a pref to go back
3) change everything to keydown, put it behind a pref, beta it and be ready to switch if it creates big troubles

I'd probably go for 1), because 3) is a lot of work to change the many tests, and those tests should be able to work with the pref flipped both sides. 2) is not much better than a simple backout, in this case.

I have no idea about other browsers, I'm sorry, but I seems clear Chrome uses and prevents keydown, since the suggested case works there.

Comment 23

[Clemens Eisserer]

any progress so far?
The #1 internet company's login form has broken auto completion with firefox for at least 10 months now (!!)

Comment 24

[Marco Bonardo [:mak]]

I have a patch that passes tests, https://hg.mozilla.org/try/rev/8759bd421b078eb8567736e59d9442ad54be08e9
It's not extremely pretty though, because of formautofill.
Formautofill depends on our old keypress behavior because due to e10s it has a bunch of abstraction and redirection of events (through messages), and thus events ordering is strictly necessary. Unfortunately if we'd accomodate formautofill necessities regarding events ordering, we'd not be able to properly autocomplete a page that submits its form on keydown.
This is the best solution I found so far, I'm open to suggestions (especially if they come in form of a tested patch on top of mine), I spent quite some time trying to order events as expected, but things just break in chain (fixing one behavior breaks another one).

While investigating this, I found we have a few wrong listeners in the tree using "capturing: true" instead of "capture: true". I fixed them here, but I'd be ok splitting that out if this should take longer.

Comment 25

[Marco Bonardo [:mak]]

Attachment: Bug 1388123 - Make autocomplete handle Enter on keydown.

Some pages, like the Google login form, submit information on keydown, that
causes us to not autocomplete, because we handle keypress instead.
The patch changes autocomplete to happen on keydown.
Unfortunately formautofill also uses keydown and tries to access popup data too
late, thus it needs some hacks to work properly.
In general the formautofill code has too many indirections due to e10s, and that
makes the fix more fragile than we'd want. Ideally content autocomplete should
have its own codebase, rather than sharing the same controller as chrome code.

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

Comment 26

[Matthew N. [:MattN]]

Sorry for the delay, I haven't found time to context switch to form autofill yet and I know the event handling is a mess (we've talked about changing it before). One random idea I thought of was that maybe we can move the autocomplete popup to be in the content process to avoid some of the mess added for e10s. This would be in the same vein as bug 1421229 for <select>. I'm not saying to do that instead but it's something to consider for a long-term solution.

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8988867 [details]
Bug 1388123 - Make autocomplete handle Enter on keydown.

https://reviewboard.mozilla.org/r/254020/#review262748

Looks good! Thanks

::: browser/extensions/formautofill/test/mochitest/test_clear_form.html:67
(Diff revision 1)
> +async function confirmClear(id) {
> +  info("Await for clearing input");
> +  let promise = new Promise(resolve =>
> +    document.querySelector(id).addEventListener("input", resolve, {once: true})

Nit: `id` isn't a great argument name since it's actually a selector, not an ID.

::: toolkit/components/satchel/test/mochitest.ini:22
(Diff revision 1)
>  [test_form_submission_cap2.html]
>  [test_password_autocomplete.html]
>  scheme = https
>  [test_popup_direction.html]
>  [test_popup_enter_event.html]
> +[test_submit_on_keydown_enter.html]

FYI: this directory is enabled on Linux so you may want to explicitly test there separately.

Comment 28

[Marco Bonardo [:mak]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #27)
> FYI: this directory is enabled on Linux so you may want to explicitly test
> there separately.

You mean just ensuring the test passes on Linux? I ran a Try there https://treeherder.mozilla.org/#/jobs?repo=try&revision=8759bd421b078eb8567736e59d9442ad54be08e9

If you meant something else, could be I don't understand the suggestion.

Comment 29

[Matthew N. [:MattN]]

(In reply to Marco Bonardo [::mak] from comment #28)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #27)
> > FYI: this directory is enabled on Linux so you may want to explicitly test
> > there separately.
> 
> You mean just ensuring the test passes on Linux? I ran a Try there
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8759bd421b078eb8567736e59d9442ad54be08e9
> 
> If you meant something else, could be I don't understand the suggestion.

Sorry, I made a typo, I meant to say that the directory is *not* enabled on Linux (see the skip-if in [DEFAULT]) so I was suggesting you may want to temporarily remove the skip-if, disable the other tests in the directory and make sure your new test passes by itself on Linux.

Comment 30

[Marco Bonardo [:mak]]

Attachment: Bug 1388123 - re-enable some of the satchel tests on Linux.

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

Comment 31

[Marco Bonardo [:mak]]

Comment on attachment 8988867 [details]
Bug 1388123 - Make autocomplete handle Enter on keydown.

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

Comment 32

[Marco Bonardo [:mak]]

I re-enabled a few tests and it looks good enough to me https://treeherder.mozilla.org/#/jobs?repo=try&revision=10f73417b89f06e9f2438b0811d3952d6e971879&group_state=expanded

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8991229 [details]
Bug 1388123 - re-enable some of the satchel tests on Linux.

https://reviewboard.mozilla.org/r/256192/#review263042

Awesome! Thanks!

Comment 34

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 31ef0be975586ffa857fb3ff968f282c894b8912 -d f1cce2949d23: rebasing 471954:31ef0be97558 "Bug 1388123 - Make autocomplete handle Enter on keydown. r=MattN"
merging browser/components/search/content/search.xml
merging browser/extensions/formautofill/FormAutofillContent.jsm
merging browser/extensions/formautofill/content/FormAutofillFrameScript.js
warning: conflicts while merging browser/extensions/formautofill/content/FormAutofillFrameScript.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 35

[Marco Bonardo [:mak]]

Comment on attachment 8988867 [details]
Bug 1388123 - Make autocomplete handle Enter on keydown.

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

Comment 36

[Marco Bonardo [:mak]]

Comment on attachment 8991229 [details]
Bug 1388123 - re-enable some of the satchel tests on Linux.

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

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a891dd4535b
Make autocomplete handle Enter on keydown. r=MattN
https://hg.mozilla.org/integration/autoland/rev/b8bcd63c24c9
re-enable some of the satchel tests on Linux. r=MattN

Comment 38

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/8a891dd4535b
https://hg.mozilla.org/mozilla-central/rev/b8bcd63c24c9