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

Status: VERIFIED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Ray Lin[:ralin]]

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 1

[Ray Lin[:ralin]]

Attachment: 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.

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

Comment 2

[Luke Chang [:lchang] (inactive)]

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 3

[Ray Lin[:ralin]]

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.

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

Comment 4

[Ray Lin[:ralin]]

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.

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

Comment 5

[Ray Lin[:ralin]]

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 6

[Ray Lin[:ralin]]

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.

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

Comment 7

[Luke Chang [:lchang] (inactive)]

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.

Comment 8

[Ray Lin[:ralin]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d89d6c218644

Comment 11

[Ray Lin[:ralin]]

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.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/488bb645c730 (FIREFOX_57b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/e433ea823e68

Comment 14

[Ciprian Georgiu, Desktop QA]

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.

Comment 15

[Ray Lin[:ralin]]

Thanks for your help :D