Password inputs do not synchronize the value attribute #12722
Conversation
| it('does not set the value attribute on password inputs', () => { | ||
| const Input = getTestInput(); | ||
| const stub = ReactTestUtils.renderIntoDocument( | ||
| <Input type="password" value="1" />, |
There was a problem hiding this comment.
Side note. Input here, as a React component, through me off for a bit. I wonder if we should rename this TestInput or ControlledInput.
|
|
||
| expect(e.value).toBe(''); | ||
| expect(e.hasAttribute('value')).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I had to split these out. Markup straight from the server never assigns the value attribute, so the related input won't have a value. It is eventually assigned during hydration, as illustrated in the itClientRenders tests.
Also, this should still respect value property modifications by a user in cases where hydration stalls and executes after a user has given input..
| if (value == null) { | ||
| node.defaultValue = '' + node._wrapperState.initialValue; | ||
| } else if (node.defaultValue !== '' + value) { | ||
| node.defaultValue = '' + value; | ||
| } |
There was a problem hiding this comment.
This is pretty blown out, but I wanted to make it painfully obvious that number inputs and passwords are unique.
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: 7dd4ca2...e190e81 react-dom
Generated by 🚫 dangerJS |
| (propKey === 'value' || propKey === 'defaultValue') | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I don't feel great about this, is there an earlier place I can sift out the value attribute for password inputs?
I think we need to filter out the value attribute on password inputs anyway, even if we eliminate value attribute syncing altogether. The value attribute for most input will still be sent down.
nhunzaker
force-pushed
the
nh-password-inputs-no-sync
branch
from
2161f6b
to
9619e81
Compare
In order to prevent passwords from showing up in the markup React generates, this commit adds exceptions for password inputs such that defaultValue synchronization is omitted. When rendered server-side, password inputs will not send markup down from the server, however the value attribute is restored upon hydration. This is probably a design decision that we should clamp down.
nhunzaker
force-pushed
the
nh-password-inputs-no-sync
branch
from
9619e81
to
e190e81
Compare
|
Did we decide on special-casing Also, how does this affect |
|
@aweary That was what I gathered from Sebastian's comment: #11896 (comment), but I don't know if we ever gathered consensus on the timeline.
I do not know about present behavior, but my opinion is that value attributes shouldn't appear for password inputs, even when generating static markup. Maybe we should take this to an RFC. |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Closing this out. It should be covered in #13526. |
On hold until 17.0.0
In order to prevent passwords from showing up in the markup React generates, this commit adds exceptions for password inputs such that
defaultValuesynchronization is omitted.When rendered server-side, password inputs no longer render the value attribute markup, however the value attribute is restored upon hydration. This is probably a design decision that we should clamp down, and something we'll need to respect if we remove value attribute syncing generally.
This should fix #11896, as it pertains to password inputs.