[Form] Trigger partialSave on username/email only form submit #702

dbajpeyi · 2024-11-12T15:16:45Z

Reviewer: @GioSensation
Asana: https://app.asana.com/0/1208273769335188/1208717384536904/f

Description

We are introducing new logic that allows native apps to combine partially saved username/email in a single field form to a password at a later step in a multi-step form. Example scenarios are reset password, or multi-step login forms.

The PR updates the heuristics of when the StoreFormData call is sent to the native apps:

When there's only 1 username on a page,
When there's either an email or phone in the form.

In these cases the StoreFormData call goes out with a new trigger called partialSave.

Steps to test

Go to https://deviantart.com/, enter a username,
Go to the next step, enter the password.
Notice that the update prompt is shown (on Mac), and both -> username and password are stored.

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1208717384536904

src/Form/Form.js

GioSensation

I'm still reviewing the part at the bottom of Form.js, but in the meantime please take a look at my comments so far.

src/DeviceInterface/InterfacePrototype.js

src/Form/Form.js

src/Form/formatters.js

src/Form/formatters.test.js

GioSensation

Needs another pass. We're almost there.

GioSensation · 2024-12-05T12:12:40Z

src/Form/Form.js

+        // So that in multi step forms (like reset-password), we can identify which username was picked, or complete a password save.
+        if (hasOnlyOneCredentialOrEmail) {
+            this.shouldPromptToStoreData = true;
+            this.shouldAutoSubmit = this.device.globalConfig.isMobileApp;


We don't want to autosubmit here, do we? Imagine people just generating a private duck address on a newsletter form. We don't want to automatically submit that. Am I missing anything?

GioSensation · 2024-12-05T12:24:13Z

src/Form/Form.js

+        const hasNoCredentialsData = !formValues.credentials?.username && !formValues.credentials?.password;
+        const hasOnlyEmail =
+            formValues.identities && Object.keys(formValues.identities ?? {}).length === 1 && formValues.identities?.emailAddress;
+
+        const hasOnlyOneCredentialOrEmail =
+            Boolean(formValues.credentials?.username) !== Boolean(formValues.credentials?.password) ||
+            (hasOnlyEmail && hasNoCredentialsData);


The complexity of this stuff blows my mind 🙃. Can you please take another look and make it simpler? Like, maybe you can first check whether the form has either an email or username, and then check that there is nothing else? Or even better return early if there is more than 1 item, then check if it's a username or email.

Also maybe move it to a utility function? Not sure how complex it is after you simplify it. Feel free to ping me on MM with a preview.

GioSensation · 2024-12-05T12:26:09Z

src/Form/formatters.js

+        Boolean(identities.emailAddress) ||
+        Boolean(identities.phone)


Will this start prompting to save identities that just have a lone phone or lone email? Do we have some backend checks in the native apps to prevent that?

Ah, we have a guard against it in the interface prototype. Got it.

But that makes this whole thing confusing because we're saying that we should store identities when in fact we shouldn't.

What I think we should do instead is: when we detect one of these email-only or phone-only forms, move that email or phone to credentials.username, then let the rest of the flow continue as usual. Because that's actually what we want, logically. It's not an identity, it's a username. Makes sense? I think this should also simplify things on the native side, because they can only expect these updated calls to have credentials.username rather than multiple potential types.

GioSensation · 2024-12-05T12:28:56Z

src/DeviceInterface/InterfacePrototype.js

+
+            // If credentials has only username field, and no password field, then trigger is a partialSave
+            const isUsernameOnly =
+                Boolean(formData.credentials?.username) && !formData.credentials?.password && form.inputs.credentials.size === 1;


Shouldn't you just check for username and size here? Also, don't need to force to Boolean.

Suggested change

Boolean(formData.credentials?.username) && !formData.credentials?.password && form.inputs.credentials.size === 1;

formData.credentials?.username && form.inputs.credentials.size === 1;

GioSensation · 2024-12-05T12:36:08Z

src/DeviceInterface/InterfacePrototype.js

+            // Is an email or phone number present in the form, but no other credentials
+            const isEmailOrPhoneOnly =
+                Boolean(formData.identities?.emailAddress) !== Boolean(formData.identities?.phone) &&
+                form.inputs.credentials.size === 0 &&
+                form.inputs.identities.size === 1;


See below, we may be able to get rid of these identities checks.

GioSensation

Just minor tweaks, but approved. Now the code is much simpler than the original implementation. Thanks!

GioSensation · 2024-12-06T12:54:56Z

src/DeviceInterface/InterfacePrototype.js


+        const isUsernameOnly = Object.keys(values?.credentials ?? {}).length === 1 && values?.credentials?.username;


Somewhat minor, but I'd prefer using || here. I could see that being an empty string at some point for whatever reason. Plus, it's easier to read for most people, which is always a plus 😉.

Suggested change

const isUsernameOnly = Object.keys(values?.credentials ?? {}).length === 1 && values?.credentials?.username;

const isUsernameOnly = Object.keys(values?.credentials || {}).length === 1 && values?.credentials?.username;

GioSensation · 2024-12-06T13:02:01Z

src/autofill-utils.js

@@ -624,6 +624,15 @@ function queryElementsWithShadow(element, selector, forceScanShadowTree = false)
    return [...elements];
 }

+/**
+ * Checks if there's only one identity in the object, and it's a username-like identity


Suggested change

* Checks if there's only one identity in the object, and it's a username-like identity

* Checks if there is a single username-like identity, i.e. email or phone

GioSensation · 2024-12-06T13:02:24Z

src/Form/formatters.js

+    /** Fixes for credentials */
+    if (!credentials.username && hasUsernameLikeIdentity(identities)) {
+        // @ts-ignore - We know that username is not a useful value here
+        credentials.username = identities.emailAddress ?? identities.phone;


Same as above, I'd use || here.

@muodov

Task/Issue URL: https://app.asana.com/0/1208923931185505/1208923931185505 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0 ## Description Updates Autofill to version [16.0.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0). ### Autofill 16.0.0 release notes ## What's Changed This PR introduces breaking changes for Android/Windows causing save autofill prompt to get triggered in username only fields. * Upgrade to shared eslint config + Adopt Prettier by @muodov in duckduckgo/duckduckgo-autofill#695 * Ignore lint PR in git blame by @muodov in duckduckgo/duckduckgo-autofill#699 * Update password-related json files (2024-11-18) by @daxmobile in duckduckgo/duckduckgo-autofill#706 * [Form] Always scan shadow elements when categorizing the form inputs by @dbajpeyi in duckduckgo/duckduckgo-autofill#703 * Bump ts-to-zod from 3.1.3 to 3.14.0 by @dependabot in duckduckgo/duckduckgo-autofill#710 * [FormAnalyzer] Fix paperlesspost.com login form by @dbajpeyi in duckduckgo/duckduckgo-autofill#711 * [FormAnalyzer] Tweak regex to match forgot password attribute by @dbajpeyi in duckduckgo/duckduckgo-autofill#712 * [Form] Trigger partialSave on username/email only form submit by @dbajpeyi in duckduckgo/duckduckgo-autofill#702 **Full Changelog**: duckduckgo/duckduckgo-autofill@15.1.0...16.0.0 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). Co-authored-by: dbajpeyi <[email protected]>

@muodov

Task/Issue URL: https://app.asana.com/0/1208923931185505/1208923931185505 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0 BSK PR: duckduckgo/BrowserServicesKit#1122 ## Description Updates Autofill to version [16.0.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0). ### Autofill 16.0.0 release notes ## What's Changed This PR introduces breaking changes for Android/Windows causing save autofill prompt to get triggered in username only fields. * Upgrade to shared eslint config + Adopt Prettier by @muodov in duckduckgo/duckduckgo-autofill#695 * Ignore lint PR in git blame by @muodov in duckduckgo/duckduckgo-autofill#699 * Update password-related json files (2024-11-18) by @daxmobile in duckduckgo/duckduckgo-autofill#706 * [Form] Always scan shadow elements when categorizing the form inputs by @dbajpeyi in duckduckgo/duckduckgo-autofill#703 * Bump ts-to-zod from 3.1.3 to 3.14.0 by @dependabot in duckduckgo/duckduckgo-autofill#710 * [FormAnalyzer] Fix paperlesspost.com login form by @dbajpeyi in duckduckgo/duckduckgo-autofill#711 * [FormAnalyzer] Tweak regex to match forgot password attribute by @dbajpeyi in duckduckgo/duckduckgo-autofill#712 * [Form] Trigger partialSave on username/email only form submit by @dbajpeyi in duckduckgo/duckduckgo-autofill#702 **Full Changelog**: duckduckgo/duckduckgo-autofill@15.1.0...16.0.0 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). --------- Co-authored-by: dbajpeyi <[email protected]> Co-authored-by: Graeme Arthur <[email protected]>

@muodov

Task/Issue URL: https://app.asana.com/0/1208923931185505/1208923931185505 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0 BSK PR: duckduckgo/BrowserServicesKit#1122 ## Description Updates Autofill to version [16.0.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0). ### Autofill 16.0.0 release notes ## What's Changed This PR introduces breaking changes for Android/Windows causing save autofill prompt to get triggered in username only fields. * Upgrade to shared eslint config + Adopt Prettier by @muodov in duckduckgo/duckduckgo-autofill#695 * Ignore lint PR in git blame by @muodov in duckduckgo/duckduckgo-autofill#699 * Update password-related json files (2024-11-18) by @daxmobile in duckduckgo/duckduckgo-autofill#706 * [Form] Always scan shadow elements when categorizing the form inputs by @dbajpeyi in duckduckgo/duckduckgo-autofill#703 * Bump ts-to-zod from 3.1.3 to 3.14.0 by @dependabot in duckduckgo/duckduckgo-autofill#710 * [FormAnalyzer] Fix paperlesspost.com login form by @dbajpeyi in duckduckgo/duckduckgo-autofill#711 * [FormAnalyzer] Tweak regex to match forgot password attribute by @dbajpeyi in duckduckgo/duckduckgo-autofill#712 * [Form] Trigger partialSave on username/email only form submit by @dbajpeyi in duckduckgo/duckduckgo-autofill#702 **Full Changelog**: duckduckgo/duckduckgo-autofill@15.1.0...16.0.0 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). --------- Co-authored-by: dbajpeyi <[email protected]> Co-authored-by: Graeme Arthur <[email protected]>

feat: add API call

28dc087

dbajpeyi changed the title ~~feat: add API call~~ WIP: feat: temporary form save Nov 12, 2024

dbajpeyi changed the title ~~WIP: feat: temporary form save~~ WIP: temporary form save Nov 12, 2024

feat: allow partial save in form submission

dbd1b66

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch 3 times, most recently from 3553dee to f7b951b Compare November 13, 2024 16:40

fix: don't need additional API

65bead1

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch from f7b951b to 65bead1 Compare November 18, 2024 09:52

dbajpeyi added 2 commits November 19, 2024 13:03

feat: update heuristics for save and update tests

664a527

Merge branch 'main' into dbajpeyi/feature/partial-form-save

e1030b4

dbajpeyi changed the title ~~WIP: temporary form save~~ [Form] Trigger partialSave on username/email only form submis Nov 21, 2024

dbajpeyi marked this pull request as ready for review November 21, 2024 12:15

dbajpeyi marked this pull request as draft November 21, 2024 12:15

dbajpeyi changed the title ~~[Form] Trigger partialSave on username/email only form submis~~ [Form] Trigger partialSave on username/email only form submit Nov 22, 2024

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch 2 times, most recently from 975591e to ccf3aa7 Compare November 22, 2024 16:26

dbajpeyi marked this pull request as ready for review November 25, 2024 10:36

dbajpeyi requested a review from GioSensation November 25, 2024 10:36

feat: prompt when autofilled

d8c7d83

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch from fe030fb to d8c7d83 Compare November 25, 2024 11:28

dbajpeyi commented Nov 25, 2024

View reviewed changes

src/Form/Form.js Outdated Show resolved Hide resolved

Merge branch 'main' into dbajpeyi/feature/partial-form-save

7165166

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch 2 times, most recently from 684868b to 6ded1b8 Compare December 3, 2024 14:47

chore: rebuild assets

40ccc96

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch from 6ded1b8 to 40ccc96 Compare December 3, 2024 14:51

dbajpeyi added 2 commits December 3, 2024 15:54

Merge branch 'main' into dbajpeyi/feature/partial-form-save

60100bb

fix: narrow down identities for partial save

01dd794

GioSensation requested changes Dec 4, 2024

View reviewed changes

dbajpeyi added 2 commits December 4, 2024 16:04

chore: address PR comments

225ca95

fix: make credentials storing more liberal

15e9ed9

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch from 72c0cd8 to 15e9ed9 Compare December 4, 2024 16:57

dbajpeyi added 2 commits December 5, 2024 10:12

Merge branch 'main' into dbajpeyi/feature/partial-form-save

35e9191

feat: preserve autosubmit

1098e1b

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch from 15ea746 to 1098e1b Compare December 5, 2024 11:21

GioSensation requested changes Dec 5, 2024

View reviewed changes

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch 7 times, most recently from 20347fc to f180c95 Compare December 6, 2024 12:43

refactor: simply value storage

c6b596f

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch from f180c95 to c6b596f Compare December 6, 2024 12:45

GioSensation approved these changes Dec 6, 2024

View reviewed changes

style: minor PR comments

8f1e476

dbajpeyi force-pushed the dbajpeyi/feature/partial-form-save branch from 61a1e68 to 8f1e476 Compare December 6, 2024 13:09

Merge branch 'main' into dbajpeyi/feature/partial-form-save

8c41aa1

dbajpeyi merged commit 88982a3 into main Dec 6, 2024
1 check passed

dbajpeyi deleted the dbajpeyi/feature/partial-form-save branch December 6, 2024 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Trigger partialSave on username/email only form submit #702

[Form] Trigger partialSave on username/email only form submit #702

dbajpeyi commented Nov 12, 2024 •

edited

Loading

GioSensation left a comment

GioSensation left a comment

GioSensation Dec 5, 2024

GioSensation Dec 5, 2024

GioSensation Dec 5, 2024

GioSensation Dec 5, 2024 •

edited

Loading

GioSensation Dec 5, 2024

GioSensation Dec 5, 2024

GioSensation left a comment

GioSensation Dec 6, 2024

GioSensation Dec 6, 2024

GioSensation Dec 6, 2024

		Boolean(identities.emailAddress) \|\|
		Boolean(identities.phone)

	Boolean(formData.credentials?.username) && !formData.credentials?.password && form.inputs.credentials.size === 1;
	formData.credentials?.username && form.inputs.credentials.size === 1;


		const isUsernameOnly = Object.keys(values?.credentials ?? {}).length === 1 && values?.credentials?.username;

	* Checks if there's only one identity in the object, and it's a username-like identity
	* Checks if there is a single username-like identity, i.e. email or phone

[Form] Trigger partialSave on username/email only form submit #702

[Form] Trigger partialSave on username/email only form submit #702

Conversation

dbajpeyi commented Nov 12, 2024 • edited Loading

Description

Steps to test

GioSensation left a comment

Choose a reason for hiding this comment

GioSensation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GioSensation Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GioSensation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbajpeyi commented Nov 12, 2024 •

edited

Loading

GioSensation Dec 5, 2024 •

edited

Loading