Skip to content

[PM-36132] add tiktok login qualifiers#24

Open
audreyality wants to merge 9 commits into
mainfrom
autofill/pm-36132/tiktok-map
Open

[PM-36132] add tiktok login qualifiers#24
audreyality wants to merge 9 commits into
mainfrom
autofill/pm-36132/tiktok-map

Conversation

@audreyality
Copy link
Copy Markdown
Member

@audreyality audreyality commented Jun 2, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-36132

📔 Objective

Map TikTok's login fields.

@audreyality audreyality requested a review from a team as a code owner June 2, 2026 21:30
@audreyality audreyality requested a review from dan-livefront June 2, 2026 21:30
@audreyality audreyality changed the title add tiktok login qualifiers [PM-24656] add tiktok login qualifiers Jun 2, 2026
@audreyality audreyality changed the title [PM-24656] add tiktok login qualifiers [PM-36132] add tiktok login qualifiers Jun 2, 2026
@audreyality audreyality added the ai-review-vnext Request a Claude code review using the vNext workflow label Jun 2, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the addition of tiktok.com to maps/forms/forms.jsonc. The change introduces four mutually-exclusive form entries covering TikTok's phone-login, email/password-login, phone account-creation, and email account-creation flows, plus a dual-purpose email/username mapping for the shared identifier input on the login form. Selector choices, including the :has(input[name='email']) disambiguator for newPassword and the per-form repetition of the container prefix, are consistent with the conventions documented in maps/forms/README.md and with prior reviewer guidance in this PR's threads.

Code Review Details

No new findings beyond those already raised and addressed by the human reviewers:

  • The container-in-field-selector convention was discussed in an existing thread and the author's approach aligns with the README's "selector specificity should not rely on other selectors" guidance interpretation reached there.
  • Splitting the phone vs. email/password login forms into separate forms entries was addressed in f35acdd.
  • The dual-purpose email/username mapping on the login form was addressed in f52a5d4.
  • The form:has(input[name='email']) qualifier on newPassword is a precise solution for distinguishing the account-creation password input from the otherwise identical login password input.

Comment thread maps/forms/forms.jsonc Outdated
Comment on lines +138 to +139
"username": ["input[name='username']"],
"password": ["input[type='password']"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"username": ["input[name='username']"],
"password": ["input[type='password']"]
"username": ["div#loginContainer input[name='username']"],
"password": ["div#loginContainer input[type='password']"]

Copy link
Copy Markdown
Member Author

@audreyality audreyality Jun 3, 2026

Choose a reason for hiding this comment

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

It seems strange to me that those fields are not relative to "container". It's noted as such in the docs. The examples, however, are misleading.

PR to fix the examples

Addressed by 9570c0c

Copy link
Copy Markdown
Collaborator

@jprusik jprusik Jun 3, 2026

Choose a reason for hiding this comment

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

A consumer should be able to determine what value belongs in the field from the key name and form category alone. Selector specificity should not rely on other selectors (e.g. container selector).

The primary reason for this is that the container is optional, and also can exist without having the related inputs directly inside it (I'm thinking particularly of controlled form scenarios). If we allow the inputs to use the form selectors as bounding contexts, we both prescribe that a consumer must consider both selectors, and that inputs must be within a container if one is described (which, to be fair, is exceedingly common, despite not being guaranteed).

I think it would make sense to revisit this after some time working with the Forms map if we find it to be more onerous than flexible, but I'd like to advocate that we stick with the convention for now and see.

Copy link
Copy Markdown
Member Author

@audreyality audreyality Jun 3, 2026

Choose a reason for hiding this comment

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

If we allow the inputs to use the form selectors as bounding contexts, we both prescribe that a consumer must consider both selectors, and that inputs must be within a container if one is described (which, to be fair, is exceedingly common, despite not being guaranteed).

I think a bounding context is implied by the name "container". I wouldn't think that you mean "form" when you say "container".

("see edit history, I accidentally overwrote this comment when trying to add a new one" -@jprusik)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think a bounding context is implied by the name "container". I wouldn't think that you mean "form" when you say "container".

I agree with that; the challenge was disambiguating between "form" as a represented concept, <form> as a tag, and the visual grouping of related fields. The three categories often overlap, but drift from each other often enough that we needed a name for "the thing the fields are anchored to"; it quite possibly needs refinement.

Part of me wonders if it's useful to describe that at all.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

("see edit history, I accidentally overwrote this comment when trying to add a new one" -@jprusik)

😂 I very nearly did this to one of your comments, too, when I was adding an "addressed by $revision"

Comment thread maps/forms/forms.jsonc Outdated
Comment on lines +144 to +145
"button[data-e2e='login-button']",
"button[type='submit']"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Similar to the suggestion above, unless the buttons exist adjacent to the form container (which is sometimes the case), include the container selector to avoid matches against other buttons on the page (particularly the second one)

Suggested change
"button[data-e2e='login-button']",
"button[type='submit']"
"div#loginContainer button[data-e2e='login-button']",
"div#loginContainer button[type='submit']"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Addressed by 9570c0c

@dan-livefront
Copy link
Copy Markdown
Contributor

dan-livefront commented Jun 3, 2026

Screenshot 2026-06-03 at 11 04 04 AM

For the sign up flow on tiktok, we can add an account-creation that maps this input to phone.

Screenshot 2026-06-03 at 11 05 43 AM

For the login flow, would setting as username be sufficient since we can safely assume the user determined their login method before saving credentials or is this a path we should think of 🤔 @jprusik

@audreyality
Copy link
Copy Markdown
Member Author

audreyality commented Jun 3, 2026

📝 The phone number field has a way to distinguish it from a username and you can log in with a phone and username by clicking "use password" on the box. From a mapping perspective, I could make it a late-binding of the username field, and/or an early binding of a phone number field (as in, positioning the selector within an ordered list before/after its other matches).

I tried making this more specific in d9a65ab.

🤔 The thing I wonder about, since we don't want this tied specifically to Bitwarden, is whether we'd want to distinguish sub-types of phone numbers. This field specifically needs to be one that can receive text messages. In that respect, it could even be used as part of, say, a mobile TOTP that expects you to enter the last-4 of your mobile number or some such thing. MtW isn't currently set up to handle this level of specificity.

❓ Another thing that I'm interested in is the email field I added in the "new-account-creation" record. It contains an email, and then you can use an email or a username to log in. Does that mean that the "new-account" form should have a "username" selector and an "email" selector pointing to the same field?

Comment thread maps/forms/forms.jsonc Outdated
Comment thread maps/forms/forms.jsonc Outdated
Comment on lines +137 to +139
"phone": ["div#loginContainer form input[name='mobile']"],
"username": ["div#loginContainer form input[name='username']"],
"password": ["div#loginContainer form input[type='password']"]
Copy link
Copy Markdown
Collaborator

@jprusik jprusik Jun 3, 2026

Choose a reason for hiding this comment

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

username + password is a mutually exclusive login form to phone, so I think these need to be split to their own form entries for this host

(The sms send/entry in the phone login is arguably a form within a form, which we don't have a way to represent in the map, so I think we can set it to the side, for the moment)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Addressed in f35acdd.

Comment thread maps/forms/forms.jsonc
Comment thread maps/forms/forms.jsonc
"email": ["div#loginContainer form input[name='email']"],
// field only distinguishable from `account-login` because the email field is present instead of `username`
"newPassword": [
"div#loginContainer form:has(input[name='email']) input[type='password']"
Copy link
Copy Markdown
Collaborator

@jprusik jprusik Jun 3, 2026

Choose a reason for hiding this comment

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

form:has(input[name='email'])

love this; VERY nice fix for the problem

Copy link
Copy Markdown
Collaborator

@jprusik jprusik Jun 3, 2026

Choose a reason for hiding this comment

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

non-blocking: shall we use this targeting technique in the other selectors to better distinguish between the otherwise identically-targeted-but-still-independent forms?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think that's necessary. The main reason why the newPassword field uses that selector is because there's an identical password input between account creation and login. Getting to that flow is buried in the TikTok UI, so I gave it that highly-specific selector.

jprusik
jprusik previously approved these changes Jun 3, 2026
@jprusik
Copy link
Copy Markdown
Collaborator

jprusik commented Jun 3, 2026

🤔 The thing I wonder about, since we don't want this tied specifically to Bitwarden, is whether we'd want to distinguish sub-types of phone numbers. This field specifically needs to be one that can receive text messages. In that respect, it could even be used as part of, say, a mobile TOTP that expects you to enter the last-4 of your mobile number or some such thing. MtW isn't currently set up to handle this level of specificity.

I think the Forms Map would presently describe it as a phone field belonging to a form of the account-login category, but yea, we might consider adding something like a messagingProviderNumber field concept, since texting capabilities of phones is presumed in these scenarios.

❓ Another thing that I'm interested in is the email field I added in the "new-account-creation" record. It contains an email, and then you can use an email or a username to log in. Does that mean that the "new-account" form should have a "username" selector and an "email" selector pointing to the same field?

Yes, since it's a single, dual-purpose field

@jprusik
Copy link
Copy Markdown
Collaborator

jprusik commented Jun 3, 2026

For the login flow, would setting as username be sufficient since we can safely assume the user determined their login method before saving credentials or is this a path we should think of 🤔

Yeah, we'll have to think about this in our browser client implementation. I think the signal from this Map is fairly unambiguous though (via the form category context).

@audreyality audreyality force-pushed the autofill/pm-36132/tiktok-map branch from 4e136ca to f52a5d4 Compare June 3, 2026 21:32
Comment thread maps/forms/forms.jsonc Outdated
Comment on lines +152 to +151
"username": ["div#loginContainer form input[name='username']"],
"password": ["div#loginContainer form input[type='password']"]
"email": ["div#loginContainer form input[name='username']"],
"password": ["div#loginContainer form input[type='password']"],
"username": ["div#loginContainer form input[name='username']"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Now addressed in f52a5d4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants