[Ad-hoc fixes] Ad-hoc fixes autofill logic #780

dbajpeyi · 2025-03-18T10:21:59Z

Reviewer: @shakyShane @GioSensation
Asana: https://app.asana.com/0/0/1209507178584801/f

Description

The goal for this PR is to get the core logic of https://app.asana.com/0/72649045549333/1208952994066584/f out, by using the new C-S-S ConfigFeature class. Ideally I want to be able to release this, as by default this setting will be turned off at the moment (native not implemented yet), and autofill's algorithm should take care as usual!

Bump C-S-S and make sure build and tests are green, and processConfig is updated
Create a new class in autofill that extends ConfigFeature, and implements some utility for the actual feature
Initialize this class in Settings.js and make a property of the settings class
Implement actual autofill logic for the fixes
Use types from privacy-configuration repo for remote config. PR + Schema implemented here: [Autofill] Add new autofill sub-feature privacy-configuration#2917

For the autofill logic there are two parts:

FormAnalyzer -> here the logic checks if there are any hard-coded form type for a manually selected form, and returns that.
Scanner -> Autofill early checks if there are any hard coded forms that we know of, and if all forms are found it skips the actual page scanning in findEligibleInputs.

The config structure is proposed in https://app.asana.com/0/72649045549333/1209507178584796/f

Steps to test

src/DeviceInterface/tests/AndroidInterface.test.js

GioSensation · 2025-04-04T12:49:26Z

package.json

+    "@duckduckgo/content-scope-scripts": "github:duckduckgo/content-scope-scripts#8.1.0",
    "@duckduckgo/eslint-config": "github:duckduckgo/eslint-config#v0.1.0",
    "@duckduckgo/privacy-test-pages": "github:duckduckgo/privacy-test-pages#1.3.1",
+    "@duckduckgo/privacy-configuration": "github:duckduckgo/privacy-configuration#dbajpeyi/add-new-autofill-config",


I guess this will need to be changed before merging. Let's make sure we don't forget.

GioSensation

A few more notes. Hit me up on MM if you want to chat sync.

GioSensation · 2025-04-04T12:53:15Z

src/Form/FormAnalyzer.js

+        }
        if (this.isHybrid) return false;
-
        return this.autofillSignal < 0;


Just styling, but when I use the early return pattern, I prefer leaving an empty line after each statement. Especially useful here since we have a conditional with an inline return, otherwise it makes it hard to visually scan the logic. Please do this for all these early returns here. Even the one with the comment, add the whitespace before the comment.

GioSensation · 2025-04-04T13:00:22Z

src/config.js

-    let userUnprotectedDomains = null;
+    let userUnprotectedDomains = [];
    /** @type {Record<string, any> | null} */
+


I guess this space is accidental.

Suggested change

GioSensation · 2025-04-04T13:03:40Z

src/Scanner.js

