Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Form] expand username re-categorization to phone #691

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

dbajpeyi
Copy link
Collaborator

@dbajpeyi dbajpeyi commented Oct 24, 2024

Reviewer: @GioSensation
Asana: https://app.asana.com/0/1205996472158114/1208518339461119/f

Description

We have seen from a bunch of autofill bug reports, where autofill doesn't trigger on forms where one of the fields is password, but the other is a phone number. We have seen that in countries like Brazil/India this is a common practice to use phone numbers as usernames.

Currently we have this approach where we categorize any unknown field to username when the other field is a password. This PR expands the categorization to also include phone numbers so that phone number fields get re-categorized in such cases to username. (when the form is a login form, and the other field is a password)

Later we might want to expand this to other types as well, but we can start with phone number and assess the impact.

Steps to test

  1. Build on MacOS (or another platform),
  2. Go to https://accounts.oneplus.com/v2/index.html
  3. Add a phone number, and a password that's valid and hit sign-in,
  4. Autofill works as expected.

@dbajpeyi dbajpeyi marked this pull request as ready for review October 24, 2024 11:03
@dbajpeyi dbajpeyi changed the title feat: expand username recategorization to phone [Form] expand username re-categorization to phone Oct 24, 2024
await forwardConsoleMessages(page)
await mocks(page, {
unknown_username_categorization: true
test.describe('when there is a single ambiguous field, and one password field', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I had already given this feedback in the previous PR related to this change. Why are we using playwright tests instead of the form test suite? Playwright is way slower and it's not usually run when we iterate on the algorithm. Can't recall if there was a specific reason to do this in playwright, but if not we should probably move this test to the unit test suite.

Copy link
Collaborator Author

@dbajpeyi dbajpeyi Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember that comment, but I don't think it was about this specific feature. I cannot find it here at least: #619.

For this specific case, I think playwright test is quite useful as it helps validate the happy path quite clearly, and adds to confidence. I can merge the asserts with the previous test though, and in that case we wouldn't need to do additional setups for the new one.

I am looking for some real world forms right now, will update the unit tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take is that this shouldn't change anything in the happy path. What we're changing is not the interaction, but the form scoring. Form scoring is tested in the unit test suite, when we change the algo that's what we look at for regressions. Once the form is scored, everything follows as usual, there's nothing specific happening here that we need to test in playwright. That is unless we think that is not the case. But I can't think of a reason why.

By the way, you can also create the test forms you need. Just drop a new form with specific requirements in the test suite. You already have one in the playwright test, for example. But of course, if you find one in the wild, all the better. I'm sure there are a few in the bug reports. Just timebox it. If you don't find anything right away, make up a form and move on. We will find failure cases in the future and we can add forms then. Just my 2 cents.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GioSensation Ready to go!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're moved this to the unit test, are these tests still needed? Or can we delete them?

Copy link
Collaborator Author

@dbajpeyi dbajpeyi Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the integration tests around it, we can stick to scoring here instead.

@dbajpeyi dbajpeyi force-pushed the dbajpeyi/expand-username-caategorization branch 2 times, most recently from c2679ab to 37403a9 Compare October 25, 2024 10:45
@dbajpeyi dbajpeyi force-pushed the dbajpeyi/expand-username-caategorization branch from 37403a9 to 4a55d98 Compare October 25, 2024 10:47
Copy link
Member

@GioSensation GioSensation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of notes, but overall this is looking good.

One thing we should probably follow up on: with this change, we loose the possibility to fill a phone for a signin form where we don't have saved credentials. The ideal UX would be:

  • Fill phone when no data
  • Include this recategorized field on form submit
  • Fill username when available

This is a harder change to get right and it only benefits macOS at the moment, which is the smallest platform for us, so I'm happy to improve every other platform at the expense of this use case. Reach out on MM if you want to chat.

</span>
<i style="display: inline-block; background-color: rgba(0, 0, 0, 0.08); width: 1px; height: 12px;"></i>
<div class="input_inner__9pf0en false">
<input autocomplete="off" class="input_input__102by1 input_has_before__bvm5rf" type="tel" placeholder="Phone" value="" data-ddg-inputtype="identities.phone">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you add the forms, you have to remove the data-ddg-inputtype and other attributes we add. It's particularly critical for the inputtype because the scoring algorithm will look at that attribute first, so you may just be adding tests that will always pass no matter what. The correct attribute here is data-manual-scoring. Also note how we don't use the full qualifier identities.phone, but only the subtype phone. I think we could make the test suite more lenient and pass with identities.phone, too. Just to save us the step of tweaking the name.

Suggested change
<input autocomplete="off" class="input_input__102by1 input_has_before__bvm5rf" type="tel" placeholder="Phone" value="" data-ddg-inputtype="identities.phone">
<input autocomplete="off" class="input_input__102by1 input_has_before__bvm5rf" type="tel" placeholder="Phone" value="" data-manual-scoring="phone">

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch totally forgot that, thanks!

<i style="display: inline-block; background-color: transparent; width: 1px; height: 16px;"></i>
<div class="input_wrapper__1x23fj undefined">
<div class="input_inner__9pf0en false">
<input autocomplete="new-password" class="input_input__102by1" type="password" placeholder="Password" data-ddg-inputtype="credentials.password.current">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Suggested change
<input autocomplete="new-password" class="input_input__102by1" type="password" placeholder="Password" data-ddg-inputtype="credentials.password.current">
<input autocomplete="new-password" class="input_input__102by1" type="password" placeholder="Password" data-manual-scoring="password.current">

</div>
<div class="controlledfeild_links__3et6gy"><span></span><a href="#" class="anchorlink_anchor__1gttsn" style="color: rgb(0, 123, 255);">Forgot password?</a></div>
<div class="controlledfeild_blank__2tj9ff"></div>
<div class="controlledfeild_buttons__16fcwv"><i style="opacity: 0;"></i><button class="button_button__2g6tsu button_middle__5fjgss bypwdandphone_button__2aqt9_" data-manual-submit="submit"><span>Sign in with password</span></button></div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content of the attribute doesn't really matter, as long as it's not false. In general, I tend to just set the attribute like this.

Suggested change
<div class="controlledfeild_buttons__16fcwv"><i style="opacity: 0;"></i><button class="button_button__2g6tsu button_middle__5fjgss bypwdandphone_button__2aqt9_" data-manual-submit="submit"><span>Sign in with password</span></button></div>
<div class="controlledfeild_buttons__16fcwv"><i style="opacity: 0;"></i><button class="button_button__2g6tsu button_middle__5fjgss bypwdandphone_button__2aqt9_" data-manual-submit><span>Sign in with password</span></button></div>

// 4. the form is a login form.
if (!hasUsername && !hasIdentitiesOrCreditCards && hasLoneUnknownInput && this.isLogin) {
const [unknownInput] = [...this.inputs.unknown]
const ambiguousInputs = [...unknownInputs, ...phoneInputs]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include credit card number? I'm happy to follow up if needed, since this will likely bring value in itself. On second thought, maybe let's leave credit card out for now. There are other implications there I wouldn't like to touch for the moment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making smaller improvements with least possibility of regression. Let's think them through after a few triages :)

@@ -463,6 +469,13 @@ class Form {
}
}

recategorizeInputToUsername (input, type) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the types 😉.

await forwardConsoleMessages(page)
await mocks(page, {
unknown_username_categorization: true
test.describe('when there is a single ambiguous field, and one password field', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're moved this to the unit test, are these tests still needed? Or can we delete them?

@dbajpeyi dbajpeyi force-pushed the dbajpeyi/expand-username-caategorization branch 3 times, most recently from 9f3c65b to f322faf Compare October 25, 2024 14:23
@dbajpeyi dbajpeyi force-pushed the dbajpeyi/expand-username-caategorization branch from f322faf to d66214c Compare October 25, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants