[PM-41294] feat: Build and fill identity autofill partitions through a multi-partition pipeline #7369
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,8 +62,6 @@ private val URL_BARS: Map<String, String> = mapOf( | |
| */ | ||
| private val LOGIN_FILL_ASSIST_CATEGORIES: List<String> = listOf( | ||
| "account-login", | ||
| ) | ||
|
|
||
| /** | ||
|
|
@@ -73,6 +71,17 @@ private val CARD_FILL_ASSIST_CATEGORIES: List<String> = listOf( | |
| "payment-card", | ||
| ) | ||
|
|
||
| /** | ||
| * A list of categories from Fill Assist that are used for [AutofillView.Identity]. | ||
| * | ||
| * Account creation and account update flows are where identity fields (name, address, phone, | ||
| * etc.) are curated alongside credential fields. | ||
| */ | ||
| private val IDENTITY_FILL_ASSIST_CATEGORIES: List<String> = listOf( | ||
| "account-creation", | ||
| "account-update", | ||
| ) | ||
|
|
||
| /** | ||
| * The default [AutofillParser] implementation for the app. This is a tool for parsing autofill data | ||
| * from the OS into domain models. | ||
|
|
@@ -132,15 +141,10 @@ class AutofillParserImpl( | |
| .flatMap { it.urlBarWebsites } | ||
| .firstOrNull() | ||
| // Heuristic views: the focused node's candidates with unfillable (Unused) fields removed, | ||
| // falling back to all fillable views when nothing has focus. | ||
| val autofillViews = traversalDataList | ||
| .selectCandidateAutofillViews(urlBarWebsite = urlBarWebsite) { | ||
| it !is AutofillView.Unused | ||
|
|
||
| } | ||
|
|
||
| val isFillAssistEnabled = featureFlagManager | ||
|
|
@@ -183,9 +187,7 @@ class AutofillParserImpl( | |
| autofillViews | ||
| } | ||
|
|
||
| val effectiveFocusedView = effectiveViews.effectiveFocusedViewOrNull() | ||
| ?: return AutofillRequest.Unfillable | ||
|
|
||
| // Choose the first focused partition of data for fulfillment. | ||
|
|
@@ -203,8 +205,12 @@ class AutofillParserImpl( | |
| } | ||
|
|
||
| is AutofillView.Identity -> { | ||
| // 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( | ||
|
Contributor
There was a problem hiding this comment.
Details and fix
Execution path once the flag is on and an Identity partition is built:
When the vault is locked, Suggested fix: add an
Contributor
Author
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
Contributor
There was a problem hiding this comment. Claude finished @aj-rosado's task in 1m 4s —— View job ResponseUnderstood — thanks for the context. Deferring the two flag-on findings to a follow-up branch makes sense given
One item the flag doesn't coverThe private val SUPPORTED_HTML_ATTRIBUTE_HINTS: List<String> = listOf(
"name",
"label",
"type",
"hint",
"autofill",
)
It may still be the right call — if some
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. |
||
| views = effectiveViews.filterIsInstance<AutofillView.Identity>(), | ||
| ) | ||
| } | ||
|
|
||
| is AutofillView.Unused -> { | ||
|
|
@@ -262,16 +268,26 @@ class AutofillParserImpl( | |
| } | ||
| ?: return this | ||
|
|
||
| // 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) { | ||
|
Collaborator
There was a problem hiding this comment. Maybe rename this to |
||
| LOGIN_FILL_ASSIST_CATEGORIES | ||
| } else { | ||
| LOGIN_FILL_ASSIST_CATEGORIES + IDENTITY_FILL_ASSIST_CATEGORIES | ||
| } | ||
|
Comment on lines
+273
to
+277
Contributor
There was a problem hiding this comment.
Details and fixWith Those categories carry Login field keys: Suggested fix: keep the identity categories additive for the Login branch in both flag states ( Note the new tests cover the flag-off case for this shape (
Contributor
Author
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
Contributor
There was a problem hiding this comment. Claude finished @aj-rosado's task in 1m 19s —— View job Understood — deferring the vault-item dataset routingThanks @aj-rosado, that disposition makes sense for that finding. The same reasoning covers my second comment (fill-assist categories on Still open: the
|
||
|
|
||
| val coversCurrentPartition = hostRules.any { rule -> | ||
| when (focusedView) { | ||
| is AutofillView.Card -> rule.category in CARD_FILL_ASSIST_CATEGORIES | ||
| is AutofillView.Login -> rule.category in loginCategories | ||
| is AutofillView.Identity -> rule.category in IDENTITY_FILL_ASSIST_CATEGORIES | ||
| is AutofillView.Unused -> { | ||
| val identityCoversRule = isIdentityAutofillEnabled && | ||
| rule.category in IDENTITY_FILL_ASSIST_CATEGORIES | ||
| rule.category in loginCategories || | ||
| rule.category in CARD_FILL_ASSIST_CATEGORIES || | ||
| identityCoversRule | ||
| } | ||
| } | ||
| } | ||
| if (!coversCurrentPartition) return this | ||
|
|
@@ -337,6 +353,18 @@ private fun List<ViewNodeTraversalData>.selectCandidateAutofillViews( | |
| private fun List<AutofillView>.firstFocusedOrNull(): AutofillView? = | ||
| firstOrNull { it.data.isFocused } ?: firstOrNull() | ||
|
|
||
| /** | ||
| * Returns the [AutofillView] that should win partition selection: a focused Login/Card view | ||
| * first, then any focused view (so a real Identity partition can still be built when Identity is | ||
| * the only classification available), then any Login/Card view, then any view at all. | ||
| */ | ||
| private fun List<AutofillView>.effectiveFocusedViewOrNull(): AutofillView? { | ||
| val focusedNonIdentity = firstOrNull { it.data.isFocused && it !is AutofillView.Identity } | ||
| val focusedAny = firstOrNull { it.data.isFocused } | ||
| val nonIdentity = firstOrNull { it !is AutofillView.Identity } | ||
| return focusedNonIdentity ?: focusedAny ?: nonIdentity ?: firstOrNull() | ||
| } | ||
|
|
||
| /** | ||
| * This helper function updates the [ViewNodeTraversalData] if necessary for missing password | ||
| * fields that were marked invalid because they contained a specific `hint` or `idEntry`. If the | ||
|
|
@@ -452,9 +480,8 @@ private fun AssistStructure.ViewNode.traverse( | |
| mutableAutofillViewList.add(AutofillView.Identity.Email(data = view.data)) | ||
| } | ||
|
|
||
| // A phone-hinted or phone-heuristic field resolves to Login.Username above (see | ||
| // supportedAutofillHint), so it needs the same dual-classification as email. | ||
| if (view is AutofillView.Login.Username && this.isPhoneField) { | ||
| mutableAutofillViewList.add(AutofillView.Identity.PhoneFull(data = view.data)) | ||
| } | ||
|
|
@@ -471,17 +498,29 @@ private fun AssistStructure.ViewNode.traverse( | |
| isIdentityAutofillEnabled = isIdentityAutofillEnabled, | ||
| ) | ||
| .let { viewNodeTraversalData -> | ||
| // Ids already claimed by an ancestor's own view (e.g. a container-redirect | ||
| // target) before this child's results are considered. A primary always precedes | ||
| // its Identity dual-classification sibling in this same child's results, so the | ||
| // sibling's id is still fresh here and passes the check below. | ||
| val idsClaimedByAncestor = claimedAutofillIds.toSet() | ||
| // Flatten child views into this node, keeping the first view seen for each autofill | ||
| // id and dropping later duplicates (e.g. a container-redirect leftover). | ||
| viewNodeTraversalData.autofillViews | ||
| .filter { view -> | ||
| val id = view.data.autofillId | ||
| when (view) { | ||
| // Never claims an id, so a real view for that id can still be kept. | ||
| is AutofillView.Unused -> id !in claimedAutofillIds | ||
| // Kept if the id is fresh, i.e. it's a dual-classification sibling of | ||
| // a primary view from this same child. Dropped if the id was already | ||
| // claimed by an ancestor's own view -- that means this is a stale | ||
| // container-redirect leftover, not an intentional sibling. | ||
| is AutofillView.Identity -> { | ||
| (id !in idsClaimedByAncestor) | ||
| .also { keep -> if (keep) claimedAutofillIds.add(id) } | ||
|
Collaborator
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. |
||
| } | ||
| // Kept only the first time its id is seen (add returns false if known). | ||
| else -> claimedAutofillIds.add(id) | ||
| } | ||
| } | ||
| .forEach(mutableAutofillViewList::add) | ||
|
|
@@ -554,7 +593,6 @@ private fun AutofillView.updateWebsiteIfNecessary(website: String?): AutofillVie | |
| is AutofillView.Identity.PostalAddressFull -> { | ||
| this.copy(data = this.data.copy(website = site)) | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. I think this is supposed to be here, right? |
||
| is AutofillView.Identity.PostalCode -> this.copy(data = this.data.copy(website = site)) | ||
| is AutofillView.Identity.Ssn -> this.copy(data = this.data.copy(website = site)) | ||
| is AutofillView.Unused -> this.copy(data = this.data.copy(website = site)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