Chris Peterson [:cpeterson] Reporter
	Description • 6 years ago

[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?

	(inactive) Jim Chen [:jchen] [:darchons]
	Comment 1 • 6 years ago

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.

	Anatoly
	Comment 2 • 6 years ago

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.

	Felipe Leme
	Comment 3 • 6 years ago

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).

	(inactive) Jim Chen [:jchen] [:darchons]
	Comment 4 • 6 years ago

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

	Pulsebot
	Comment 9 • 5 years ago

Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/b4889c3321ae
<input autocomplete="email"> should be autofill target. r=geckoview-reviewers,snorp