[PM-41293] feat: Add identity field-key mapping to fill-assist #7342
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,27 +16,49 @@ private const val FIELD_KEY_CARD_EXPIRATION_MONTH = "cardExpirationMonth" | |
| private const val FIELD_KEY_CARD_EXPIRATION_YEAR = "cardExpirationYear" | ||
| private const val FIELD_KEY_CARD_CVV = "cardCvv" | ||
| private const val FIELD_KEY_CARD_TYPE = "cardType" | ||
| private const val FIELD_KEY_PERSON_NAME_FULL = "fullName" | ||
| private const val FIELD_KEY_PERSON_NAME_PREFIX = "honorificPrefix" | ||
| private const val FIELD_KEY_PERSON_NAME_GIVEN = "firstName" | ||
| private const val FIELD_KEY_PERSON_NAME_MIDDLE = "middleName" | ||
| private const val FIELD_KEY_PERSON_NAME_FAMILY = "lastName" | ||
| private const val FIELD_KEY_ADDRESS_STREET = "addressLine1" | ||
| private const val FIELD_KEY_ADDRESS_LOCALITY = "addressLevel2" | ||
| private const val FIELD_KEY_ADDRESS_REGION = "addressLevel1" | ||
| private const val FIELD_KEY_ADDRESS_COUNTRY = "country" | ||
| private const val FIELD_KEY_POSTAL_CODE = "postalCode" | ||
| private const val FIELD_KEY_COMPANY = "organization" | ||
| private const val FIELD_KEY_SSN = "ssn" | ||
| private const val FIELD_KEY_PASSPORT_NUMBER = "passportNumber" | ||
| private const val FIELD_KEY_LICENSE_NUMBER = "licenseNumber" | ||
|
|
||
| /** | ||
| * Traverses the [AssistStructure] and returns a list of [AutofillView]s classified by the | ||
| * provided [hostRules]. Only view nodes whose [android.view.ViewStructure.HtmlInfo] attributes | ||
| * match a [FillAssistRules.SelectorClause] are included; unmatched nodes are omitted (no | ||
| * heuristic fallback). All identity classification is gated behind [isIdentityAutofillEnabled]. | ||
| */ | ||
| internal fun AssistStructure.buildFillAssistViews( | ||
| hostRules: List<FillAssistRules.HostRule>, | ||
| urlBarWebsite: String?, | ||
| isIdentityAutofillEnabled: Boolean, | ||
| ): List<AutofillView> = | ||
| (0 until windowNodeCount) | ||
| .mapNotNull { getWindowNodeAt(it).rootViewNode } | ||
| .flatMap { | ||
| it.traverseForFillAssist( | ||
| hostRules = hostRules, | ||
| parentWebsite = urlBarWebsite, | ||
| isIdentityAutofillEnabled = isIdentityAutofillEnabled, | ||
| ) | ||
| } | ||
|
|
||
| private fun AssistStructure.ViewNode.traverseForFillAssist( | ||
| hostRules: List<FillAssistRules.HostRule>, | ||
| parentWebsite: String?, | ||
| isIdentityAutofillEnabled: Boolean, | ||
| ): List<AutofillView> { | ||
| val website = this.website ?: parentWebsite | ||
| val ownViews = autofillId?.let { id -> | ||
| hostRules | ||
| .flatMap { it.fields.entries } | ||
| .filter { (_, alternatives) -> | ||
|
|
@@ -48,45 +70,108 @@ private fun AssistStructure.ViewNode.traverseForFillAssist( | |
| ?.let { matchingEntries -> | ||
| val data = toAutofillViewData(autofillId = id, website = website) | ||
| val candidateViews = matchingEntries.mapNotNull { (key, _) -> | ||
| key.toAutofillViewForFieldKey( | ||
| data = data, | ||
| isIdentityAutofillEnabled = isIdentityAutofillEnabled, | ||
| )?.let { key to it } | ||
| } | ||
| // Prefer Username: it has no format gate, while Login.Email rejects non-email | ||
| // values via isValidEmail(). Prefer any non-Identity view next, since Identity | ||
| // partitions are unbuilt and Login/Card must remain authoritative while the | ||
| // flag is on. | ||
| val view = candidateViews | ||
| .firstOrNull { (_, view) -> view is AutofillView.Login.Username } | ||
| ?.second | ||
| ?: candidateViews.firstOrNull { (_, view) -> view !is AutofillView.Identity } | ||
| ?.second | ||
| ?: candidateViews.firstOrNull()?.second | ||
| ?: return@let null | ||
|
Comment on lines
+82
to
+88
Contributor
There was a problem hiding this comment. Details and fix
Traceable path (flag on):
With the flag off,
val view = candidateViews
.firstOrNull { (_, view) -> view is AutofillView.Login.Username }
?.second
?: candidateViews.firstOrNull { (_, view) -> view !is AutofillView.Identity }?.second
?: candidateViews.firstOrNull()?.second
?: return@let nullA test with a node matched under both an identity key (listed first) and
Contributor
There was a problem hiding this comment. Claude finished @aj-rosado's task in 1m 6s —— View job Fix verified ✅
The tie-break in val view = candidateViews
.firstOrNull { (_, view) -> view is AutofillView.Login.Username }
?.second
?: candidateViews.firstOrNull { (_, view) -> view !is AutofillView.Identity }
?.second
?: candidateViews.firstOrNull()?.second
?: return@let nullTracing the path I reported — node matched under No regressions I can find:
One nit (non-blocking, Phase D)An identity classification can now be silently dropped when the same node also matches a login/card key that isn't in the dual-classify set. E.g. a node matched under
|
||
|
|
||
| // Dual-classify off the full matched-key set, not just the winning key, so a | ||
| // field matched under both "email" and "phone" gets both Identity views. | ||
| val isLoginIdentifierView = view is AutofillView.Login.Username || | ||
| view is AutofillView.Login.Email | ||
| buildList<AutofillView> { | ||
| add(view) | ||
| if (isIdentityAutofillEnabled && isLoginIdentifierView) { | ||
| val matchedKeys = candidateViews.mapTo(mutableSetOf()) { it.first } | ||
| if (FIELD_KEY_EMAIL in matchedKeys) { | ||
| add(AutofillView.Identity.Email(data = view.data)) | ||
| } | ||
| if (FIELD_KEY_PHONE in matchedKeys) { | ||
| add(AutofillView.Identity.PhoneFull(data = view.data)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }.orEmpty() | ||
| val childViews = (0 until childCount) | ||
| .flatMap { index -> | ||
| getChildAt(index).traverseForFillAssist( | ||
| hostRules = hostRules, | ||
| parentWebsite = website, | ||
| isIdentityAutofillEnabled = isIdentityAutofillEnabled, | ||
| ) | ||
| } | ||
| return ownViews + childViews | ||
| } | ||
|
|
||
| /** | ||
| * Maps this field key to the [AutofillView] it represents, or null if this key is unrecognized. | ||
| * Delegates to a type-specific mapper ([toLoginViewForFieldKey], [toCardViewForFieldKey], | ||
| * [toIdentityViewForFieldKey]) grouped by the category the field key belongs to. | ||
| */ | ||
| private fun String.toAutofillViewForFieldKey( | ||
| data: AutofillView.Data, | ||
| isIdentityAutofillEnabled: Boolean, | ||
| ): AutofillView? = | ||
| toLoginViewForFieldKey(data = data) | ||
| ?: toCardViewForFieldKey(data = data) | ||
| ?: if (isIdentityAutofillEnabled) toIdentityViewForFieldKey(data = data) else null | ||
|
|
||
| private fun String.toLoginViewForFieldKey(data: AutofillView.Data): AutofillView.Login? = | ||
| when (this) { | ||
| FIELD_KEY_USERNAME, FIELD_KEY_PHONE -> AutofillView.Login.Username(data = data) | ||
| FIELD_KEY_EMAIL -> AutofillView.Login.Email(data = data) | ||
| FIELD_KEY_PASSWORD, FIELD_KEY_NEW_PASSWORD -> AutofillView.Login.Password(data = data) | ||
| else -> null | ||
| } | ||
|
|
||
| private fun String.toCardViewForFieldKey(data: AutofillView.Data): AutofillView.Card? = | ||
| when (this) { | ||
| FIELD_KEY_CARD_NUMBER -> AutofillView.Card.Number(data = data) | ||
| FIELD_KEY_CARDHOLDER_NAME -> AutofillView.Card.CardholderName(data = data) | ||
| FIELD_KEY_CARD_EXPIRATION_DATE -> AutofillView.Card.ExpirationDate(data = data) | ||
| FIELD_KEY_CARD_EXPIRATION_MONTH -> AutofillView.Card.ExpirationMonth( | ||
| data = data, | ||
| monthValue = null, | ||
| ) | ||
|
|
||
| FIELD_KEY_CARD_EXPIRATION_YEAR -> AutofillView.Card.ExpirationYear( | ||
| data = data, | ||
| yearValue = null, | ||
| ) | ||
|
|
||
| FIELD_KEY_CARD_CVV -> AutofillView.Card.SecurityCode(data = data) | ||
| FIELD_KEY_CARD_TYPE -> AutofillView.Card.Brand(data = data, brandValue = null) | ||
| else -> null | ||
| } | ||
|
|
||
| private fun String.toIdentityViewForFieldKey(data: AutofillView.Data): AutofillView.Identity? = | ||
| when (this) { | ||
| FIELD_KEY_PERSON_NAME_FULL -> AutofillView.Identity.PersonNameFull(data = data) | ||
| FIELD_KEY_PERSON_NAME_PREFIX -> AutofillView.Identity.PersonNamePrefix(data = data) | ||
| FIELD_KEY_PERSON_NAME_GIVEN -> AutofillView.Identity.PersonNameGiven(data = data) | ||
| FIELD_KEY_PERSON_NAME_MIDDLE -> AutofillView.Identity.PersonNameMiddle(data = data) | ||
| FIELD_KEY_PERSON_NAME_FAMILY -> AutofillView.Identity.PersonNameFamily(data = data) | ||
| FIELD_KEY_ADDRESS_STREET -> AutofillView.Identity.AddressStreet(data = data) | ||
| FIELD_KEY_ADDRESS_LOCALITY -> AutofillView.Identity.AddressLocality(data = data) | ||
| FIELD_KEY_ADDRESS_REGION -> AutofillView.Identity.AddressRegion(data = data) | ||
| FIELD_KEY_ADDRESS_COUNTRY -> AutofillView.Identity.AddressCountry(data = data) | ||
| FIELD_KEY_POSTAL_CODE -> AutofillView.Identity.PostalCode(data = data) | ||
| FIELD_KEY_COMPANY -> AutofillView.Identity.Company(data = data) | ||
| FIELD_KEY_SSN -> AutofillView.Identity.Ssn(data = data) | ||
| FIELD_KEY_PASSPORT_NUMBER -> AutofillView.Identity.PassportNumber(data = data) | ||
| FIELD_KEY_LICENSE_NUMBER -> AutofillView.Identity.LicenseNumber(data = data) | ||
| else -> null | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -270,5 +270,4 @@ private val SUPPORTED_HTML_ATTRIBUTE_HINTS: List<String> = listOf( | |
| "type", | ||
| "hint", | ||
| "autofill", | ||
| ) | ||
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.
Unfillable, unlike the heuristic path.Details and fix
toEffectiveViewsreturnsfillAssistViewsunfiltered, andbuildFillAssistViewsis given all of the host's rules (including identity-category ones), so an identity-keyed node is now classified even whencoversCurrentPartitionmatched on a login/card rule.parseInternalthen does:Traceable path (flag on, fill-assist on, host has
account-loginrules plus identity field keys):Identity(orUnused), so it is not inautofillViews;focusedViewfalls back toautofillViews.firstOrNull()— an unfocusedLogin.Username.coversCurrentPartitionis true (Login rule present), so fill-assist runs.fillAssistViewsnow containsIdentity.PersonNameFull(isFocused = true)alongsideLogin.Username/Login.Password.firstFocusedOrNull()returns the Identity view →Unfillable.Before this change the identity key mapped to
null, the node was omitted, andfirstFocusedOrNull()fell back to the first Login view → a fillable Login partition. So turning the flag on removes login suggestions on these hosts.This is the exact case the heuristic path guards against at line 141-144 (
it !is AutofillView.Unused && it !is AutofillView.Identity), covered byparse should choose AutofillPartition Login when an Identity view is focused but a Login view is fillable elsewhere. The fill-assist path has no equivalent.Mirroring that filter until Phase D would keep the two paths aligned, e.g. in
parseInternal:A parser-level test with a focused identity-keyed fill-assist node would lock this in.
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.
@claude fixed
Uh oh!
There was an error while loading. Please reload this page.
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.
Claude finished @aj-rosado's task in 1m 18s —— View job
Fix verified ✅
e75acee"Filter out autofill identity")Unfillablepath is closedThe fix is correct and complete.
AutofillParserImpl.kt:186-189now filtersIdentityout before resolving the effective focused view:Tracing the path I reported: with the flag on, the focused identity-keyed node is now skipped,
firstFocusedOrNull()'s?: firstOrNull()fallback (AutofillParserImpl.kt:337-338) lands on the fill-assistLogin.Username, and the request stays a fillable Login partition. The two paths are aligned — this mirrors the heuristic filter atAutofillParserImpl.kt:141-144.No regressions I can see from the filter itself:
autofillViewsalready excludesIdentity, sofilterNotis a no-op.Unfillable, same as before.The new test
parse should choose AutofillPartition Login when fill-assist resolves the focused view to Identity but a Login view is fillable elsewhere(AutofillParserTests.kt:1789+) exercises exactly the scenario — focusedUnusednode reclassified toIdentity.PersonNameFullby a rule that also carries ausernamekey — and asserts the Login partition rather thanUnfillable. It would fail without thefilterNot. Fixtures all check out against the existing test setup.One nit (non-blocking)
AutofillParserImpl.kt:205-208— theis AutofillView.Identity -> return AutofillRequest.Unfillablebranch is now unreachable, sinceIdentitycan no longer reacheffectiveFocusedView. The branch still has to exist forwhenexhaustiveness, but the comment now reads as if it's a live path. Consider matching the tone of the adjacentUnusedbranch:Fix this →
./gradlew :app:testStandardDebugUnitTestin this job — the Gradle command was blocked by the tool permission gate — so my verification is by code reading only. CI test results are authoritative.•
PM-41293/identity-autofill-fill-assist-field-mapping