-
Notifications
You must be signed in to change notification settings - Fork 97
Conversation
a80cdca
to
8509f54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving as comment-only for now.
Overall it looks good. I have some concerns in the details, but it's shaping up nicely!
val usernamePresentation = RemoteViews(packageName, android.R.layout.simple_list_item_1) | ||
val passwordPresentation = RemoteViews(packageName, android.R.layout.simple_list_item_1) | ||
usernamePresentation.setTextViewText(android.R.id.text1, credential.username) | ||
passwordPresentation.setTextViewText(android.R.id.text1, "Password for ${credential.username}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any localization concerns with that string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good call-out. I actually wasn't sure what to put here; we could show the password in plaintext but that doesn't seem ideal. The designs don't cover the password field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, currently when the user is focusing the password field, I'm sending back an autofill selection box that says Password for x
where x
is the username. What do y'all think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sashei that makes sense to me 👍
Through my testing, I haven't encountered any apps where tapping on an autofill suggestion for the username field fills both username and password fields. Is that how this implementation will work, where the user will have to select the autofill suggestion for both the username and password fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickbrandt in my testing, yes, we end up filling both fields when the user selects an account in one of them (can highlight username or password fields). You're welcome to grab the app from the link above and see if it works as you expect.
8509f54
to
cbea149
Compare
Autofill detection thoughts: if we have an |
ba6b6e4
to
1e2c7c3
Compare
@sashei here is some of my testing results: (using build #2143, debug) Apps tested: Perhaps I'm doing something wrong here. I'm confused why some of these are working for you/others, but not for me :/ I'm not seeing any errors either, except occasionally seeing "unexpected package name format" when on the entry list or an entry detail |
I've loaded up the debug build of #2143 a few times on my Pixel 2 and have had issues just signing in. I was finally able to log in, but am experiencing some of the same issues as Nick with suggestions not showing up. I made sure to set up Lockbox as the AutoFill provider in settings once Lockbox was installed. Apps tried: |
@nickbrandt @changecourse cool, thanks for testing y'all, I will look into those with some other device / version emulators! |
FWIW I tried some apps with two-step sign in processes, and the autofill service frequently only gets triggered on the first page (e.g. Slack app), so that might be a system limitation :-/ |
3ee92b8
to
6703db1
Compare
6703db1
to
a7f2b81
Compare
a7f2b81
to
a38fcd3
Compare
@changecourse @nickbrandt thanks again for testing this yesterday; I cleverly introduced a regression and did no further testing right before making this build so... yeah! Hopefully that is cleared up for y'all today; I've made a point to re-test against all the apps listed in the issue description! |
@sashei it's working much better now, thanks! Below are my testing results: Device: Pixel 1 running Android 9 Apps Tested Errors: |
f96b015
to
6e89ca7
Compare
6e89ca7
to
eea524e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really awesome. Some very quick testing this is working pretty well, though i suspect we're entering an arms race w/r/t heuristics.
"Requesting Changes" for the discussion of a couple of points in-line; will happily change to "approved" once discussed.
View.AUTOFILL_HINT_EMAIL_ADDRESS, | ||
"email", | ||
"username", | ||
"user name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆
A note on testing: The |
Fixes #215
Testing and Review Notes
This will fill credentials, regardless of current
DataStore
lock status, into the appropriate views in an app as requested. This ticket currently does not differentiate between the app and a contained WebView, so browsers and apps using a WebView-based login (such as ours :p) will not receive the correct credentials to limit scope on this PR. I've filed a follow-up #371 specifically to handle the webview / geckoview view parsing.There is also a follow-up issue #375 to address the limitations of the naive package name parsing (matching the app name with a credential website) that's being used here. You may see an error "unexpected package name format"
Apps I've tested with so far:
Helping test the AutofillService
I've only tested this against the top ~10 apps on the app store... if we can cast a wider net, that would be awesome! I've tried to add some useful error pop-ups from the AutofillService, and would really appreciate folks installing the build from this branch and trying to log in to several apps. Feel free to add any apps you test as comments (with the pop-up error if you catch it) or in the section above.
As we are only testing login screen parsing with this ticket, I have amended the package name parsing to only return "twitter". You will see your twitter credentials filled on every screen; I'm particularly interested in apps in which no login fields (username or password) can be found, or in which only one or the other is filled.I've pulled out the twitter login, but will be merging this with the rest of the popup errors. The domain one is particularly likely to pop up in this stage, and issues with that message can be moved to #375. If you'd like to test the view parsing functionality only and fill twitter credentials to everything, the app link below has that functionality.✨✨✨✨✨✨APP LINK✨✨✨✨✨✨
To Do