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

Avoid setting empty value on reset & submit inputs #12780

Merged
merged 4 commits into from Aug 16, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
79 changes: 75 additions & 4 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Expand Up @@ -752,15 +752,86 @@ describe('ReactDOMInput', () => {

it('should not set a value for submit buttons unnecessarily', () => {
const stub = <input type="submit" />;
const node = ReactDOM.render(stub, container);
ReactDOM.render(stub, container);
const node = container.firstChild;

// The value shouldn't be '', or else the button will have no text; it
// should have the default "Submit" or "Submit Query" label. Most browsers
// report this as not having a `value` attribute at all; IE reports it as
// the actual label that the user sees.
expect(
!node.hasAttribute('value') || node.getAttribute('value').length > 0,
).toBe(true);
expect(node.hasAttribute('value')).toBe(false);
});

it('should remove the value attribute on submit inputs when value is updated to undefined', () => {
const stub = <input type="submit" value="foo" onChange={emptyFunction} />;
ReactDOM.render(stub, container);

// Not really relevant to this particular test, but changing to undefined
// should nonetheless trigger a warning
expect(() =>
ReactDOM.render(
<input type="submit" value={undefined} onChange={emptyFunction} />,
container,
),
).toWarnDev(
'A component is changing a controlled input of type ' +
'submit to be uncontrolled.',
);

const node = container.firstChild;
expect(node.getAttribute('value')).toBe(null);
});

it('should remove the value attribute on reset inputs when value is updated to undefined', () => {
const stub = <input type="reset" value="foo" onChange={emptyFunction} />;
ReactDOM.render(stub, container);

// Not really relevant to this particular test, but changing to undefined
// should nonetheless trigger a warning
expect(() =>
ReactDOM.render(
<input type="reset" value={undefined} onChange={emptyFunction} />,
container,
),
).toWarnDev(
'A component is changing a controlled input of type ' +
'reset to be uncontrolled.',
);

const node = container.firstChild;
expect(node.getAttribute('value')).toBe(null);
});

it('should set a value on a submit input', () => {
let stub = <input type="submit" value="banana" />;
ReactDOM.render(stub, container);
const node = container.firstChild;

expect(node.getAttribute('value')).toBe('banana');
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for an intentionally empty value?

it('should set a value on a reset input', () => {
let stub = <input type="reset" value="banana" />;
ReactDOM.render(stub, container);
const node = container.firstChild;

expect(node.getAttribute('value')).toBe('banana');
});

it('should set an empty string value on a submit input', () => {
let stub = <input type="submit" value="" />;
ReactDOM.render(stub, container);
const node = container.firstChild;

expect(node.getAttribute('value')).toBe('');
});

it('should set an empty string value on a reset input', () => {
let stub = <input type="reset" value="" />;
ReactDOM.render(stub, container);
const node = container.firstChild;

expect(node.getAttribute('value')).toBe('');
});

it('should control radio buttons', () => {
Expand Down
18 changes: 17 additions & 1 deletion packages/react-dom/src/client/ReactDOMFiberInput.js
Expand Up @@ -172,9 +172,10 @@ export function updateWrapper(element: Element, props: Object) {
updateChecked(element, props);

const value = getToStringValue(props.value);
const type = props.type;

if (value != null) {
if (props.type === 'number') {
if (type === 'number') {
if (
(value === 0 && node.value === '') ||
// We explicitly want to coerce to number here if possible.
Expand All @@ -186,6 +187,11 @@ export function updateWrapper(element: Element, props: Object) {
} else if (node.value !== toString(value)) {
node.value = toString(value);
}
} else if (type === 'submit' || type === 'reset') {
// Submit/reset inputs need the attribute removed completely to avoid
// blank-text buttons.
node.removeAttribute('value');
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I remove this line none of the tests seem to fail. Please make sure the tests cover all lines.

return;
}

if (props.hasOwnProperty('value')) {
Expand All @@ -207,6 +213,16 @@ export function postMountWrapper(
const node = ((element: any): InputWithWrapperState);

if (props.hasOwnProperty('value') || props.hasOwnProperty('defaultValue')) {
// Avoid setting value attribute on submit/reset inputs as it overrides the
// default value provided by the browser. See: #12872
const type = props.type;
if (
(type === 'submit' || type === 'reset') &&
(props.value === undefined || props.value === null)
) {
return;
musakarakas marked this conversation as resolved.
Show resolved Hide resolved
}

const initialValue = toString(node._wrapperState.initialValue);
const currentValue = node.value;

Expand Down