1491989 - Autofill does not support LastPass's field classification

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Logins, Passwords and Form Fill, defect, P2)

Description

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

In bug 1485810 comment 7, Anatoly from LastPass reports that GV's Autofill support broke LastPass in Fennec 63 Beta:

Question about this issue: In the current Aurora build, we can now see the individual fields inside the browser, but they are all the same size and position as the containing ViewGroup, which is causing problems with field classification. Will there be an update to correct this?

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

This will likely require fuller support for the DOM tree that :eeejay will be working on. That is a longer term project though so unfortunately this will not be fixed in 62 or 63.

I don't think this is a regression though, because we went from not supporting LastPass to partially supporting LastPass (with caveats such as this bug). There was never a regression in functionality.

Comment 2

[Anatoly]

I agree this is a huge improvement, and that you all for your work on this!

I should have specified that the issues only manifest only when using the new autofill framework on Android 8+.

 - The pop-up generated by the autofill framework is pinned to either the top or bottom of the browser window, which is confusing for the user. In other apps, the pop-up usually appears immediately below the target field.

 - Our heuristics for O autofill take position into account, so it causes problems for identifying fields correctly. Currently that means it fills the password into both the username and password fields. Knowing that this will not be fixed until after 63, we will look into adding a special case to handle it, but it would much be better if the field size and position were reported correctly.

Both of these issues are demonstrated in the following video: https://www.dropbox.com/s/govvmf379jx8qv9/20180918_101817_edited.mp4?dl=0

While I was testing, I also noticed that none of the generated nodes have resource ids. Could these be added at least to the edit fields?

Our changes to support Firefox haven't been released yet, but I am happy to provide a private build if you need one for testing.

Comment 3

[Felipe Leme]

I think the problem is on notifyAutoFill(); from https://hg.mozilla.org/mozilla-central/rev/d0e40284db4b, I see the following call:

case AUTO_FILL_NOTIFY_VIEW_ENTERED:
   // Use a dummy rect for the View.
   manager.notifyViewEntered(view, virtualId, getDummyAutoFillRect(session, /* screen */ true, view));


notifyViewEntered(...) should return the proper coordinates for the virtual child, but it's returning a generic Rect for all children (notice that getDummyAutoFillRect() doesn't take the virtualId in account). 

I'm guessing this is work in progress, and eventually getDummyAutoFillRect() will be replaced by the real code...

Also, for performance reasons, I'd recommend keeping a temporary Rect on GeckoView that could be re-used on demand, instead of creating a new one every time (as long as all calls are handled in the same thread, you shouldn't worry about corrupting it).

Regarding the resourceId, this is an Android-specific concept that doesn't map to HTML (and even on Android, not all views declare a resourceId), Firefox should use HtmlInfo tags instead (see https://developer.android.com/reference/android/webkit/WebView.html#onProvideAutofillVirtualStructure(android.view.ViewStructure,%20int) for the WebView / browser contract, and https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwAutofillProvider.java?q=onprovideAutofillvirtualstructure&dr=CSs&l=254 for its implementation).

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Thanks for testing and for the pointers! We do plan to add the correct coordinates in the future. We have some support for `HtmlInfo` right now [1] but we should add support for `autocomplete` and `placeholder` attributes.

[1] https://searchfox.org/mozilla-central/rev/881a3c5664ede5e08ee986d76433bc5c4b5680e6/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionTextInput.java#616-621

Comment 5

[Makoto Kato [:m_kato]]

(In reply to (inactive) Jim Chen [:jchen] [:darchons] from comment #4)

Thanks for testing and for the pointers! We do plan to add the correct
coordinates in the future.

I believe that emily has already fixed this by bug 1532582

We have some support for HtmlInfo right now [1] but we should add support for autocomplete and placeholder attributes.

This bug focuses this implementation. We assign username type from Gecko's login form's JSM, but we don't set other type yet.

Comment 6

[Chris Peterson [:cpeterson]]

Makoto says part of this issue is already fixed, but we can improve autofill parameter in Q3 or Q4.

Comment 7

[Makoto Kato [:m_kato]]

Now I test some form samples with autocomplete attribute. Since we use desktop JS module now, remained issue has been fixed...

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Bug 1491989 - <input autocomplete="email"> should be autofill target.

Autofill service application uses email, username and password hint for login
form. Some sites use email value for autocomplete attribute even if
<input type="text">. So we should set autofill hint for this situation.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4889c3321ae
<input autocomplete="email"> should be autofill target. r=geckoview-reviewers,snorp

Comment 10

[Razvan Maries]

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

Comment 11

[Diana Rus]

Hi, I verified this with Google Pixel 3a(Android 9) on a build of "geckoview_example.apk" on android platform and having as autofill service "LastPass" app enabled. Checked on the following sites:

• Twitter which seems to have the input field from "Create your account" of email type and autocomplete=email but no "LastPass" autofill hint is present.
• I have also checked with the email fields from: https://greenido.github.io/Product-Site-101/form-cc-example.html section Contact Info but the last pass hint was not displayed.
  @Makoto, can you please have a look?

Comment 12

[Makoto Kato [:m_kato]]

(In reply to Diana Rus from comment #11)

Hi, I verified this with Google Pixel 3a(Android 9) on a build of "geckoview_example.apk" on android platform and having as autofill service "LastPass" app enabled. Checked on the following sites:

• Twitter which seems to have the input field from "Create your account" of email type and autocomplete=email but no "LastPass" autofill hint is present.
• I have also checked with the email fields from: https://greenido.github.io/Product-Site-101/form-cc-example.html section Contact Info but the last pass hint was not displayed.
  @Makoto, can you please have a look?

This is another issue. Current our implementation is for login form. So at least, we require password field. I will file a new issue for more support of autocomplete attribute.

Comment 13

[Makoto Kato [:m_kato]]

filed as bug 1607114 for more support of hint.