Closed
Bug 1388123
Opened 7 years ago
Closed 6 years ago
Press enter key to input suggestion be canceled for google login form
Categories
(Toolkit :: Form Manager, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | wontfix |
firefox61 | --- | wontfix |
firefox62 | --- | wontfix |
firefox63 | --- | fixed |
People
(Reporter: linuxhippy, Assigned: mak)
References
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
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
Looks the page to catch the Enter key and cancel the event.
It also happens for old versions, like Firefox 36, 44,48, etc.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Component: Untriaged → Form Manager
Ever confirmed: true
Product: Firefox → Toolkit
Summary: input suggestion broken for google login form → Press enter key to input suggestion be canceled for google login form
Comment 2•7 years ago
|
||
Based on bug 286933 comment 60 this actually sounds like a regression since bug 1121040 got fixed. Matt, is that right?
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Assignee | ||
Updated•7 years ago
|
Component: Autocomplete → Form Manager
Comment 6•7 years ago
|
||
Yeah, probably an e10s regression.
Has Regression Range: --- → no
Flags: needinfo?(MattN+bmo)
Keywords: regression,
regressionwindow-wanted
Priority: -- → P2
Comment hidden (advocacy) |
Comment hidden (advocacy) |
Comment 9•7 years ago
|
||
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?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(MattN+bmo)
Keywords: regressionwindow-wanted
OS: Unspecified → All
Hardware: Unspecified → All
Comment 10•7 years ago
|
||
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•7 years ago
|
||
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.
Flags: needinfo?(mconley)
Flags: needinfo?(mak77)
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 13•7 years ago
|
||
And, it's the "blur" event, basically we blur the field before the Enter keypress event arrives
Comment 14•7 years ago
|
||
I'm reminded vaguely of bug 1311301.
Hey daleharvey, got any of this swapped into your head still?
Flags: needinfo?(mconley) → needinfo?(dharvey)
Assignee | ||
Comment 15•7 years ago
|
||
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.
Assignee | ||
Comment 16•7 years ago
|
||
Assignee | ||
Comment 17•7 years ago
|
||
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.
Flags: needinfo?(mak77)
Comment 18•7 years ago
|
||
(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. :-)
Assignee | ||
Comment 19•7 years ago
|
||
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•7 years ago
|
||
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
Flags: needinfo?(dharvey)
Updated•7 years ago
|
Flags: needinfo?(MattN+bmo)
Comment 21•7 years ago
|
||
(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?
Flags: needinfo?(mak77)
Assignee | ||
Comment 22•7 years ago
|
||
(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.
Flags: needinfo?(mak77)
Reporter | ||
Comment 23•6 years ago
|
||
any progress so far?
The #1 internet company's login form has broken auto completion with firefox for at least 10 months now (!!)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Comment 24•6 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8963398 -
Attachment is obsolete: true
Comment 26•6 years ago
|
||
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•6 years ago
|
||
mozreview-review |
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.
Attachment #8988867 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 28•6 years ago
|
||
(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.
Flags: needinfo?(MattN+bmo)
Comment 29•6 years ago
|
||
(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.
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•6 years ago
|
||
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•6 years ago
|
||
mozreview-review |
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!
Attachment #8991229 -
Flags: review?(MattN+bmo) → review+
Comment 34•6 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 37•6 years ago
|
||
Pushed by mak77@bonardo.net:
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8a891dd4535b
https://hg.mozilla.org/mozilla-central/rev/b8bcd63c24c9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
status-firefox57:
fix-optional → ---
status-firefox61:
--- → wontfix
status-firefox62:
--- → wontfix
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•