Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add note about typical autocomplete combos for conditional UI #1992

Merged
merged 5 commits into from
Nov 29, 2023

Conversation

MasterKale
Copy link
Contributor

@MasterKale MasterKale commented Nov 1, 2023

This PR includes a new note detailing two simple examples of "correct" autocomplete attributes that should reliably trigger conditional UI across most browsers.

Fixes #1982.


Preview | Diff

@MasterKale MasterKale changed the title Add note about typical autocomplete combos Add note about typical autocomplete combos for conditional UI Nov 1, 2023
@MasterKale
Copy link
Contributor Author

I'm not a huge fan of how the new Note renders, but I couldn't get it to indent one level closer to the left without tripping CI's HTML validator which complained that I tried to put a <div> inside a <dt>. The way it renders now CI is happy since the <div> is inside the accompanying <dd> that contains the subsequent logic block, "If silentlyDiscoveredCredentials is not empty":

Screenshot 2023-10-31 at 10 17 31 PM

Copy link
Member

@timcappalli timcappalli left a comment

Choose a reason for hiding this comment

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

LGTM

@arianvp
Copy link

arianvp commented Nov 1, 2023

drive-by question: Why does the webauthn token need to be at the end? This sounds like something many people will get wrong and I think most people don't read w3c specs but e.g. MDN.

I'm happy the note is there. I just wonder why it's needed in the first place

index.bs Outdated Show resolved Hide resolved
@nsatragno
Copy link
Member

drive-by question: Why does the webauthn token need to be at the end? This sounds like something many people will get wrong and I think most people don't read w3c specs but e.g. MDN.

I'm happy the note is there. I just wonder why it's needed in the first place

The autofill attribute has a grammar with a strict order that parses right-to-left [1]. webauthn fit naturally with the existing process model at the end.

[1] https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#autofill

@emlun emlun self-requested a review November 15, 2023 20:14
Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

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

Thank you! Approved.

Comment on lines +2418 to +2419
- `"username webauthn"`
- `"current-password webauthn"`
Copy link
Member

Choose a reason for hiding this comment

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

I would add autofill= to make the examples that tiny bit clearer, but this is fine as is too.

Suggested change
- `"username webauthn"`
- `"current-password webauthn"`
- `autofill="username webauthn"`
- `autofill="current-password webauthn"`

@emlun
Copy link
Member

emlun commented Nov 16, 2023

If I understood the HTML spec right it seems like autofill="webauthn" alone should also work (this will set the non-autofill credential type since it jumps to the Done step), is that right @nsatragno? If so, should we point that out too?

@nsatragno
Copy link
Member

Yes, that should work too. Either pointing it out or not sounds good to me.

@MasterKale
Copy link
Contributor Author

If I understood the HTML spec right it seems like autofill="webauthn" alone should also work (this will set the non-autofill credential type since it jumps to the Done step), is that right @nsatragno? If so, should we point that out too?

I think it'd be better to encourage here more use of proper autofill tokens so I'm inclined to not call this otherwise valid combination out...

@emlun
Copy link
Member

emlun commented Nov 16, 2023

Sounds fair. 🙂

@nadalin nadalin added this to the L3-WD-02 milestone Nov 29, 2023
@nicksteele nicksteele merged commit 2c63082 into main Nov 29, 2023
1 check passed
github-actions bot added a commit that referenced this pull request Nov 29, 2023
SHA: 2c63082
Reason: push, by nicksteele

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@emlun emlun deleted the mm/conditional-ui-autofill-token-order branch November 30, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spec is not specific enough about order of conditional UI autofill tokens
7 participants