[Form Autofill] Keydown handler for footer affects form history popup when using keyboard navigation

VERIFIED FIXED in Firefox 57

Status

()

defect
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: ralin, Assigned: ralin)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 attachment)

Assignee

Description

2 years ago
Form autofill keydown handler fails to distinguish between form autofill popup and form history popup, and unexpectedly open new tab for form history result.

STR:
1. add a form autofill profile
2. visit https://luke-chang.github.io/autofill-demo/layout.html
3. add two form history entries for username field
4. open form autofill popup, and close
5. focus on username field and press arrow-down key to open form history popup
6. user keyboard navigate to 2nd item
7. press enter


Actual result:
username field is auto-filled, but a new tab#preferences was opened

Expected results
no new tab opened

Comment 2

2 years ago
mozreview-review
Comment on attachment 8924092 [details]
Bug 1413473 - Verify current focused input to determine whether the opening popup is for form autofill in order not to accidentally open new tab in wrong result types.

https://reviewboard.mozilla.org/r/195306/#review200804

::: browser/extensions/formautofill/FormAutofillContent.jsm:576
(Diff revision 1)
>    _onKeyDown(e) {
>      let lastAutoCompleteResult = ProfileAutocomplete.getProfileAutoCompleteResult();
>      let focusedInput = formFillController.focusedInput;
>  
> -    if (e.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN || !lastAutoCompleteResult || !focusedInput) {
> +    if (e.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN || !lastAutoCompleteResult ||
> +        !focusedInput || focusedInput != ProfileAutocomplete._lastAutoCompleteFocusedInput) {

`_lastAutoCompleteFocusedInput` is supposed to be a private variable. If it needs to be accessed outside, you can either remove the leading underscore or add a getter function like what `getProfileAutoCompleteResult` does.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
Sorry for messing up the commit message, (thank to smart vim auto line break :/)

I've updated the patch, could you help me to review it again? Thanks.
Comment hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8924092 [details]
Bug 1413473 - Verify current focused input to determine whether the opening popup is for form autofill in order not to accidentally open new tab in wrong result types.

https://reviewboard.mozilla.org/r/195306/#review200908

Looks good. Thanks.

::: browser/extensions/formautofill/FormAutofillContent.jsm:546
(Diff revision 4)
>        ProfileAutocomplete._previewSelectedProfile(selectedIndex);
>      }
>    },
>  
> +  onPopupClosed() {
> +    ProfileAutocomplete._clearProfilePreview();

nit: `_clearProfilePreview` shouldn't start with underscore as it's a public method. 

However, since there are also other methods of `ProfileAutocomplete` not following the convention, we can fix it in a separate bug.
Attachment #8924092 - Flags: review?(lchang) → review+
Assignee

Comment 8

2 years ago
Thanks for reviewing.

> However, since there are also other methods of `ProfileAutocomplete` not following the convention, we can fix it in a separate > bug.

I'll file a separate bug with clearer scope and fix those at once.
Keywords: checkin-needed
Assignee

Updated

2 years ago
Blocks: 1413972

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d89d6c218644
Verify current focused input to determine whether the opening popup is for form autofill in order not to accidentally open new tab in wrong result types. r=lchang
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d89d6c218644
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee

Comment 11

2 years ago
Comment on attachment 8924092 [details]
Bug 1413473 - Verify current focused input to determine whether the opening popup is for form autofill in order not to accidentally open new tab in wrong result types.

Approval Request Comment
[Feature/Bug causing the regression]: Form autofill footer UI
[User impact if declined]: Using keyboard to operate form history(autocomplete) might unexpectedly trigger form autofill handler that opens and redirects to tab#preferences. It's 100% reproducible with STR in comment 0.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: only verified by myself
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see comment 0 to reproduce.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: low, one line for the focused input check.
[String changes made/needed]: none

Thanks.
Attachment #8924092 - Flags: approval-mozilla-release?
Comment on attachment 8924092 [details]
Bug 1413473 - Verify current focused input to determine whether the opening popup is for form autofill in order not to accidentally open new tab in wrong result types.

MattN helped weigh in on this one, the risk of taking this is low 1) low code risk and 2) this may only be going out to a limited set of release users who use form autofill
Attachment #8924092 - Flags: approval-mozilla-release? → approval-mozilla-release+
I've managed to reproduce this bug using an affected Nightly build from 2017-11-01.

This is verified fixed on 57.0 RC (20171106194249) and latest Nightly 58.0a1 (2017-11-07) on the following OSes: Win 10 x64, Ubuntu 16.04 x64 and macOS 10.13.
Status: RESOLVED → VERIFIED
Assignee

Comment 15

2 years ago
Thanks for your help :D
See Also: → 1417316
You need to log in before you can comment on or make changes to this bug.