[Credentials] Autofill when elements are in shadow #592

dbajpeyi · 2024-07-04T09:14:15Z

Reviewer: @shakyShane
Asana: https://app.asana.com/0/1200930669568058/1207793442006227/f

Description

Currently if a form contains elements that are within shadow root, the auto scanner misses out on all the elements of the form, and fails to categorize the inputs, although it does pierce through the shadow and find the hidden input element that was clicked on. This PR expands that a bit more:

When any of the input elements are clicked, scanner tries to find the parent form (using getParentForm),
getParentForm code was modified to traverse through the shadow boundary, and find the enclosing form element,
The found parent form is then passed to findEligibleInputs, which was also modified to find other related inputs in the form that are hidden.

TLDR; This way essentially we do a two way scan - identify the clicked input which is in the shadow, traverse up the form and find the closest form and then build out the rest of the form from there.

Steps to test

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

dbajpeyi · 2024-07-16T10:03:28Z

Thanks for the review all! Made some updates:

Simplified loop: 5ec6e2a
Only scan things when necessary 1c876a6 and deb7fb7
Keeping track of shadow tree: 6f19720. Naming improved 460bd1c
Type fix for the findEnclosedElements function 6c543b6
URL evaluation improvement b321a1f

GioSensation · 2024-07-16T10:28:35Z

My apologies, the slowdown was introduced in #574, not in this PR. It's good that we've worked through these improvements, but they were not the root cause. It's my bad, I'm sorry 😿.

I haven't reviewed the updates, I'll leave that to @shakyShane or maybe I will have some time tomorrow. While I don't think we should address the slowdown in this PR, I think we should address it before cutting a new autofill release, otherwise we risk introducing performance issues to the clients.

shakyShane

@dbajpeyi just a couple of tiny things, otherwise this is looking really good now.

src/autofill-utils.js

src/Scanner.js

src/Form/Form.js

dbajpeyi · 2024-07-18T02:11:59Z

@shakyShane All fixed here 2f8488c

GioSensation

Great work 💪 . Just a few minor things.

GioSensation · 2024-07-19T07:38:45Z

src/Form/Form.js

+                // If there are not form elements, we try to look for all
+                // enclosed elements within the form.


I think this comments should be moved 1 line below.

GioSensation · 2024-07-19T07:51:26Z

src/autofill-utils.js

@@ -566,6 +566,34 @@ function getActiveElement (root = document) {
    return innerActiveElement
 }

