Skip to content

Conversation

jprusik
Copy link
Contributor

@jprusik jprusik commented Oct 16, 2025

🎟️ Tracking

PM-24650

📔 Objective

This PR addresses two issues that together fix autofill breakage on employee https://login.adp.com login portals.

The changes in this PR include:

  • ensuring we're executing fill actions serially
  • ensuring we don't re-enter values in fields that already have the value entered

📸 Screenshots and further details

The original issue looks like this:

current-bug.mp4

This occurs due to two behaviours. First, if the fields are filled too quickly, the web app (as designed) gets caught in an unintended state, where the username input change triggers the removal of the password field and revert of the submit button to the next step button. But because we fill the password input before it's removed, the app's inferences that determine which step it's on results in conflicting information that leaves the app in a bad state (that a human user would not be able to normally achieve).

One reason our fill script runs afoul of this scenario is because we execute the individual actions for filling a field (click, focus, fill) synchronously. This allows their execution to happen out of intended order and all at once, undermining the intention to simulate user entry. This is resolved by awaiting each action before executing the next (0390ce8).

While serial execution currently appears sufficient to resolve this case, our timings could potentially be slowed in the future, as we delay each fill action by 20ms (see runFillScriptAction in InsertAutofillContentService). We likely could go as high as 200ms before it will be noticeable, and should consider it for future changes, since it will bring us closer to simulating human entry speeds and avoiding breakages from app race cases like this (however, no adjustments to these timings have been made in this PR).

With the invalid app state concern resolved, we get behaviour closer to what users who are doing manual entry experience; the button remains, but reverts from "sign in" to "next". This happens because the page/app behaviour resets the login flow to the first step when the username field is changed in any way. This is what the page behaviour looks like when the user (with no password manager installed) does the same re-entry of the username like the extension is doing:

no-extension-app-behaviour.mp4

With the autofill script, we always enter field values regardless of any present values, and so trigger the app reset behaviour every time because we re-enter the username field value.

This is resolved by doing an (intentionally late) check on the field receiving the value. If the field has a value and that value matches what is about to be filled, the script will now skip filling that field (c8cf288).

The new autofill experience after these changes looks like this:

new-fix.mp4

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@jprusik jprusik requested a review from bensbits91 October 16, 2025 13:44
@jprusik jprusik changed the title [PM-24650] [PM-24650] Resolve sign in button disappearing from ADP login form Oct 16, 2025
Copy link
Contributor

github-actions bot commented Oct 16, 2025

Logo
Checkmarx One – Scan Summary & Detailse0f32946-a374-4cd0-8448-6ad466733ebd

Great job! No new security vulnerabilities introduced in this pull request

@jprusik jprusik self-assigned this Oct 17, 2025
@jprusik jprusik marked this pull request as ready for review October 17, 2025 13:33
@jprusik jprusik requested a review from a team as a code owner October 17, 2025 13:33
Copy link

Copy link

codecov bot commented Oct 17, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 39.16%. Comparing base (9ba1de7) to head (5b97f8a).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...tofill/services/insert-autofill-content.service.ts 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #16901   +/-   ##
=======================================
  Coverage   39.15%   39.16%           
=======================================
  Files        3456     3456           
  Lines       97968    97970    +2     
  Branches    14730    14732    +2     
=======================================
+ Hits        38360    38369    +9     
+ Misses      57939    57932    -7     
  Partials     1669     1669           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

},
metadata: {},
autosubmit: null,
autosubmit: [],
Copy link
Contributor Author

@jprusik jprusik Oct 17, 2025

Choose a reason for hiding this comment

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

unrelated typing warning fix

@bw-ghapp
Copy link
Contributor

bw-ghapp bot commented Oct 17, 2025

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and all feature flags disabled.

✅ Fortunately, these BIT tests have passed! 🎉

@bw-ghapp
Copy link
Contributor

bw-ghapp bot commented Oct 17, 2025

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and the feature flag configuration used by vault.bitwarden.com.

✅ Fortunately, these BIT tests have passed! 🎉

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.

1 participant