Pylons / deform

A Python HTML form library.
Other
416 stars 160 forks source link

Prevent browser from auto-filling CheckedPasswordWidget #515

Open JocelynDelalande opened 3 years ago

JocelynDelalande commented 3 years ago

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

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

stevepiercy commented 3 years ago

@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.

JocelynDelalande commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.