[PM-41292] feat: Add heuristic detection for identity autofill fields - #7233
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed Phase C of Identity Autofill: the new identity hint term lists in Code Review Details
Lower-priority note not posted inline: several terms are matched as substrings on |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## PM-41291/identity-autofill-model-and-data-layer #7233 +/- ##
===================================================================================
+ Coverage 85.38% 85.51% +0.12%
===================================================================================
Files 1041 1041
Lines 67785 68074 +289
Branches 9903 10048 +145
===================================================================================
+ Hits 57878 58213 +335
+ Misses 6362 6227 -135
- Partials 3545 3634 +89
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| is AutofillView.Identity -> { | ||
| // Identity partition construction is wired up in a later phase; treating it as | ||
| // unfillable for now is a no-op since nothing yet classifies a view as Identity. | ||
| // Identity partition construction lands in Phase D. Unfillable until then. |
There was a problem hiding this comment.
❌ CRITICAL: Classifying identity fields today (with no Identity partition) turns requests that used to be fillable into Unfillable, removing autofill suggestions from registration/checkout/phone-login screens.
Details and fix
Before this PR an identity-looking field (e.g. idEntry/hint of "First name", "City", "Mobile number") resolved to AutofillView.Unused. autofillViews filters Unused out, so firstFocusedOrNull() fell through to ?: firstOrNull() and returned a Login/Card view — the request stayed Fillable.
Now those fields resolve to AutofillView.Identity.*, survive the it !is AutofillView.Unused filter, and become the focused view — so this branch returns Unfillable.
Traceable regressions:
- Signup form
[First name][Email][Password]. User taps First name → focused view isIdentity.PersonNameGiven→Unfillable. Previously the user got the login suggestion. - Login screen whose identifier field is
hint = "Mobile number".isUsernameFielddoesn't match ("mobile" isn't inSUPPORTED_RAW_USERNAME_HINTS), so it is nowIdentity.PhoneFullinstead ofUnused. That also meansupdateForMissingUsernameFields()can no longer promote it toLogin.Username, so login autofill is lost on that screen entirely.
This contradicts the PR description's "behavior-neutral in production until a later stacked phase".
Suggested fix — keep Identity out of the heuristic candidate set until Phase D lands, e.g. in selectCandidateAutofillViews:
val autofillViews = traversalDataList
.selectCandidateAutofillViews(urlBarWebsite = urlBarWebsite) {
it !is AutofillView.Unused && it !is AutofillView.Identity
}or gate the new identity heuristics behind a feature flag so classification and fulfillment ship together.
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 3m 31s —— View job
Re-checked against be0bed8
- Gather PR context / find the new commit
- Re-verify the dual-classification dedup finding (this thread)
- Re-check the
Unfillablefinding - Report back
The push landed — be0bed8 Gating Identity autofill behind feature flag. Here's what it does and doesn't cover.
✅ Fixed: the Unfillable regression (the other thread)
Two layers, both correct:
FlagKey.IdentityAutofillis read inparseInternaland threaded throughtraverse()→toAutofillView(). With the flag off,supportedAutofillHint()short-circuits at!isIdentityAutofillEnabled -> null(ViewNodeExtensions.kt:170) after the card checks andSUPPORTED_IDENTITY_VIEW_HINTSis excluded fromfirstSupportedAutofillHintOrNull— so every node classifies exactly as it did on the base branch.- Belt-and-braces:
it !is AutofillView.Unused && it !is AutofillView.IdentityinselectCandidateAutofillViews(AutofillParserImpl.kt:141), so even with the flag on a focused identity field falls through instead of forcingUnfillable.
Both of my traced cases are now covered by tests — the signup-form fall-through and the updateForMissingUsernameFields promotion of a phone-hinted login field (AutofillParserTests.kt:1721, :1791, :1860). Good call codifying the flag-on variant too; it documents that phone-login username promotion is still lost once the flag flips, which is the right thing for Phase D to pick up.
⚠️ Not fixed: this thread's finding (dedup filter)
AutofillParserImpl.kt:470-482 is still byte-for-byte the base-branch filter — no AutofillView.Identity case:
.filter { view ->
val id = view.data.autofillId
if (id in claimedAutofillIds) {
false
} else if (view !is AutofillView.Unused) {
claimedAutofillIds.add(id)
true
} else {
true
}
}So the sibling added at :442-459 is still dropped by the parent node whenever the classified field isn't a window root — which it never is in a real structure. AutofillParserTests.kt has no case asserting a dual-classified sibling survives traversal.
Severity is much lower than when I first flagged it, though, and it's reasonable to leave: with the flag off no sibling is created at all, and with the flag on the sibling is dropped by this filter and excluded from candidates by the new it !is AutofillView.Identity guard. Either way it's currently unreachable code rather than a user-visible defect. The practical consequence is just that the dual-classification block is dead and untested until Phase D — at which point the filter has to change or whole-identity fill will skip the email/phone field.
Your call: fix it now (small diff + one parser test), or note it on the Phase D ticket. Fix this → — or say the word and I'll push it here.
One thing the flag doesn't cover
"autocomplete" was added to SUPPORTED_HTML_ATTRIBUTE_HINTS (HtmlInfoExtensions.kt:276), and that list feeds hints() for all checks — including the pre-existing isUsernameField/isPasswordField/card ones. That change is live with the flag off, so "classifies exactly as before" holds for native nodes but not for web nodes.
I checked the collision surface and it looks like a clean win rather than a risk: cc-number, cc-exp-month, cc-name, email, new-password now match (all correctly), while one-time-code, tel, off and nickname match nothing. Worth a conscious decision rather than an accident, since it's the one part of Phase C that changes production behavior today.
• branch PM-41292/identity-autofill-heuristic-field-detection
| // An email-hinted or email-heuristic field is offered as both a Login candidate | ||
| // (above) and an Identity candidate, since the two partitions aren't mutually | ||
| // exclusive for this field. Reuses the same (container-redirect-corrected) data as | ||
| // the primary view rather than re-deriving it. | ||
| if (view is AutofillView.Login.Username && this.isEmailField) { | ||
| mutableAutofillViewList.add(AutofillView.Identity.Email(data = view.data)) | ||
| } | ||
|
|
||
| // Some phone hints (e.g. "mobilephone") also match the username heuristic's "phone" | ||
| // term and resolve to Login.Username above, so they need the same dual-classification | ||
| // as email. | ||
| if (view is AutofillView.Login.Username && this.isPhoneField) { | ||
| mutableAutofillViewList.add(AutofillView.Identity.PhoneFull(data = view.data)) | ||
| } |
There was a problem hiding this comment.
Identity siblings are dropped by the dedup filter in the ancestor's traversal, so they never reach the parser output.
Details and fix
The PR description states "the container-redirect dedup filter in traversal is updated to special-case AutofillView.Identity so these dual-classification siblings aren't dropped as duplicates", but the filter below is unchanged from the base branch.
Trace for an email EditText (id X) nested under any parent node:
- In the leaf's
traverse(), the list becomes[Login.Username(X), Identity.Email(X)]. - The parent iterates that list through the dedup filter:
Login.Username(X)isn't claimed yet → kept, andXis added toclaimedAutofillIds. Identity.Email(X)then hitsif (id in claimedAutofillIds) false→ filtered out.
Since the classified field is never the root node passed to traverse(), the sibling is always dropped. The feature is currently unreachable, and nothing asserts it (AutofillParserTests has no dual-classification case).
Fix — let a non-Unused view through when its id is claimed by a view of a different partition, e.g.:
.filter { view ->
val id = view.data.autofillId
when {
// A dual-classified Identity sibling deliberately shares its id with the
// primary Login/Card view, so it must not be treated as a duplicate.
view is AutofillView.Identity -> true
id in claimedAutofillIds -> false
view !is AutofillView.Unused -> {
claimedAutofillIds.add(id)
true
}
else -> true
}
}Please also add a parser test covering a nested email/phone node so this can't silently regress.
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 1m 48s —— View job
Re-checked against the pushed branch
- Gather PR context
- Check current branch state for the reported fixes
- Re-verify the flagged findings against the code
- Report back
I don't see a fix on the branch — the PR head is still 1160476 (Split buildAutoFillView to have a better readability), the same two commits as when the review ran. If you fixed this locally, it looks like the commit hasn't been pushed yet.
Both remaining findings are still present in 1160476:
1. Identity focus ⇒ Unfillable — AutofillParserImpl.kt:130-133 still admits Identity views into the heuristic candidate set (only Unused is filtered), so a focused identity field reaches AutofillParserImpl.kt:191-194 and returns Unfillable:
is AutofillView.Identity -> {
// Identity partition construction lands in Phase D. Unfillable until then.
return AutofillRequest.Unfillable
}The only change here versus the base branch is the comment wording. The [First name][Email][Password] signup case and the hint = "Mobile number" login case both still lose their login suggestion.
2. Dual-classified siblings still dropped — the dedup filter at AutofillParserImpl.kt:447-457 is byte-for-byte unchanged from the base branch; there's no AutofillView.Identity special case:
.filter { view ->
val id = view.data.autofillId
if (id in claimedAutofillIds) {
false
} else if (view !is AutofillView.Unused) {
claimedAutofillIds.add(id)
true
} else {
true
}
}So the Identity.Email(X) / Identity.PhoneFull(X) views added at AutofillParserImpl.kt:426-435 are filtered out by the parent node's traversal whenever the field isn't the root node. AutofillParserTests.kt still has no case asserting a dual-classified sibling survives traversal (the only diff there is two every { hint } returns null stubs).
Happy to push either fix if you'd like — just say which, or push your local commit and I'll re-verify.
• branch PM-41292/identity-autofill-heuristic-field-detection
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 5m 9s —— View job
Re-verified against be0bed8
- Gather PR context / new commits
- Finding 1 — Identity focus ⇒
Unfillable - Finding 2 — dual-classified siblings dropped by dedup filter
- Check the new gate for holes
- Report back
New commit since the last check: be0bed8 Gating Identity autofill behind feature flag (AutofillParserImpl.kt, ViewNodeExtensions.kt, + 716 lines of tests).
✅ Finding 1 — fixed
The gate is in two independent layers and I traced both:
- Classification —
ViewNodeExtensions.kt:149-183: identityautofillHintswere split out intoSUPPORTED_IDENTITY_VIEW_HINTSand are only unioned into the lookup when the flag is on (:196-207), and the heuristicwhenshort-circuits with!isIdentityAutofillEnabled -> nullplaced after the username/password/card branches and before everyisXxx…Fieldidentity branch. So with the flag off thewhenis byte-equivalent to the pre-PR one. - Candidate selection —
AutofillParserImpl.kt:141-144now also excludesAutofillView.Identity, so even with the flag on a focused identity field can't forceUnfillablethrough the heuristic path.
FlagKey.IdentityAutofill defaults to false (core/.../FlagKey.kt:226-229), and I confirmed AutofillView.Identity has no other construction site that escapes the gate — only buildIdentityView (reachable only via an IDENTITY_* hint) and the two dual-classification sites in traverse(), which are now inside if (isIdentityAutofillEnabled). Both of my original repro cases are covered by new tests: AutofillParserTests.kt:1721 (focused Identity + Login elsewhere ⇒ Login partition) and :1791 (phone-hinted field still promoted by updateForMissingUsernameFields with the flag off).
⚠️ Finding 2 — still not fixed, but no longer user-visible
The dedup filter at AutofillParserImpl.kt:470-480 is unchanged from the base branch — there's no AutofillView.Identity case:
.filter { view ->
val id = view.data.autofillId
if (id in claimedAutofillIds) {
false
} else if (view !is AutofillView.Unused) {
claimedAutofillIds.add(id)
true
} else {
true
}
}The trace still holds: a child returning [Login.Username(X), Identity.Email(X)] has X claimed by Login.Username first, so Identity.Email(X) is dropped in the parent's filter whenever the classified node isn't the root.
Severity drops, though, because the new it !is AutofillView.Identity exclusion at :143 removes identity views from autofillViews in both flag states — so the dual-classification at :444-456 has no observable effect either way in this phase. It's now a latent Phase D blocker rather than a live bug. Two things to settle:
- The PR description still claims "the container-redirect dedup filter in traversal is updated to special-case
AutofillView.Identity" — that's not in the diff; worth correcting so Phase D doesn't inherit the assumption. - No test pins the dual-classification survival, so it can't regress-detect. Fix this →
Three things the new gate doesn't cover
A. autocomplete is added ungated, so flag-off Login/Card classification does change. HtmlInfoExtensions.kt:276 adds "autocomplete" to SUPPORTED_HTML_ATTRIBUTE_HINTS, but hints() (:180-197) is shared by isPasswordField(), isUsernameField() and every isCard*Field() — none of which are behind the flag. Concretely, with IdentityAutofill off:
<input type="text" name="q" autocomplete="email">→"email"∈SUPPORTED_RAW_USERNAME_HINTS→htmlInfo.isUsernameField()true →Login.Username. Before this PR:Unused.<input autocomplete="cc-number">→ full-matchesSUPPORTED_RAW_CARD_NUMBER_HINT_PATTERNS→Card.Number. Before:Unused.
Both are plausibly improvements, so I'm not calling it a regression — but it contradicts the new comment at AutofillParserImpl.kt:123-125 ("every node must classify exactly as it did before identity heuristics existed") and the PR's "behavior-neutral in production" claim. Either move the attribute behind the flag or update those two statements. Fix this →
B. ViewNodeExtensionsTest.kt passes isIdentityAutofillEnabled = true; the string isIdentityAutofillEnabled = false appears exactly once in the whole test tree (AutofillParserTests.kt:1830), and that test mocks toAutofillView, so it never exercises the real supportedAutofillHint gate. The mechanism that prevents the Finding-1 regression is currently unpinned — a future edit that moves the !isIdentityAutofillEnabled -> null branch above the card checks, or drops it, would go green. Worth three assertions: identity autofillHints + flag off ⇒ Unused; hint = "First name" + flag off ⇒ Unused; hint = "Mobile number" + flag off ⇒ Unused. Fix this →
C. 💡 With the flag on, an Identity field loses the fill-assist rescue. AutofillParserImpl.kt:261-274: coversCurrentPartition returns false for is AutofillView.Identity, and the receiver of toEffectiveViews is the Identity-excluded (hence empty) autofillViews — so effectiveViews is empty and the request is Unfillable. With the flag off the same field is Unused, hits the is AutofillView.Unused -> branch, and gets rescued. So flag-on is strictly worse than flag-off on fill-assist hosts. Not a production issue (flag defaults off) and the tests at :1860 document the adjacent flag-on tradeoff deliberately, but the comment at :269-271 ("no-op today since nothing yet classifies a view as Identity") is now stale — with the flag on, things do. Fix this →
Net: the critical finding is genuinely resolved and the flag-off path is equivalent to the base branch for the native heuristics. Remaining before merge, in priority order: (A) reconcile the ungated autocomplete change with the "behavior-neutral" claim, (B) add flag-off tests for the gate, (2) correct the PR description's dedup-filter claim and decide whether to fix the filter now or in Phase D. (C) is a Phase D note.
I couldn't run ./gradlew :app:testStandardDebugUnitTest or detekt in this environment (command not permitted), so this is static analysis only — CI is the source of truth for the new tests passing.
• branch PM-41292/identity-autofill-heuristic-field-detection
| "type", | ||
| "hint", | ||
| "autofill", | ||
| "autocomplete", |
There was a problem hiding this comment.
autocomplete tokens have no matching term, so the new web path silently misses them — including tel, which the KDoc above cites as a supported signal.
Details and fix
hints() values are passed through toLowerCaseAndStripNonAlpha() (which also strips digits) and then substring-matched against the SUPPORTED_RAW_* lists. Checking the WHATWG token list against those lists:
autocomplete |
after normalize | matches? |
|---|---|---|
tel |
tel |
❌ (phonenumber/telephone/mobilephone/mobile) |
name (full name) |
name |
❌ (fullname) |
additional-name |
additionalname |
❌ (middlename) |
address-level2 (city) |
addresslevel |
❌ (locality/city/town) |
address-level1 (region) |
addresslevel |
❌ (addressregion/province/state) |
given-name, family-name, honorific-prefix, street-address, address-line1, postal-code, organization, country-name and email all do match, so the gap is limited to the rows above — but phone and full name are among the most common identity fields on web forms, and type="tel" misses for the same reason.
Because address-level1/address-level2 normalize to the same string, they also can't be distinguished by substring matching on the stripped value.
Suggested fix: match autocomplete values as exact tokens on the raw (un-stripped) value in a separate lookup — e.g. tel → IDENTITY_PHONE_FULL, name → IDENTITY_PERSON_NAME_FULL, additional-name → IDENTITY_PERSON_NAME_MIDDLE, address-level2 → IDENTITY_ADDRESS_LOCALITY, address-level1 → IDENTITY_ADDRESS_REGION — rather than adding short substrings like tel/name to the term lists (tel would match "hotel", name would match nearly everything).
There was a problem hiding this comment.
@claude I have not found those extra fields would improve our autofill. Will take it as a note and add them for future iterations if problems on Identity autofill appears
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 47s —— View job
Acknowledged — deferring the extra autocomplete tokens
- Read the trigger comment and identify which finding it addresses
- Re-verify the claim against the current branch
- Respond
That's a reasonable call, and it's your judgment to make. The finding was a coverage gap, not a regression — nothing that matches today stops matching, so deferring it costs nothing beyond a few forms that won't classify until you revisit it.
For the record, so the note has something concrete to point at when you do come back to it, the gap on the current branch is exactly these five tokens:
autocomplete |
normalized by toLowerCaseAndStripNonAlpha() |
nearest term list |
|---|---|---|
tel |
tel |
phonenumber / telephone / mobilephone / mobile |
name |
name |
fullname (ViewStructureUtils.kt:164) |
additional-name |
additionalname |
middlename (:187) |
address-level2 |
addresslevel |
locality / city / town (:221) |
address-level1 |
addresslevel |
addressregion / province / state (:230) |
Everything else in the WHATWG list that you cover (given-name, family-name, honorific-prefix, street-address, address-line1, postal-code, organization, country-name, email) matches fine. The address-level1/address-level2 collision is the one worth remembering — both normalize to addresslevel, so substring matching on the stripped value can't tell region from city regardless of which terms get added. That one needs exact-token matching on the raw value, not a new term.
One optional tidy-up while it's fresh: the KDoc on SUPPORTED_HTML_ATTRIBUTE_HINTS (HtmlInfoExtensions.kt:262-267) cites tel as an example of a token this path handles, which is the one case in that sentence that doesn't currently resolve. Swapping the example (or dropping tel from it) would keep the comment matching the behavior. Happy to push that one-line change if you want it — otherwise no action needed here and this thread can be resolved.
Note that the two other findings from the review are separate threads and unaddressed as of 1160476; this response covers only the autocomplete token gap.
• branch PM-41292/identity-autofill-heuristic-field-detection
d4d3b08 to
982905e
Compare
077600b to
1160476
Compare
1160476 to
be0bed8
Compare
982905e to
6f0c34c
Compare
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41292
📔 Objective
Phase C of Identity Autofill (PM-38138): teaches the heuristic detection path (
ViewNodeExtensions.kt,HtmlInfoExtensions.kt,ViewStructureUtils.kt) to classify identity fields — name, all address parts, phone, company, SSN, passport, and license — using both nativeautofillHints/idEntry/hintand webautocomplete/HTML attribute signals. Stacked on #7232 (model/data layer).ViewStructureUtils.kt, evidence-backed against real observedidEntry/hint/HTML attribute values rather than speculative guesses.ViewNodeExtensions.ktandHtmlInfoExtensions.ktgain the correspondingisXxxFieldchecks and dispatch into the newAutofillView.Identity.*leaves.Login.*primary and a siblingIdentity.Email/Identity.PhoneFull(sameautofillId) — the two partitions aren't mutually exclusive for that field. This is retained deliberately so a whole-identity fill still populates the login-classified email/phone field.buildAutofillView's dispatcher was extracted into a newAutofillViewBuilderExtensions.ktfor readability as it grew to cover every identity leaf.AutofillView.Identityso these dual-classification siblings aren't dropped as duplicates.Classification only — nothing yet builds an
AutofillPartition.Identityor offers identity suggestions, so this remains behavior-neutral in production until a later stacked phase turns it into observable behavior.📸 Screenshots
N/A — heuristic detection logic only, no UI changes.
═══════════════════════════════════════