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

Prevent browser from auto-filling CheckedPasswordWidget #515

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JocelynDelalande
Copy link

Browser support is incomplete, but better than nothing :).

See https://caniuse.com/mdn-html_global_attributes_autocomplete_new-password

@stevepiercy
Copy link
Member

@JocelynDelalande thank you for your PR. However there already exists a method to add arbitrary HTML attributes to widgets. Additionally developers should be allowed to opt-in to this attribute, but making it mandatory would eliminate that option.

Please let me know if the current method does not satisfy your use case.

@stevepiercy stevepiercy closed this May 9, 2021
@JocelynDelalande
Copy link
Author

@stevepiercy Thanks for your input.

You are right, I can handle it via this way. Thanks for pointing me to this part of the doc, I adopted it in my project :).

But still, IMHO this is a very sane default for the CheckedPasswordWidget (not having it in a browser with pw autocomplete leads to frequent frustration/errors).

(I think this discussion has an interest, but I am no deform insider, and will not advocate indefinitely on this, given that I don't know enough deform design/practices).

@stevepiercy
Copy link
Member

All Pylons Project projects attempt to not break existing developer implementations with backward incompatible feature additions.

There is a difference between a mandatory value and a default value. This PR sets a mandatory value for autocomplete with no option for the developer to choose any other value or override it. This could break existing applications. I think that new-password would be a sane default for this widget, but not mandatory. Does that make sense?

I would accept a modification to this PR that:

@JocelynDelalande
Copy link
Author

There is a difference between a mandatory value and a default value. This PR sets a mandatory value for autocomplete with no option for the developer to choose any other value or override it. This could break existing applications. I think that new-password would be a sane default for this widget, but not mandatory. Does that make sense?

Perfectly :).

I would accept a modification to this PR that:

* Sets `autocomplete="new-password"` as the default, with an option for the developer to override it. This is easier said than done, as it would need to check for the presence of `autocomplete` in the `attributes` argument.
* Includes an assertion in the checkedPasswordWidget test for `autocomplete="new-password"` in deformdemo.

OK ; I'll do that.

* Includes your signature on https://github.com/Pylons/deform/blob/main/CONTRIBUTORS.txt if you have not already signed it.

* Includes a change note in https://github.com/Pylons/deform/blob/main/CHANGES.txt

Those two I did them in current PR (Was it the right maneer to do it ?).

Thanks again for taking time to discuss/welcome contribution. Appreciated :).

@stevepiercy
Copy link
Member

Those two I did them in current PR (Was it the right maneer to do it ?).

Yes, although the change will probably need to be updated to reflect your upcoming change.

Thanks again for taking time to discuss/welcome contribution. Appreciated :).

Thank you for persisting. You made me think about how to meet everyone's use case without breaking existing forms.

I've reopened the PR.

@stevepiercy stevepiercy reopened this May 11, 2021
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.

2 participants