Allow hidden_field(_tag) to accept a custom autocomplete value#53512
Allow hidden_field(_tag) to accept a custom autocomplete value#53512byroot merged 1 commit intorails:mainfrom
Conversation
| # value="" onchange="alert('Input collected!')" autocomplete="off" /> | ||
| def hidden_field_tag(name, value = nil, options = {}) | ||
| text_field_tag(name, value, options.merge(type: :hidden, autocomplete: "off")) | ||
| text_field_tag(name, value, options.merge(type: :hidden).reverse_merge(autocomplete: "off")) |
There was a problem hiding this comment.
Since the preceding options.merge call will create a new Hash instance, would it make sense to invoke the ! variant?
| text_field_tag(name, value, options.merge(type: :hidden).reverse_merge(autocomplete: "off")) | |
| text_field_tag(name, value, options.merge(type: :hidden).reverse_merge!(autocomplete: "off")) |
There was a problem hiding this comment.
Could the Hash#with_defaults! alias work instead?
| text_field_tag(name, value, options.merge(type: :hidden).reverse_merge(autocomplete: "off")) | |
| text_field_tag(name, value, options.merge(type: :hidden).with_defaults!(autocomplete: "off")) |
There was a problem hiding this comment.
Interesting. I didn't know about that alias. It does read better :)
Is there a reason not to use merge! also?
|
@seanpdoyle, did you need me to do anything else on this PR? |
|
@brendon as far as the code is concerned, I think this is ready. Like you mentioned in the PR description, setting @zzak as the author of #47798, what do you think the next steps should be? |
Thanks @seanpdoyle, I don't see that there's anything stopping this from being merged in regardless. It doesn't break existing behaviour, it allows one to set a custom In saying that, there's probably room for another PR that adds a configuration option to disable the |
rails#43280 introduced an enforced `autocomplete="off"` to all hidden inputs generated by Rails to fix a [firefox bug](https://bugzilla.mozilla.org/show_bug.cgi?id=520561). Unfortunately it's also a legitimate use-case to specify an `autocomplete` with a value such as `username` and a value on a hidden input. This hints to the browser that (in this example) the username of a password reset form is what we've provided as the value and the password manager can store it as such. This commit only sets `autocomplete="off"` if another `autocomplete` value isn't provided. Supersedes rails#47798 which didn't seem to be as elegant a solution and didn't have proper tests.
ccb06bb to
09daec6
Compare
|
Just wondering why this fix hasn't been released yet? It doesn't break any existing behaviour :) |
#43280 introduced an enforced
autocomplete="off"to all hidden inputs generated by Rails to fix a firefox bug.Unfortunately it's also a legitimate use-case to specify an
autocompletewith a value such asusernameand a value on a hidden input. This hints to the browser that (in this example) the username of a password reset form is what we've provided as the value and the password manager can store it as such.This commit only sets
autocomplete="off"if anotherautocompletevalue isn't provided.Supersedes #47798 which didn't seem to be as elegant a solution and didn't have proper tests.