1419724 - The autofill'ed text will be removed after pressing ESC key

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect, P2)

Description

[Luke Chang [:lchang] (inactive)]

It affects both address and credit card autofill.


[Steps:]

0. Ensure there is at least one address record saved

1. Open demo page https://luke-chang.github.io/autofill-demo/basic.html
2. Focus one of the fields that can trigger autofill dropdown
3. Click the field again to trigger the dropdown
4. Select any profile from dropdown to autofill
5. Press ESC key


[Actual Result:]

The autofill'ed text has been removed.


[Expected Result:]

Nothing should happen.

Comment 2

[Ray Lin[:ralin]]

Attachment: Bug 1419724 - Explicitly make searchString consistent with autocompleteController as form autofill goes different route to fill the value.

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

Comment 3

[Ray Lin[:ralin]]

The patch in comment 2 is a workaround, I'd like to seek for other appropriate fix to the underlying problem. If no feasible approach can be done by 58, then we go the workaround.

Comment 4

[Ray Lin[:ralin]]

To decrypt the data in the runtime which requires master password from user's input, e.g. credit card number, we have to by pass the autocomplete EnterMatch()[0][1] and fill-in the decrypted value by ourself in asynchronous way right afterwards. So the mSearchString in AutocompleteController would become inconsistent with the real filled value. 

In this case, the mSearchString remains empty even after the form is populated by form autofill, hence, pressing ESC would revert the value of focused input to empty according to empty mSearchString.



[0] https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/browser/extensions/formautofill/ProfileAutoCompleteResult.jsm#98-111
[1] https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/browser/extensions/formautofill/FormAutofillHandler.jsm#394

Comment 5

[Ray Lin[:ralin]]

Hi Marco,

Could you give us some feedback about the workaround in comment 2? It might be a bit hacky to manipulate the "searchString" directly, but I couldn't come up with better way to fix the problem unless EnterMatch() can adapt asynchronous value. Thanks.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

At first glance, you could use the onTextReverted handler on the input binding, if you implement it and make it return false when the text should not be reverted, it may solve your troubles (provided the problem is HandleEscape). I'm not sure how complex that is from content though, I didn't work much with e10s autocomplete.

Btw, there's nothing wrong with updating the searchString if it's out of sync for some reason, but I don't know your code well enough to tell if it fits your case. MattN could have more knowledge in this area.

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8931220 [details]
Bug 1419724 - Explicitly make searchString consistent with autocompleteController as form autofill goes different route to fill the value.

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

Comment 8

[Ray Lin[:ralin]]

(In reply to Marco Bonardo [::mak] from comment #6)
> At first glance, you could use the onTextReverted handler on the input
> binding, if you implement it and make it return false when the text should
> not be reverted, it may solve your troubles (provided the problem is
> HandleEscape). I'm not sure how complex that is from content though, I
> didn't work much with e10s autocomplete.
> 
> Btw, there's nothing wrong with updating the searchString if it's out of
> sync for some reason, but I don't know your code well enough to tell if it
> fits your case. MattN could have more knowledge in this area.

Thank you Marco, that's helpful! We'll try to update searchString first. If we keep the searchString synced, I guess we don't need to worry about other operations like text revert anyways.

Comment 9

[Ray Lin[:ralin]]

Hi Matt,

Could you help review is the searchString updating reasonable? Thanks

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8931220 [details]
Bug 1419724 - Explicitly make searchString consistent with autocompleteController as form autofill goes different route to fill the value.

https://reviewboard.mozilla.org/r/202328/#review208724

I don't see any problems off-hand but I think we should have tests in this bug for this somewhat obscure behaviour.

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8931220 [details]
Bug 1419724 - Explicitly make searchString consistent with autocompleteController as form autofill goes different route to fill the value.

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

Comment 12

[Ray Lin[:ralin]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #10)
> Comment on attachment 8931220 [details]
> Bug 1419724 - Explicitly make searchString consistent with
> autocompleteController as form autofill goes different route to fill the
> value.
> 
> https://reviewboard.mozilla.org/r/202328/#review208724
> 
> I don't see any problems off-hand but I think we should have tests in this
> bug for this somewhat obscure behaviour.

Thank you Matt! Test added, and TH result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e60d88c74f171c4d1866d9af7abd2073601f62f2 , let's land it.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d7297509f5f
Explicitly make searchString consistent with autocompleteController as form autofill goes different route to fill the value. r=MattN

Comment 14

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/4d7297509f5f

Comment 15

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7297509f5f

Comment 16

[Ray Lin[:ralin]]

Attachment: Beta-Bug-1419724-Explicitly-make-searchString-consistent.patch

Approval Request Comment
[Feature/Bug causing the regression]: Autocomplete keyboard accessibility
[User impact if declined]: as comment 0 mentioned, user would unexpectedly erase the filled value by pressing ESC
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, see STR in comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: only update the searchString, doesn't affect the main logic of any
[String changes made/needed]: none

Thanks!!

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8934415 [details] [diff] [review]
Beta-Bug-1419724-Explicitly-make-searchString-consistent.patch

To avoid unexpectedly erasing the filled value by pressing ESC issue. Beta58+.

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/62323bdb968a

Comment 19

[roxleitan]

Reproduced the issue using Nightly (2017-11-22) on Windows 10 x64.
Verified fixed using the latest Nightly 59.0a1 (2017-12-12)on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12

Comment 20

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce the issue mentioned in comment 0 using Firefox 59.0a1 (BuildId:20171122220056).

This issue is no longer reproducible using Firefox 58.0b11 (BuildId:20171211020921) on Windows 10 64bit, macOS 10.13 and Ubuntu 16.04 64bit.