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

Password inputs do not synchronize the value attribute #12722

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1433,6 +1433,40 @@ describe('ReactDOMInput', () => {
expect(node.getAttribute('value')).toBe('2');
});

it('does not set the value attribute on password inputs', () => {
const Input = getTestInput();
const stub = ReactTestUtils.renderIntoDocument(
<Input type="password" value="1" />,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note. Input here, as a React component, through me off for a bit. I wonder if we should rename this TestInput or ControlledInput.

);
const node = ReactDOM.findDOMNode(stub);

expect(node.hasAttribute('value')).toBe(false);
expect(node.value).toBe('1');
});

it('does not update the value attribute on password inputs', () => {
const Input = getTestInput();
const stub = ReactTestUtils.renderIntoDocument(
<Input type="password" value="1" />,
);
const node = ReactDOM.findDOMNode(stub);

ReactTestUtils.Simulate.change(node, {target: {value: '2'}});

expect(node.hasAttribute('value')).toBe(false);
expect(node.value).toBe('2');
});

it('does not set the defaultValue attribute on password inputs', () => {
const stub = ReactTestUtils.renderIntoDocument(
<input type="password" defaultValue="1" />,
);
const node = ReactDOM.findDOMNode(stub);

expect(node.hasAttribute('value')).toBe(false);
expect(node.value).toBe('1');
});

it('does not set the value attribute on number inputs if focused', () => {
const Input = getTestInput();
const stub = ReactTestUtils.renderIntoDocument(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,50 @@ describe('ReactDOMServerIntegration', () => {
expect(e.getAttribute('defaultValue')).toBe(null);
},
);

itClientRenders(
'a password input hydrates client-side with the value prop',
async render => {
const e = await render(
<input type="password" value="foo" readOnly={true} />,
0,
);

expect(e.value).toBe('foo');
expect(e.hasAttribute('value')).toBe(false);
},
);

itClientRenders(
'a password input hydrates client-side with the defaultValue prop',
async render => {
const e = await render(
<input type="password" defaultValue="foo" readOnly={true} />,
0,
);

expect(e.value).toBe('foo');
expect(e.hasAttribute('value')).toBe(false);
},
);

it('will not render the value prop server-side', async () => {
const e = await serverRender(
<input type="password" value="foo" readOnly={true} />,
);

expect(e.value).toBe('');
expect(e.hasAttribute('value')).toBe(false);
});

it('will not render the defaultValue prop server-side', async () => {
const e = await serverRender(
<input type="password" defaultValue="foo" readOnly={true} />,
);

expect(e.value).toBe('');
expect(e.hasAttribute('value')).toBe(false);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


describe('checkboxes', function() {
Expand Down Expand Up @@ -540,6 +584,20 @@ describe('ReactDOMServerIntegration', () => {
<input defaultValue="Hello" />,
));

it('should not blow away user-entered text on successful reconnect to an uncontrolled password input', () =>
testUserInteractionBeforeClientRender(
<input type="password" defaultValue="Hello" />,
'',
'Hello',
));

it('should not blow away user-entered text on successful reconnect to an controlled password input', () =>
testUserInteractionBeforeClientRender(
<input type="password" value="Hello" readOnly={true} />,
'',
'Hello',
));

it('should not blow away user-entered text on successful reconnect to a controlled input', async () => {
let changeCount = 0;
await testUserInteractionBeforeClientRender(
Expand Down
32 changes: 21 additions & 11 deletions packages/react-dom/src/client/ReactDOMFiberInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,9 @@ export function postMountWrapper(element: Element, props: Object) {
// value must be assigned before defaultValue. This fixes an issue where the
// visually displayed value of date inputs disappears on mobile Safari and Chrome:
// https://github.com/facebook/react/issues/7233
node.defaultValue = '' + node._wrapperState.initialValue;
if (props.type !== 'password') {
node.defaultValue = '' + node._wrapperState.initialValue;
}
}

// Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug
Expand Down Expand Up @@ -304,16 +306,24 @@ export function setDefaultValue(
type: ?string,
value: *,
) {
if (
// Focused number inputs synchronize on blur. See ChangeEventPlugin.js
type !== 'number' ||
node.ownerDocument.activeElement !== node
) {
if (value == null) {
node.defaultValue = '' + node._wrapperState.initialValue;
} else if (node.defaultValue !== '' + value) {
node.defaultValue = '' + value;
}
if (type === 'password') {
// Do not synchronize password inputs to prevent password from
// being exposed in markup
// https://github.com/facebook/react/issues/11896
return;
}

// Only assign the value attribute on number inputs when they are not selected
// This avoids edge cases in Chrome. Number inputs are synchronized on blur.
// See ChangeEventPlugin.js
if (type === 'number' && node.ownerDocument.activeElement === node) {
return;
}

if (value == null) {
node.defaultValue = '' + node._wrapperState.initialValue;
} else if (node.defaultValue !== '' + value) {
node.defaultValue = '' + value;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty blown out, but I wanted to make it painfully obvious that number inputs and passwords are unique.

}

Expand Down
8 changes: 8 additions & 0 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ function createOpenTagMarkup(
isRootElement: boolean,
): string {
let ret = '<' + tagVerbatim;
const isPasswordInput = tagLowercase === 'input' && props.type === 'password';

for (const propKey in props) {
if (!props.hasOwnProperty(propKey)) {
Expand All @@ -335,6 +336,13 @@ function createOpenTagMarkup(
if (propValue == null) {
continue;
}
// Do not render values for password inputs
if (
isPasswordInput &&
(propKey === 'value' || propKey === 'defaultValue')
) {
continue;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (propKey === STYLE) {
propValue = createMarkupForStyles(propValue);
}
Expand Down