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 autocomplete attrs to password/username fields #5143

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

janwillemvd
Copy link
Contributor

@janwillemvd janwillemvd commented Dec 2, 2022

Some browsers give console warnings if the autocomplete attribute is missing on username/password fields.

@mcrumm
Copy link
Member

mcrumm commented Dec 2, 2022

Which browsers emit warnings here?

@janwillemvd
Copy link
Contributor Author

janwillemvd commented Dec 2, 2022

I think all Chromium browsers (Google Chrome, Microsoft Edge) do this:

Screenshot 2022-12-02 at 16 19 05

Screenshot 2022-12-02 at 16 22 02

https://www.chromium.org/developers/design-documents/create-amazing-password-forms/

@janwillemvd
Copy link
Contributor Author

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 />
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

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/

Copy link
Contributor Author

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.

@chrismccord
Copy link
Member

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 cd integration and mix test there to find out. Thanks!

@janwillemvd
Copy link
Contributor Author

Can you resolve conflicts?

Yes, will do so in the beginning of next week!

@janwillemvd janwillemvd force-pushed the add-autocomplete-attrs-for-inputs branch from 1c38720 to 6ba6a4b Compare August 7, 2023 13:47
@janwillemvd
Copy link
Contributor Author

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 cd integration and mix test there to find out. Thanks!

Done!

@janwillemvd
Copy link
Contributor Author

First of all, congrats with the great 0.20 release, @chrismccord!

Do you think this PR is mergeable or do we mis something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants