From 2fa41d5bf1e810950b0ec927e59b309a19cb13e1 Mon Sep 17 00:00:00 2001 From: Ellis Clayton Date: Fri, 11 May 2018 10:38:51 +1000 Subject: [PATCH 1/3] Avoid setting empty value on reset & submit inputs --- .../src/__tests__/ReactDOMInput-test.js | 79 ++++++++++++++++++- .../src/client/ReactDOMFiberInput.js | 17 +++- 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 5afa9dcf7aa..f0b0f5df7a4 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -752,15 +752,86 @@ describe('ReactDOMInput', () => { it('should not set a value for submit buttons unnecessarily', () => { const stub = ; - 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 = ; + ReactDOM.render(stub, container); + + // Not really relevant to this particular test, but changing to undefined + // should nonetheless trigger a warning + expect(() => + ReactDOM.render( + , + 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 = ; + ReactDOM.render(stub, container); + + // Not really relevant to this particular test, but changing to undefined + // should nonetheless trigger a warning + expect(() => + ReactDOM.render( + , + 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 = ; + ReactDOM.render(stub, container); + const node = container.firstChild; + + expect(node.getAttribute('value')).toBe('banana'); + }); + + it('should set a value on a reset input', () => { + let stub = ; + 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 = ; + 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 = ; + ReactDOM.render(stub, container); + const node = container.firstChild; + + expect(node.getAttribute('value')).toBe(''); }); it('should control radio buttons', () => { diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index 8547aa50c1f..d856f8105ee 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -169,9 +169,10 @@ export function updateWrapper(element: Element, props: Object) { updateChecked(element, props); const value = getSafeValue(props.value); + const type = props.type; if (value != null) { - if (props.type === 'number') { + if (type === 'number') { if ( (value === 0 && node.value === '') || // eslint-disable-next-line @@ -182,6 +183,11 @@ export function updateWrapper(element: Element, props: Object) { } else if (node.value !== '' + value) { node.value = '' + value; } + } else if (type === 'submit' || type === 'reset') { + // Submit/reset inputs need the attribute removed completely to avoid + // blank-text buttons. + node.removeAttribute('value'); + return; } if (props.hasOwnProperty('value')) { @@ -203,6 +209,15 @@ 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 + if ( + (props.type === 'submit' || props.type === 'reset') && + (props.value === undefined || props.value === null) + ) { + return; + } + const initialValue = '' + node._wrapperState.initialValue; const currentValue = node.value; From 345da685e158a743d9630e83c8cfb6c92361a6c4 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 16 Aug 2018 15:01:48 +0100 Subject: [PATCH 2/3] Update ReactDOMFiberInput.js --- packages/react-dom/src/client/ReactDOMFiberInput.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index d856f8105ee..3080782029f 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -211,8 +211,9 @@ export function postMountWrapper( 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 ( - (props.type === 'submit' || props.type === 'reset') && + (type === 'submit' || type === 'reset') && (props.value === undefined || props.value === null) ) { return; From 3fa437aaa238aaf72b3d0eb89ff19a97d9a601eb Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 16 Aug 2018 15:15:43 +0100 Subject: [PATCH 3/3] More test coverage --- .../src/__tests__/ReactDOMInput-test.js | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 300af0283c1..2db0570c23a 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -810,6 +810,62 @@ describe('ReactDOMInput', () => { expect(node.getAttribute('value')).toBe('banana'); }); + it('should not set an undefined value on a submit input', () => { + let stub = ; + ReactDOM.render(stub, container); + const node = container.firstChild; + + // Note: it shouldn't be an empty string + // because that would erase the "submit" label. + expect(node.getAttribute('value')).toBe(null); + + ReactDOM.render(stub, container); + expect(node.getAttribute('value')).toBe(null); + }); + + it('should not set an undefined value on a reset input', () => { + let stub = ; + ReactDOM.render(stub, container); + const node = container.firstChild; + + // Note: it shouldn't be an empty string + // because that would erase the "reset" label. + expect(node.getAttribute('value')).toBe(null); + + ReactDOM.render(stub, container); + expect(node.getAttribute('value')).toBe(null); + }); + + it('should not set a null value on a submit input', () => { + let stub = ; + expect(() => { + ReactDOM.render(stub, container); + }).toWarnDev('`value` prop on `input` should not be null'); + const node = container.firstChild; + + // Note: it shouldn't be an empty string + // because that would erase the "submit" label. + expect(node.getAttribute('value')).toBe(null); + + ReactDOM.render(stub, container); + expect(node.getAttribute('value')).toBe(null); + }); + + it('should not set a null value on a reset input', () => { + let stub = ; + expect(() => { + ReactDOM.render(stub, container); + }).toWarnDev('`value` prop on `input` should not be null'); + const node = container.firstChild; + + // Note: it shouldn't be an empty string + // because that would erase the "reset" label. + expect(node.getAttribute('value')).toBe(null); + + ReactDOM.render(stub, container); + expect(node.getAttribute('value')).toBe(null); + }); + it('should set a value on a reset input', () => { let stub = ; ReactDOM.render(stub, container);