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

Avoid setting empty value on reset & submit inputs #12780

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

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.

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.


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?

@pull-bot
Copy link

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

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

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.

const node = ReactDOM.findDOMNode(stub);

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

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.

Copy link
Contributor Author

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.

@ellsclytn
Copy link
Contributor Author

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.

@gaearon
Copy link
Collaborator

gaearon commented May 23, 2018

You can take the test you added and render twice.

@gebilaoxiong
Copy link
Contributor

gebilaoxiong commented May 23, 2018

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

@aweary
Copy link
Contributor

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!

@gebilaoxiong
Copy link
Contributor

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;
}

@aweary
Copy link
Contributor

aweary commented May 23, 2018

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

@ellsclytn
Copy link
Contributor Author

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

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot. Fixed 😄

@gaearon
Copy link
Collaborator

gaearon commented Aug 3, 2018

@aweary Are you good with this fix?

} 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
Collaborator

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.

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

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

) {
// 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.

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

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

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

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

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.

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

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

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

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

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

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

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

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"

@ellsclytn
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

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

@gaearon gaearon merged commit 9832a1b into facebook:master Aug 16, 2018
@gaearon
Copy link
Collaborator

gaearon commented Aug 16, 2018

Thank you!

@ellsclytn ellsclytn deleted the fix/empty-submit-value branch August 16, 2018 22:39
@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
Development

Successfully merging this pull request may close these issues.

Submit/Reset inputs lose text when value=undefined.
6 participants