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

Conversation

@ellsclytn
Copy link
Contributor

@ellsclytn ellsclytn commented May 11, 2018

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

// 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')) {
Copy link
Contributor

@aweary aweary May 21, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@ellsclytn ellsclytn May 21, 2018

Choose a reason for hiding this comment

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

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

Loading


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

Copy link
Contributor

@aweary aweary May 21, 2018

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?

Loading

@ellsclytn ellsclytn force-pushed the fix/empty-submit-value branch from f243454 to 187390f May 21, 2018
@pull-bot
Copy link

@pull-bot pull-bot commented May 21, 2018

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

Loading

Copy link
Member

@gaearon gaearon left a comment

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.

Loading

const node = ReactDOM.findDOMNode(stub);

expect(
!node.hasAttribute('value') || node.getAttribute('value').length > 0,
Copy link
Member

@gaearon gaearon May 22, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@ellsclytn ellsclytn May 22, 2018

Choose a reason for hiding this comment

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

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

Loading

@ellsclytn ellsclytn force-pushed the fix/empty-submit-value branch from 187390f to 48a151b May 22, 2018
@ellsclytn
Copy link
Contributor Author

@ellsclytn ellsclytn commented May 23, 2018

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

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented May 23, 2018

You can take the test you added and render twice.

Loading

@gebilaoxiong
Copy link
Contributor

@gebilaoxiong gebilaoxiong commented May 23, 2018

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

Loading

@aweary
Copy link
Contributor

@aweary aweary commented May 23, 2018

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:

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!

Loading

@gebilaoxiong
Copy link
Contributor

@gebilaoxiong gebilaoxiong commented May 23, 2018

@aweary

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

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

Loading

@aweary
Copy link
Contributor

@aweary aweary commented May 23, 2018

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

Loading

@ellsclytn ellsclytn force-pushed the fix/empty-submit-value branch from 48a151b to d423702 May 24, 2018
@ellsclytn
Copy link
Contributor Author

@ellsclytn ellsclytn commented May 24, 2018

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

Loading

@ellsclytn ellsclytn force-pushed the fix/empty-submit-value branch 2 times, most recently from 3db8654 to 14d5707 May 31, 2018
@@ -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,
Copy link
Member

@gaearon gaearon Aug 2, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@ellsclytn ellsclytn Aug 3, 2018

Choose a reason for hiding this comment

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

Good spot. Fixed 😄

Loading

@ellsclytn ellsclytn force-pushed the fix/empty-submit-value branch from 14d5707 to ac1f0d2 Aug 3, 2018
@gaearon
Copy link
Member

@gaearon gaearon commented Aug 3, 2018

@aweary Are you good with this fix?

Loading

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

@gaearon gaearon Aug 3, 2018

Choose a reason for hiding this comment

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

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

Loading

@ellsclytn ellsclytn force-pushed the fix/empty-submit-value branch 2 times, most recently from 72c049f to 7b1047c Aug 3, 2018
@@ -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)
Copy link
Member

@gaearon gaearon Aug 6, 2018

Choose a reason for hiding this comment

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

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

Loading

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

@gaearon gaearon Aug 6, 2018

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.

Loading

@@ -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') &&
Copy link
Member

@gaearon gaearon Aug 6, 2018

Choose a reason for hiding this comment

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

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

Loading

Copy link
Member

@gaearon gaearon left a comment

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.

Loading

@ellsclytn ellsclytn force-pushed the fix/empty-submit-value branch from 7b1047c to d93659e Aug 7, 2018
aweary
aweary approved these changes Aug 7, 2018
Copy link
Contributor

@aweary aweary left a comment

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.

Loading

@@ -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.
Copy link
Contributor

@aweary aweary Aug 7, 2018

Choose a reason for hiding this comment

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

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

Loading

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

it('should set a value to a submit input', () => {
Copy link
Contributor

@aweary aweary Aug 7, 2018

Choose a reason for hiding this comment

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

set a value *on a submit input

Loading

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

it('should not set a value for submit buttons when undefined is supplied', () => {
Copy link
Contributor

@aweary aweary Aug 7, 2018

Choose a reason for hiding this comment

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

I think a better name would be:

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

Loading

@ellsclytn ellsclytn force-pushed the fix/empty-submit-value branch from d93659e to 2fa41d5 Aug 7, 2018
@gaearon gaearon force-pushed the fix/empty-submit-value branch from 9a0d26d to 3fa437a Aug 16, 2018
@gaearon gaearon merged commit 9832a1b into facebook:master Aug 16, 2018
1 check was pending
Loading
@gaearon
Copy link
Member

@gaearon gaearon commented Aug 16, 2018

Thank you!

Loading

@ellsclytn ellsclytn deleted the fix/empty-submit-value branch Aug 16, 2018
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* Avoid setting empty value on reset & submit inputs

* Update ReactDOMFiberInput.js

* More test coverage
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants