Avoid setting empty value on reset & submit inputs #12780

ellsclytn · 2018-05-11T00:43:55Z

This fixes a regression of #7179. Previously, creating an input with type='submit' value={undefined} would result in a Submit button with the text using the browser default for the input type (usually "Submit", for en). Currently, passing through undefined results in an input with no text at all. The same behaviour can be demonstrated with type='reset'.

Resolves #12872

aweary · 2018-05-21T06:25:20Z

packages/react-dom/src/client/ReactDOMFiberInput.js

+  // Submit or reset inputs should not have their value overridden
+  // with a falsy value, or else this will result in a button with no text,
+  // ignoring the browser-default "Submit", etc.
+  if (!props.value && (props.type === 'submit' || props.type === 'reset')) {


This would make it impossible to intentionally set an empty value

<input type="submit" value="" />

We'd need a more specific check like props.value === undefined. It should probably be moved into the hasOwnProperty check below, so we can avoid checking type if value doesn't exist at all.

Should we cater to null in the same way? If we want to match React 15 behavior, anyway.

aweary · 2018-05-21T06:26:48Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

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


Can you add a test for an intentionally empty value?

pull-bot · 2018-05-21T07:14:15Z

ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.2%

Details of bundled changes.

Comparing: 8862172...3fa437a

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.1%	642.08 KB	642.58 KB	151.31 KB	151.45 KB	UMD_DEV
react-dom.production.min.js	🔺+0.2%	🔺+0.2%	95.07 KB	95.22 KB	30.9 KB	30.96 KB	UMD_PROD
react-dom.development.js	+0.1%	+0.1%	638.22 KB	638.72 KB	150.17 KB	150.33 KB	NODE_DEV
react-dom.production.min.js	🔺+0.2%	🔺+0.2%	95.03 KB	95.19 KB	30.48 KB	30.53 KB	NODE_PROD
ReactDOM-dev.js	+0.1%	+0.1%	644.97 KB	645.49 KB	148.23 KB	148.38 KB	FB_WWW_DEV
ReactDOM-prod.js	🔺+0.1%	🔺+0.1%	277.57 KB	277.85 KB	52.09 KB	52.17 KB	FB_WWW_PROD
react-dom.profiling.min.js	+0.2%	+0.2%	96.24 KB	96.39 KB	30.86 KB	30.91 KB	NODE_PROFILING
ReactDOM-profiling.js	+0.1%	+0.1%	279.99 KB	280.27 KB	52.73 KB	52.8 KB	FB_WWW_PROFILING

Generated by 🚫 dangerJS

gaearon

Returning is not sufficient.

This doesn't work if you first render with value="Foo" and then render with value={undefined} because it will just fail to update, and incorrectly keep "Foo" as the value.

gaearon · 2018-05-22T19:06:12Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

+    const node = ReactDOM.findDOMNode(stub);
+
+    expect(
+      !node.hasAttribute('value') || node.getAttribute('value').length > 0,


This is confusing. Which one of the two do you expect in jsdom environment? Please be specific. We don't run these tests in browsers so you don't need to account for different behavior.

Heh, good spot. This is a bit of copy-pasta from the tests above. Will fix.

ellsclytn · 2018-05-23T00:02:55Z

@gaearon Would you have any examples of tests where we handle that type of situation? Otherwise I'm more or less working in the dark, because we don't seem to cover that situation on any input field tests.

gaearon · 2018-05-23T01:55:41Z

You can take the test you added and render twice.

gebilaoxiong · 2018-05-23T04:53:24Z

We also need to consider the update phase, let me fix it : )

aweary · 2018-05-23T06:50:53Z

Returning early is sufficient for mounting, we just need to handle updates as well. @ellsclytn updates are handled by updateWrapper. This is the relevant code path for how we handle updating value:

react/packages/react-dom/src/client/ReactDOMFiberInput.js

Lines 183 to 195 in c601f7a

    
           if (value != null) { 
        
             if (props.type === 'number') { 
        
               if ( 
        
                 (value === 0 && node.value === '') || 
        
                 // eslint-disable-next-line 
        
                 node.value != value 
        
               ) { 
        
                 node.value = '' + value; 
        
               } 
        
             } else if (node.value !== '' + value) { 
        
               node.value = '' + value; 
        
             } 
        
           }

Since this already checks for null and undefined we can add an else if statement that checks if the props.type is submit or reset. In that case, we need to remove the value attribute and return early so it doesn't end up setting the default value later on in the function

if (value != null) {
 // ...
} else if (props.type === 'submit' || props.type === 'reset') {
  node.removeAttribute('value');
  return;
}

I tested this locally and verified that updates to and from undefined work as expected. @ellsclytn let me know if you have any questions!

gebilaoxiong · 2018-05-23T07:29:47Z

@aweary

I think we should consider isControlled(), it will case an warn, Right?

react/packages/react-dom/src/client/ReactDOMFiberInput.js

Lines 37 to 40 in c601f7a

    
           function isControlled(props) { 
        
             const usesChecked = props.type === 'checkbox' || props.type === 'radio'; 
        
             return usesChecked ? props.checked != null : props.value != null; 
        
           }

aweary · 2018-05-23T11:30:31Z

@gebilaoxiong we still want the warning to occur in this case.

ellsclytn · 2018-05-24T23:14:36Z

Thanks for the advice all! Made the appropriate changes, tested re-rendering with changing props, looks to be behaving properly.

gaearon · 2018-08-02T11:34:13Z

packages/react-dom/src/client/ReactDOMFiberInput.js

@@ -209,6 +214,16 @@ export function postMountWrapper(element: Element, props: Object) {
  const node = ((element: any): InputWithWrapperState);

  if (props.hasOwnProperty('value') || props.hasOwnProperty('defaultValue')) {
+    // Submit or reset inputs should not have their value overridden
+    // with a falsy value, or else this will result in a button with no text,


This says "falsy" but the code checks for null and undefined. What should happen for ''? For 0? For false or true? Let's make sure code and comments agree.

Good spot. Fixed 😄

gaearon · 2018-08-03T01:17:38Z

@aweary Are you good with this fix?

gaearon · 2018-08-03T01:19:16Z

packages/react-dom/src/client/ReactDOMFiberInput.js

+    } else if (props.type === 'submit' || props.type === 'reset') {
+      // Submit/reset inputs need the attribute removed completely to avoid
+      // blank-text buttons.
+      node.removeAttribute('value');


It's a bit surprising this is the only place we use removeAttribute in this file.

gaearon · 2018-08-06T15:32:06Z

packages/react-dom/src/client/ReactDOMFiberInput.js

@@ -182,6 +182,14 @@ export function updateWrapper(element: Element, props: Object) {
    } else if (node.value !== '' + value) {
      node.value = '' + value;
    }
+  } else if (
+    (props.type === 'submit' || props.type === 'reset') &&
+    (value === null || value === undefined)


This condition seems unnecessary because we're in an else branch for if (value != null).

gaearon · 2018-08-06T16:30:36Z

packages/react-dom/src/client/ReactDOMFiberInput.js

+  ) {
+    // Submit/reset inputs need the attribute removed completely to avoid
+    // blank-text buttons.
+    node.removeAttribute('value');


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

gaearon · 2018-08-06T16:31:43Z

packages/react-dom/src/client/ReactDOMFiberInput.js

@@ -182,6 +182,14 @@ export function updateWrapper(element: Element, props: Object) {
    } else if (node.value !== '' + value) {
      node.value = '' + value;
    }
+  } else if (
+    (props.type === 'submit' || props.type === 'reset') &&


Nit: since we access props.type so much can you please read type in a variable early and use that

gaearon

Please see review comments above. We need to make sure that any new lines are covered by tests that would fail were these lines removed.

aweary

Solution looks fine to me, just some minor nits. We're only testing for submit inputs, but this also affects reset inputs. It'd be nice to test that as well to prevent regressions.

aweary · 2018-08-07T01:15:11Z

packages/react-dom/src/client/ReactDOMFiberInput.js

@@ -203,6 +209,16 @@ export function postMountWrapper(
  const node = ((element: any): InputWithWrapperState);

  if (props.hasOwnProperty('value') || props.hasOwnProperty('defaultValue')) {
+    // Submit or reset inputs should not have their value overridden
+    // with a null or undefined value, or else this will result in a button with
+    // no text, ignoring the browser-default "Submit", etc.


Can we just shorten this and add a link to #12872 here for reference?

Avoid setting value attribute on submit/reset inputs as it overrides the default value provided by the browser. See: #12872

aweary · 2018-08-07T01:23:50Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

+    expect(node.getAttribute('value')).toBe(null);
+  });
+
+  it('should set a value to a submit input', () => {


set a value *on a submit input

aweary · 2018-08-07T01:23:52Z

packages/react-dom/src/__tests__/ReactDOMInput-test.js

+    expect(node.hasAttribute('value')).toBe(false);
+  });
+
+  it('should not set a value for submit buttons when undefined is supplied', () => {


I think a better name would be:

"should remove the value attribute on submit inputs when value is updated to undefined"

ellsclytn · 2018-08-07T01:34:34Z

@aweary Oops, I totally forgot about the reset type. I'll throw in some tests for those.

gaearon · 2018-08-07T01:34:55Z

packages/react-dom/src/client/ReactDOMFiberInput.js

+    // no text, ignoring the browser-default "Submit", etc.
+    if (
+      (props.value === undefined || props.value === null) &&
+      (props.type === 'submit' || props.type === 'reset')


Let's put this line first? It'll probably be false more often.

packages/react-dom/src/client/ReactDOMFiberInput.js

gaearon · 2018-08-16T14:19:07Z

Thank you!

facebook-github-bot added the CLA Signed label May 11, 2018

ellsclytn mentioned this pull request May 21, 2018

Submit/Reset inputs lose text when value=undefined. #12872

Closed

aweary reviewed May 21, 2018

View reviewed changes

gaearon requested changes May 22, 2018

View reviewed changes

aweary mentioned this pull request May 23, 2018

allow use empty value in input button #12887

Closed

gaearon reviewed Aug 2, 2018

View reviewed changes

gaearon reviewed Aug 3, 2018

View reviewed changes

gaearon reviewed Aug 6, 2018

View reviewed changes

gaearon requested changes Aug 6, 2018

View reviewed changes

aweary approved these changes Aug 7, 2018

View reviewed changes

gaearon reviewed Aug 7, 2018

View reviewed changes

ellsclytn and others added 3 commits August 7, 2018 11:43

Avoid setting empty value on reset & submit inputs

2fa41d5

Update ReactDOMFiberInput.js

345da68

Merge branch 'master' into fix/empty-submit-value

c9c97e9

gaearon approved these changes Aug 16, 2018

View reviewed changes

gaearon reviewed Aug 16, 2018

View reviewed changes

packages/react-dom/src/client/ReactDOMFiberInput.js Show resolved Hide resolved

More test coverage

3fa437a

gaearon merged commit 9832a1b into facebook:master Aug 16, 2018

ellsclytn deleted the fix/empty-submit-value branch August 16, 2018 22:39

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid setting empty value on reset & submit inputs #12780

Avoid setting empty value on reset & submit inputs #12780

ellsclytn commented May 11, 2018 •

edited

aweary May 21, 2018

ellsclytn May 21, 2018 •

edited

aweary May 21, 2018

pull-bot commented May 21, 2018 •

edited

react-dom

gaearon left a comment

gaearon May 22, 2018

ellsclytn May 22, 2018

ellsclytn commented May 23, 2018 •

edited

gaearon commented May 23, 2018

gebilaoxiong commented May 23, 2018 •

edited

aweary commented May 23, 2018

gebilaoxiong commented May 23, 2018 •

edited

aweary commented May 23, 2018

ellsclytn commented May 24, 2018

gaearon Aug 2, 2018

ellsclytn Aug 3, 2018

gaearon commented Aug 3, 2018

gaearon Aug 3, 2018

gaearon Aug 6, 2018

gaearon Aug 6, 2018

gaearon Aug 6, 2018

gaearon left a comment

aweary left a comment

aweary Aug 7, 2018

aweary Aug 7, 2018

aweary Aug 7, 2018

ellsclytn commented Aug 7, 2018

gaearon Aug 7, 2018

gaearon commented Aug 16, 2018

Avoid setting empty value on reset & submit inputs #12780

Avoid setting empty value on reset & submit inputs #12780

Conversation

ellsclytn commented May 11, 2018 • edited

Choose a reason for hiding this comment

ellsclytn May 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-bot commented May 21, 2018 • edited

react-dom

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellsclytn commented May 23, 2018 • edited

gaearon commented May 23, 2018

gebilaoxiong commented May 23, 2018 • edited

aweary commented May 23, 2018

gebilaoxiong commented May 23, 2018 • edited

aweary commented May 23, 2018

ellsclytn commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

aweary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellsclytn commented Aug 7, 2018

Choose a reason for hiding this comment

gaearon commented Aug 16, 2018

ellsclytn commented May 11, 2018 •

edited

ellsclytn May 21, 2018 •

edited

pull-bot commented May 21, 2018 •

edited

ellsclytn commented May 23, 2018 •

edited

gebilaoxiong commented May 23, 2018 •

edited

gebilaoxiong commented May 23, 2018 •

edited