+     * @returns {HTMLFormElement|null}
+     */
+    get forcedForm() {
+        return this.device.settings.siteSpecificFeature?.getForcedForm() ?? null;


I really dislike how we're always forcing to null with the nullish coaleshing operator even when the API naturally returns null, like when using querySelector. We're doing it over and over. I'm going to flag them all, bear with me. 🙂

Interestingly I am seeing this, even if the API returns null - somehow compiler complains 😄 Definitely had a reason for doing it this way.

GioSensation · 2025-04-04T13:06:56Z

src/site-specific-feature.js

+     * @returns {HTMLFormElement|null}
+     */
+    getForcedForm() {
+        return this.formBoundarySelector ? document.querySelector(this.formBoundarySelector) : null;


What state can this.formBoundarySelector be? I guess here you're concerned it's null or undefined, so you don't need this conditional. You can just return document.querySelector(this.formBoundarySelector) and that will be null anyway. https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector#return_value

Yes, but tsc is not happy about that :)

GioSensation · 2025-04-04T13:15:02Z

src/site-specific-feature.js

+     * @returns {import('@duckduckgo/privacy-configuration/schema/features/autofill.js').SiteSpecificFixes['formTypeSettings']}
+     */
+    get formTypeSettings() {
+        return this.getFeatureSetting('formTypeSettings') ?? [];


You're very fond of the ?? operator, aren't you? 🙂 TBH, unless it's really needed for specific reasons (i.e. when your value can be 0 or '') I prefer the good old ||. This seems to imply a specific need that it's not there (i.e. as if we were expecting this to be either 0 or '' which we don't).

src/site-specific-feature.js

GioSensation · 2025-04-04T13:27:45Z

src/site-specific-feature.js

+     * @returns {string|null}
+     */
+    getForcedFormType(form) {
+        return this.formTypeSettings?.find((config) => form.matches(config.selector))?.type ?? null;


Here you might make a case for null, otherwise this method returns undefined which is different from the others methods, but still I don't think we care. Let me know if you disagree.

src/Scanner.js

GioSensation · 2025-04-04T13:48:32Z

src/Scanner.js

    /** @type {import("./Form/matching").Matching} matching */
    matching;

+    /** @type {boolean} A flag to indicate the forced form has been added */


This is somewhat incorrect. The flag will be true even if we tried to find a forced form but failed. Which I think is what you wanted to do and is correct.

I would rename it to avoid confusion, though. Something like hasScannedForForcedForm. When I looked at the boolean I drafted a whole comment for lines 311-313 to simplify this, but I was wrong so I deleted it. I was misled by the name.

GioSensation · 2025-04-04T13:50:28Z

src/Form/Form.js

+        // If the form has only one input and it's unknown, discard the form
+        if (this.inputs.all.size === 1 && this.inputs.unknown.size === 1) {
+            this.destroy();
+            return;
+        }
+


Weren't we doing this somewhere already? Am I misremembering? 🤔

GioSensation · 2025-04-04T15:09:23Z

src/Form/FormAnalyzer.js

+            return forcedFormType === 'login';
+        }

+        if (this.isHybrid) return false;


@dbajpeyi You forgot this 😉.

GioSensation · 2025-04-04T15:49:45Z

src/Scanner.js

+        const forcedForm = this.hasRetrievedForcedForm ? null : this.forcedForm;
+        this.hasRetrievedForcedForm = true;
+        const parentForm = forcedForm || form || this.getParentForm(input);

-        const parentForm = form || this.getParentForm(input);
+        if (this.forcedForm && parentForm.contains(this.forcedForm) && this.forcedForm !== parentForm) return;


I'd like to review this again. In the meantime, can you please add a comment above this conditional? Not entirely sure what it's supposed to do 😬.

I think the hasRetrievedForcedForm part is just confusing me. Maybe I just need to see it without that part, but now I'm fried and can't devote it attention. Will do it on Monday. Please assign the Asana task to me for Monday.

hasRetrievedForcedForm is making sure we only get forced form for the first input. I have tried to hide that in a function now that does a bit of complex caching, but simplifies the code flow here IMO.

Removed that condition you mentioned, instead preventing child form deletion, if we have a forced form in config now. I think it's equivalent but combines well with existing logic (it's for cases like https://estore.archives.gov/roosevelt/Login.aspx, where nested forms can delete child forms).

src/Settings.js

dbajpeyi · 2025-04-08T10:13:21Z

src/Scanner.js

+        if (this._forcedForm === null) {
+            this._forcedForm = this.device.settings.siteSpecificFeature?.getForcedForm() || null;
+        }


Same comment as above, for some reason getForcedForm() is undefined too, so forcing/narrowing it to null here.

dbajpeyi · 2025-04-08T10:17:29Z

src/Scanner.js

-            if (childForm) {
+            // unless it's the forced form, in that case we want to keep it.
+            const forcedFormFromConfig = this.device.settings.siteSpecificFeature?.getForcedForm();
+            if (childForm && childForm !== forcedFormFromConfig) {


We don't want to delete the element if we have a forced boundary in the config. E.g in cases where there are nested forms, and one of the child boundaries are forced. In those cases, we want to still keep the forced boundary (added test for it).

GioSensation · 2025-04-09T07:45:18Z

src/Scanner.js


+    /**
+     * Gets the forced form only for the first call, subsequent calls return null
+     * @returns {HTMLFormElement|HTMLElement|null}


HTMLFormElement inherits from HTMLElement, so using both is not needed here. You should go stricter when possible (not here) but otherwise it's ok to just use the ancestor. I'm pushing the change as I wanted to run tsc to verify.

Suggested change

* @returns {HTMLFormElement|HTMLElement|null}

* @returns {HTMLElement|null}

Signed-off-by: Emanuele Feliziani <[email protected]>

* Move forcedForm checks in getParentForm Signed-off-by: Emanuele Feliziani <[email protected]> * Fix docs on real-world-html-tests Signed-off-by: Emanuele Feliziani <[email protected]> * Add assets Signed-off-by: Emanuele Feliziani <[email protected]> * Linting fix Signed-off-by: Emanuele Feliziani <[email protected]> --------- Signed-off-by: Emanuele Feliziani <[email protected]>

GioSensation

With the latest change merged I think we're good. Thank you for working through the feedback. And remember to point that dependency to main once ready.

We have one approval overseeing the changes while shane is out!

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch 9 times, most recently from d87bc24 to 5c751e7 Compare March 18, 2025 13:08

dbajpeyi changed the title ~~wip: try out a test config~~ [ConfigFeature] Ad-hoc fixes autofill logic Mar 18, 2025

dbajpeyi changed the title ~~[ConfigFeature] Ad-hoc fixes autofill logic~~ [Ad-hoc fixes] Ad-hoc fixes autofill logic Mar 18, 2025

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch 8 times, most recently from 29cab10 to 6ea4a46 Compare March 19, 2025 08:44

feat: implement config parsing feature

e925b63

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch from 6ea4a46 to e925b63 Compare March 19, 2025 08:56

test: skip settings and interface tests temporarily

4ab2e7f

dbajpeyi marked this pull request as ready for review March 19, 2025 15:20

test: update mocks

c7059af

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch from c5cfe92 to c7059af Compare March 19, 2025 16:57

dbajpeyi added 2 commits March 19, 2025 18:42

feat: skip for extension

980d2a9

fix: try catch

64c7e7f

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch from af0326f to 64c7e7f Compare March 19, 2025 18:24

test: don't need the additional feature for now

c9e6f33

dbajpeyi commented Mar 20, 2025

View reviewed changes

src/DeviceInterface/tests/AndroidInterface.test.js Outdated Show resolved Hide resolved

GioSensation reviewed Apr 4, 2025

View reviewed changes

GioSensation requested changes Apr 4, 2025

View reviewed changes

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch 4 times, most recently from ef70ecf to 20678cc Compare April 4, 2025 14:55

GioSensation reviewed Apr 4, 2025

View reviewed changes

src/Form/FormAnalyzer.js

return forcedFormType === 'login';

}

if (this.isHybrid) return false;

Copy link

Member

GioSensation Apr 4, 2025

Choose a reason for hiding this comment

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

@dbajpeyi You forgot this 😉.

chore: pr comments

bc306bf

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch from 20678cc to bc306bf Compare April 4, 2025 15:15

GioSensation reviewed Apr 4, 2025

View reviewed changes

jonathanKingston reviewed Apr 5, 2025

View reviewed changes

src/Settings.js Outdated Show resolved Hide resolved

dbajpeyi commented Apr 8, 2025

View reviewed changes

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch from 60fee7e to 948f628 Compare April 8, 2025 10:13

dbajpeyi commented Apr 8, 2025

View reviewed changes

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch from 948f628 to 2fe6954 Compare April 8, 2025 10:21

fix: don't delete child forms when there are forced forms in config

6626710

dbajpeyi force-pushed the dbajpeyi/chore-bumo-c-s-s-3 branch from 2fe6954 to 6626710 Compare April 8, 2025 10:57

GioSensation reviewed Apr 9, 2025

View reviewed changes

GioSensation and others added 3 commits April 9, 2025 09:45

Use ancestor type

62765e9

Signed-off-by: Emanuele Feliziani <[email protected]>

Merge branch 'main' into dbajpeyi/chore-bumo-c-s-s-3

4c4e9f0

GioSensation approved these changes Apr 9, 2025

View reviewed changes

dbajpeyi added 2 commits April 9, 2025 16:51

fix: remove javascriptConfig from the properties

62e6e73

chore: update privacy-configuration

dccf33d

dbajpeyi merged commit 0ab1b04 into main Apr 10, 2025
1 check passed

dbajpeyi deleted the dbajpeyi/chore-bumo-c-s-s-3 branch April 10, 2025 07:00

This was referenced Apr 10, 2025

Update autofill to 17.1.0 duckduckgo/apple-browsers#478

Merged

Update autofill to 17.1.0 duckduckgo/Android#5892

Closed

Update autofill to 17.1.0 duckduckgo/duckduckgo-privacy-extension#3027

Closed

	* @returns {HTMLFormElement\|HTMLElement\|null}
	* @returns {HTMLElement\|null}

[Ad-hoc fixes] Ad-hoc fixes autofill logic #780

[Ad-hoc fixes] Ad-hoc fixes autofill logic #780

Uh oh!

Conversation

dbajpeyi commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to test

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GioSensation left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbajpeyi Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbajpeyi Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbajpeyi Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GioSensation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dbajpeyi commented Mar 18, 2025 •

edited

Loading

dbajpeyi Apr 4, 2025 •

edited

Loading

dbajpeyi Apr 8, 2025 •

edited

Loading

dbajpeyi Apr 8, 2025 •

edited

Loading