[PM-41294] feat: Build and fill identity autofill partitions through a multi-partition pipeline - #7369
Conversation
…or IdentityAutofill turned off
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed at I traced each changed path against the Code Review DetailsNo new findings in this round. Notes on the three existing threads, for the record:
Both deferred items need to land before |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## PM-41293/identity-autofill-fill-assist-field-mapping #7369 +/- ##
========================================================================================
- Coverage 86.41% 86.20% -0.21%
========================================================================================
Files 930 946 +16
Lines 67345 67967 +622
Branches 10197 10253 +56
========================================================================================
+ Hits 58198 58594 +396
- Misses 5567 5774 +207
- Partials 3580 3599 +19
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}
| // Gated behind FlagKey.IdentityAutofill until the feature is ready for | ||
| // production; disabled matches this partition's pre-feature behavior. | ||
| if (!isIdentityAutofillEnabled) return AutofillRequest.Unfillable | ||
| AutofillPartition.Identity( |
There was a problem hiding this comment.
Type.LOGIN.
Details and fix
FilledDataExtensions.kt:37-43 maps AutofillPartition.Identity to AutofillSelectionData.Type.LOGIN, justified by the comment "no-op placeholder today since nothing yet produces an Identity-only partition". This PR invalidates that premise, and nothing in the diff updates it.
Execution path once the flag is on and an Identity partition is built:
FillResponseBuilderImpl.buildunconditionally addsfilledData.buildVaultItemDataset(...).- That intent carries
Type.LOGIN, whichAutofillSelectionDataExtensionsmaps toVaultItemListingType.Login, so the "My vault" entry opens a Login-only item list on an identity form. - Selecting a login there runs
AutofillCompletionManagerImpl, which reparses the structure (still an Identity partition) and builds againstcipherView.toAutofillCipherProvider().getIdentityAutofillCiphers()returnsemptyList()for a login cipher, sofilledPartitions.firstOrNull()is null and the activity cancels with "Dataset not found" — nothing is filled.
When the vault is locked, getIdentityAutofillCiphers() returns empty, so the vault-item dataset is the only entry offered, and this is the whole identity-autofill experience for a locked vault.
Suggested fix: add an IDENTITY selection type (or omit the vault-item dataset for AutofillPartition.Identity while that type does not exist) so the fallback entry cannot route the user to a list that can never fulfil the request.
There was a problem hiding this comment.
@claude this will be addressed in a future branch. Until the feature is fully ready feature flag won't be active
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 1m 4s —— View job
Response
Understood — thanks for the context. Deferring the two flag-on findings to a follow-up branch makes sense given FlagKey.IdentityAutofill stays off through this rollout:
-
AutofillParserImpl.kt:211— vault-item dataset mapping Identity →Type.LOGIN(flag-gated, deferred) -
AutofillParserImpl.kt:277— Login fill-assist coverage onaccount-creation/account-updatehosts (flag-gated, deferred) -
HtmlInfoExtensions.kt:272— not flag-gated
One item the flag doesn't cover
The "autocomplete" removal from SUPPORTED_HTML_ATTRIBUTE_HINTS ships to production with the flag off:
private val SUPPORTED_HTML_ATTRIBUTE_HINTS: List<String> = listOf(
"name",
"label",
"type",
"hint",
"autofill",
)hints() is called unconditionally from the HtmlInfo heuristics — isUsernameField(), isPasswordField(), and the card patterns all read the same values, and none of those paths check FlagKey.IdentityAutofill. containsAnyTerms matches on the attribute name, and "autocomplete" doesn't contain "autofill", so autocomplete="email" / "cc-number" / "new-password" on a web form now contributes no hint at all. That's a behavior change to Login and Card detection today, regardless of the flag.
It may still be the right call — if some autocomplete values were misclassifying identity fields, dropping the attribute is one way to fix it. Two things would help:
- A line in the PR description saying it's a deliberate revert of the
autocompleteterm added ind23d1d17, and why. - Consider
IGNORED_RAW_HINTSfor the specific offending values instead, so the standard web attribute keeps contributing where it's correct. This file has no unit coverage, so a regression here wouldn't be caught by CI.
If it is intentional and scoped as-is, feel free to resolve — just wanted to make sure it wasn't swept in under the flag-off assumption alongside the other two.
| val loginCategories = if (isIdentityAutofillEnabled) { | ||
| LOGIN_FILL_ASSIST_CATEGORIES | ||
| } else { | ||
| LOGIN_FILL_ASSIST_CATEGORIES + IDENTITY_FILL_ASSIST_CATEGORIES | ||
| } |
There was a problem hiding this comment.
account-creation/account-update hosts.
Details and fix
With isIdentityAutofillEnabled == true, loginCategories is just account-login. So when focusedView is an AutofillView.Login on a host whose rules are categorized only account-creation or account-update, coversCurrentPartition is false and the curated selectors are discarded in favour of heuristics — a regression against today's behavior, where both categories are Login categories.
Those categories carry Login field keys: FIELD_KEY_NEW_PASSWORD ("newPassword") only makes sense on a signup or change-password flow. So on a registration or account-settings host, focusing the password or username field is exactly where the authored rules were meant to apply, and is exactly where they now stop applying. The category describes the page flow, not the field type.
Suggested fix: keep the identity categories additive for the Login branch in both flag states (LOGIN_FILL_ASSIST_CATEGORIES + IDENTITY_FILL_ASSIST_CATEGORIES); the new is AutofillView.Identity branch already claims these categories when an identity field is what has focus, and effectiveFocusedViewOrNull keeps Login authoritative afterwards.
Note the new tests cover the flag-off case for this shape (...account-creation and a login view is focused with IdentityAutofill disabled) but not the flag-on case.
There was a problem hiding this comment.
@claude this will be addressed in a future branch. Until the feature is fully ready feature flag won't be active
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 1m 19s —— View job
Understood — deferring the vault-item dataset routing
Thanks @aj-rosado, that disposition makes sense for that finding. AutofillPartition.Identity is only reachable when FlagKey.IdentityAutofill is on (AutofillParserImpl.kt:210 returns Unfillable otherwise), so with the flag inactive the Type.LOGIN mapping in FilledDataExtensions.kt:37-43 stays the no-op its comment describes. Nothing to change in this PR — worth carrying into the follow-up ticket so the placeholder comment gets retired alongside the new selection type.
The same reasoning covers my second comment (fill-assist categories on account-creation/account-update): the narrowing only takes effect with the flag on, so it can ride the same follow-up. One thing to note for when the flag flips — that one is a regression against today's shipped behavior rather than an incomplete new path, so it's easy to lose track of. If you'd rather not carry it, keeping the categories additive in both branches is a two-line change and the new is AutofillView.Identity branch already handles identity-focused fields.
Still open: the autocomplete removal is not flag-gated
This one isn't covered by the flag argument, so it's worth a decision before merge.
SUPPORTED_HTML_ATTRIBUTE_HINTS at HtmlInfoExtensions.kt:267-273 contains "autocomplete" on main (present at b1d6d6b); the first commit on this branch (f926f4c) drops it, and it's still absent at HEAD. hints() feeds every HtmlInfo heuristic — isPasswordField(), isUsernameField(), all seven card predicates — none of which consult the flag. So on web forms, autocomplete="new-password", "username", "cc-number" etc. stop contributing a hint today, flag off included.
If that was deliberate (some autocomplete values misclassifying identity fields), a one-line note here is enough and I'll drop it — though IGNORED_RAW_HINTS would scope the fix to the offending values rather than the whole attribute. If it was incidental to the heuristics work, restoring the entry is the safer call; there's no unit coverage on this file to catch the difference.
No code changes made — say the word if you'd like me to push either fix.
· branch PM-41294/identity-autofill-multi-partition-pipeline
| "label", | ||
| "type", | ||
| "hint", | ||
| "autofill", |
There was a problem hiding this comment.
❓ QUESTION: Why is "autocomplete" dropped from the supported HTML attribute hints here?
Details
hints() feeds every HtmlInfo heuristic, not just the identity ones — isUsernameField(), isPasswordField(), and the card patterns all read the same attribute values. Dropping autocomplete means the standard web attribute (autocomplete="email", "cc-number", "new-password", …) no longer contributes a hint on web forms, and unlike the rest of this PR the change is not gated by FlagKey.IdentityAutofill, so it also affects Login and Card detection today.
The term was added by the merged identity-heuristics change (d23d1d1) and is removed again here with no mention in the PR description. If this is a deliberate revert because some autocomplete values were misclassifying fields, it would help to say so — and possibly to handle it via IGNORED_RAW_HINTS rather than dropping the whole attribute, since this file has no unit coverage to catch the regression.
There was a problem hiding this comment.
@claude autocomplete is not being used by autofill framework so I'm removing it
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 1m 6s —— View job
Question resolved — reasoning checks out
- Gather context on
HtmlInfoExtensions.hints()and attribute matching - Verify the claim that
autocompletenever contributes a hint - Check for test coverage that would regress
Thanks — that explanation holds up, and I traced why.
hints() filters HtmlInfo.attributes by substring, case-insensitive match on the attribute name (HtmlInfoExtensions.kt:189-193 → StringExtensions.kt:9-18). So "autocomplete" in the list only ever mattered if a browser literally forwarded an attribute whose name contains autocomplete. Chromium's AutofillProvider doesn't: it surfaces the parsed autocomplete signal as ua-autofill-hints (alongside the server/crowdsourcing prediction hint attributes), not as the raw autocomplete attribute. Those are already admitted by the "autofill" and "hint" terms that remain — which is exactly what the KDoc on SUPPORTED_HTML_ATTRIBUTE_HINTS (HtmlInfoExtensions.kt:261-266) documents. And type / name / label still cover type="email", type="tel", name="cc-number", etc.
So the entry was dead weight rather than an active signal, and dropping it is the right call over routing it through IGNORED_RAW_HINTS (that filters values, not attribute names — it wouldn't have applied here anyway).
Two small notes, neither blocking:
- The only theoretical loss is a non-Chromium WebView that forwards a raw
autocompleteattribute; thereautocomplete="new-password"/"cc-number"would no longer contribute. Given the framework path goes throughAutofillProvider, that's not a case we see in practice. - No test regression to worry about: this file is untestable under unit tests (
HtmlInfowrapsandroid.util.Pair, per the KDoc atHtmlInfoExtensions.kt:170-178), and the onlyautocompletereferences in tests areFillAssistManagerTestselector-parsing cases, which are unrelated. Nothing to update.
Since the intent was that it's unused rather than a behavior revert, it may be worth a one-liner in the PR description so the flag-independent line doesn't read as an accidental drop to the next reviewer. No code change needed from my side — closing this out as answered.
• Branch: PM-41294/identity-autofill-multi-partition-pipeline
aj-rosado
added
t:feature
| private suspend fun buildFilledPartitions( | ||
| autofillRequest: AutofillRequest.Fillable, | ||
| getCipherInlinePresentationOrNull: () -> InlinePresentationSpec?, | ||
| ): List<FilledPartition> = when (autofillRequest.partition) { |
| /** | ||
| * Get the autofill value for the given [autofillView], or null if no value is available. | ||
| */ | ||
| @Suppress("CyclomaticComplexMethod") |
| val autofillViews = traversalDataList | ||
| .selectCandidateAutofillViews(urlBarWebsite = urlBarWebsite) { | ||
| it !is AutofillView.Unused && it !is AutofillView.Identity | ||
| it !is AutofillView.Unused |
|
|
||
| // Identity categories were Login categories before identity autofill, so with the flag | ||
| // off they must stay Login's to keep fill-assist coverage unchanged on those hosts. | ||
| val loginCategories = if (isIdentityAutofillEnabled) { |
There was a problem hiding this comment.
Maybe rename this to fillAssistCategories since it can now contain identity categories too
| is AutofillView.Identity.PostalAddressFull -> { | ||
| this.copy(data = this.data.copy(website = site)) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this is supposed to be here, right?
| this.isAddressCountryField -> AutofillHint.Identity.ADDRESS_COUNTRY | ||
| this.isPostalCodeField -> AutofillHint.Identity.POSTAL_CODE | ||
| this.isPhoneField -> AutofillHint.Identity.PHONE_FULL | ||
| this.isPhoneField -> AutofillHint.Login.USERNAME |
There was a problem hiding this comment.
Does this mean that AutofillHint.Identity.PHONE_FULL is never used?
There was a problem hiding this comment.
It will autofill phone numbers, just not display the identity when pressing on a phone number
| nameView, | ||
| AutofillView.Identity.Email(data = emailLoginView.data), | ||
| ), | ||
| (partition as AutofillPartition.Identity).views, |
There was a problem hiding this comment.
Can we just compare the entire result
| // container-redirect leftover, not an intentional sibling. | ||
| is AutofillView.Identity -> { | ||
| (id !in idsClaimedByAncestor) | ||
| .also { keep -> if (keep) claimedAutofillIds.add(id) } |
There was a problem hiding this comment.
Could we just do this:
if (id !in idsClaimedByAncestor) {
claimedAutofillIds.add(id)
}That may be more straight forward to read.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41294
📔 Objective
Adds Identity autofill, behind
FlagKey.IdentityAutofill:AutofillParserImpl.kt— builds and fills a realAutofillPartition.Identity.ViewNodeExtensions.kt— heuristic detection of identity fields (name, address, phone, etc.), classified alongside Login/Card.FillAssistViewNodeExtensions.kt— fill-assist targeting rule support for identity field keys.FilledDataBuilderImpl.kt— fills the Identity partition from the vault.📸 Screenshots
N/A — no UI changes.