+/**
+ * Takes a root element, creates a treewalker and finds all shadow elements that match the selector


This needs to be updated now to reflect the fact that we use querySelectorAll first.

GioSensation · 2024-07-19T07:57:12Z

src/Scanner.js

-        while (traversalLayerCount <= 5 && element.parentElement && element.parentElement !== document.documentElement) {
+        while (traversalLayerCount <= 5 && element.parentElement !== document.documentElement) {


I don't quite recall why we put this here, but I do think it had a purpose. Maybe both the parentElement and the documentElement can be null in certain weird cases. I would leave it alone unless we have reason to do otherwise.

@GioSensation I've moved the condition under instead, that's why it's removed from here.

Ah, you're right. My bad.

@dbajpeyi

Task/Issue URL: https://app.asana.com/0/1207920271697429/1207920271697429 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/12.1.0 ## Description Updates Autofill to version [12.1.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/12.1.0). ### Autofill 12.1.0 release notes ## What's Changed * [Form] Skip autofilling select input if it was changed already by @dbajpeyi in duckduckgo/duckduckgo-autofill#580 * scanner: discard Form instances with no classified inputs by @sjbarag in duckduckgo/duckduckgo-autofill#574 * [Credentials] Autofill when elements are in shadow by @dbajpeyi in duckduckgo/duckduckgo-autofill#592 * Update password-related json files (2024-07-19) by @daxmobile in duckduckgo/duckduckgo-autofill#607 * [Scanner] Don't stop scanner fully on max forms by @dbajpeyi in duckduckgo/duckduckgo-autofill#617 * tests: remove spurious logging from unit tests by @sjbarag in duckduckgo/duckduckgo-autofill#575 * Add credit card test form failure by @GioSensation in duckduckgo/duckduckgo-autofill#588 ## New Contributors * @dbajpeyi made their first contribution in duckduckgo/duckduckgo-autofill#580 **Full Changelog**: duckduckgo/duckduckgo-autofill@12.0.1...12.1.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: GioSensation <1828326+GioSensation@users.noreply.github.com>

dbajpeyi added 2 commits July 4, 2024 11:14

wip: try parsing the whole url instead of just pathname

a328f7b

wip: scan all shadow hosts

5edc95f

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch from 16b8730 to 751dde1 Compare July 11, 2024 04:14

wip: add a chase like form (shadow elements in iframe)

91c4a8f

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch from 751dde1 to 91c4a8f Compare July 11, 2024 04:15

wip: find hidden input elements

7cf1ec8

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch 6 times, most recently from eb1ad44 to d2e4907 Compare July 15, 2024 05:37

dbajpeyi changed the title ~~wip: try parsing the whole url instead of just pathname~~ [Credentials] Autofill when elements are in shadow Jul 15, 2024

dbajpeyi marked this pull request as ready for review July 15, 2024 05:46

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch 2 times, most recently from 77ef0e3 to 366baaa Compare July 15, 2024 05:55

feat: trying out categorising inputs

6f2363e

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch from 366baaa to 6f2363e Compare July 15, 2024 05:55

dbajpeyi requested a review from shakyShane July 15, 2024 06:13

test: add test

a359a12

dbajpeyi mentioned this pull request Jul 15, 2024

[Credentials] Add a test case to reproduce chase.com issue with custom form #600

Closed

style: add annotations

87aa95d

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch 5 times, most recently from eef0b68 to 99799e4 Compare July 15, 2024 07:13

refactor: keep track of shadow input to avoid re-scanning

2401665

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch from 99799e4 to 2401665 Compare July 15, 2024 07:13

fix: get shadowed submit buttons

ff41863

refactor: only discover shadow elements when needed

1c876a6

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch 2 times, most recently from c79a3ee to 6c30306 Compare July 16, 2024 08:01

feat: evaluate url and match against all components

b321a1f

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch from 6c30306 to b321a1f Compare July 16, 2024 08:03

dbajpeyi added 2 commits July 16, 2024 18:34

refactor: run shadow scan only when needed

6f19720

chore: fix types

6c543b6

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch from 5fb9d91 to 6c543b6 Compare July 16, 2024 09:49

dbajpeyi added 2 commits July 16, 2024 18:55

fix: check filtered form elements

deb7fb7

style: rename property

460bd1c

chore: update comment

2ec91b7

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch from bd8d639 to 2ec91b7 Compare July 17, 2024 01:23

fix: remove search from pathname

3d3133f

shakyShane requested changes Jul 17, 2024

View reviewed changes

src/autofill-utils.js Outdated Show resolved Hide resolved

src/Scanner.js Outdated Show resolved Hide resolved

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

chore: pr comments

2f8488c

shakyShane approved these changes Jul 19, 2024

View reviewed changes

GioSensation approved these changes Jul 19, 2024

View reviewed changes

chore: update comments

a01ca2f

dbajpeyi force-pushed the dbajpeyi/chase-autofill branch from 1d9e4e7 to a01ca2f Compare July 19, 2024 08:17

Merge branch 'main' into dbajpeyi/chase-autofill

5a80aab

dbajpeyi merged commit 88c8291 into main Jul 19, 2024
1 check passed

dbajpeyi deleted the dbajpeyi/chase-autofill branch July 19, 2024 08:27

dbajpeyi mentioned this pull request Jul 24, 2024

Update password-related json files (2024-07-19) #607

Merged

This was referenced Jul 29, 2024

Update autofill to 12.1.0 duckduckgo/BrowserServicesKit#916

Merged

Update autofill to 12.1.0 duckduckgo/Android#4810

Merged

Update BSK with autofill 12.1.0 duckduckgo/iOS#3155

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Credentials] Autofill when elements are in shadow #592

[Credentials] Autofill when elements are in shadow #592

dbajpeyi commented Jul 4, 2024 •

edited

Loading

dbajpeyi commented Jul 16, 2024 •

edited

Loading

GioSensation commented Jul 16, 2024

shakyShane left a comment

dbajpeyi commented Jul 18, 2024 •

edited

Loading

GioSensation left a comment

GioSensation Jul 19, 2024

GioSensation Jul 19, 2024

GioSensation Jul 19, 2024

dbajpeyi Jul 19, 2024

GioSensation Jul 19, 2024

		// If there are not form elements, we try to look for all
		// enclosed elements within the form.

		while (traversalLayerCount <= 5 && element.parentElement && element.parentElement !== document.documentElement) {
		while (traversalLayerCount <= 5 && element.parentElement !== document.documentElement) {

[Credentials] Autofill when elements are in shadow #592

[Credentials] Autofill when elements are in shadow #592

Conversation

dbajpeyi commented Jul 4, 2024 • edited Loading

Description

Steps to test

dbajpeyi commented Jul 16, 2024 • edited Loading

GioSensation commented Jul 16, 2024

shakyShane left a comment

Choose a reason for hiding this comment

dbajpeyi commented Jul 18, 2024 • edited Loading

GioSensation left a comment

Choose a reason for hiding this comment

GioSensation Jul 19, 2024

Choose a reason for hiding this comment

GioSensation Jul 19, 2024

Choose a reason for hiding this comment

GioSensation Jul 19, 2024

Choose a reason for hiding this comment

dbajpeyi Jul 19, 2024

Choose a reason for hiding this comment

GioSensation Jul 19, 2024

Choose a reason for hiding this comment

dbajpeyi commented Jul 4, 2024 •

edited

Loading

dbajpeyi commented Jul 16, 2024 •

edited

Loading

dbajpeyi commented Jul 18, 2024 •

edited

Loading