-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 autocomplete attrs to password/username fields #5143
base: main
Are you sure you want to change the base?
Add autocomplete attrs to password/username fields #5143
Conversation
Which browsers emit warnings here? |
I think all Chromium browsers (Google Chrome, Microsoft Edge) do this: https://www.chromium.org/developers/design-documents/create-amazing-password-forms/ |
What about this PR? Do we want to add the autocomplete attributes on the applicable inputs? |
@@ -8,7 +8,7 @@ defmodule <%= inspect context.web_module %>.<%= inspect Module.concat(schema.web | |||
<.header>Resend confirmation instructions</.header> | |||
|
|||
<.simple_form :let={f} for={:<%= schema.singular %>} id="resend_confirmation_form" phx-submit="send_instructions"> | |||
<.input field={{f, :email}} type="email" label="Email" required /> | |||
<.input field={{f, :email}} type="email" label="Email" autocomplete="username" required /> |
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.
Is there a reason why you are using autocomplete="username"
rather than autocomplete="email"
on the email fields?
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.
Not really, I think you're right and we should update this to autocomplete="email"
. I'm happy to do so and also bring this PR in sync with the master branch?
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.
That couldn't hurt. I'm not a maintainer myself, but I'd be keen to see it merged.
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.
Not really, I think you're right and we should update this to
autocomplete="email"
. I'm happy to do so and also bring this PR in sync with the master branch?
autocomplete="username" with type=“email” is correct as those are the attributes of the same field in other templates (e.g. registration). So with the current value the browser is advised to use the same value the user as entered before.
HTML spec allows the combination of type=“email” and autocomplete=username”.
https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#control-group-username
Chromium advices the combi:
https://www.chromium.org/developers/design-documents/form-styles-that-chromium-understands/
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.
Thanks @BartOtten. I'll bring this PR up to date with master later this week.
Can you resolve conflicts? Note you will also need to ensure the update files are formatted properly as they likely will be broken into new lines. You can |
Yes, will do so in the beginning of next week! |
1c38720
to
6ba6a4b
Compare
Done! |
First of all, congrats with the great 0.20 release, @chrismccord! Do you think this PR is mergeable or do we mis something? |
Some browsers give console warnings if the
autocomplete
attribute is missing on username/password fields.