Skip to content

Restore server controlled form fields to whatever they should be #26708

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 23, 2023
Merged

Restore server controlled form fields to whatever they should be #26708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9bc65c9
Restore server controlled form fields to whatever they should be
sebmarkbage Apr 23, 2023
fbcdceb
We need to get the lastProp in when diffInCommitPhase is false
sebmarkbage Apr 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 118 additions & 27 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,68 @@ function setProp(
case 'action':
case 'formAction': {
// TODO: Consider moving these special cases to the form, input and button tags.
if (enableFormActions) {
if (typeof value === 'function') {
// Set a javascript URL that doesn't do anything. We don't expect this to be invoked
// because we'll preventDefault, but it can happen if a form is manually submitted or
// if someone calls stopPropagation before React gets the event.
// If CSP is used to block javascript: URLs that's fine too. It just won't show this
// error message but the URL will be logged.
domElement.setAttribute(
key,
// eslint-disable-next-line no-script-url
"javascript:throw new Error('" +
'A React form was unexpectedly submitted. If you called form.submit() manually, ' +
"consider using form.requestSubmit() instead. If you're trying to use " +
'event.stopPropagation() in a submit event handler, consider also calling ' +
'event.preventDefault().' +
"')",
);
break;
} else if (typeof prevValue === 'function') {
// When we're switching off a Server Action that was originally hydrated.
// The server control these fields during SSR that are now trailing.
// The regular diffing doesn't apply since we compare against the previous props.
// Instead, we need to force them to be set to whatever they should be now.
// This would be a lot cleaner if we did this whole fork in the per-tag approach.
if (key === 'formAction') {
if (tag !== 'input') {
// Setting the name here isn't completely safe for inputs if this is switching
// to become a radio button. In that case we let the tag based override take
// control.
setProp(domElement, tag, 'name', props.name, props, null);
}
setProp(
domElement,
tag,
'formEncType',
props.formEncType,
props,
null,
);
setProp(
domElement,
tag,
'formMethod',
props.formMethod,
props,
null,
);
setProp(
domElement,
tag,
'formTarget',
props.formTarget,
props,
null,
);
} else {
setProp(domElement, tag, 'encType', props.encType, props, null);
setProp(domElement, tag, 'method', props.method, props, null);
setProp(domElement, tag, 'target', props.target, props, null);
}
}
}
if (
value == null ||
(!enableFormActions && typeof value === 'function') ||
Expand All @@ -495,24 +557,6 @@ function setProp(
if (__DEV__) {
validateFormActionInDevelopment(tag, key, value, props);
}
if (enableFormActions && typeof value === 'function') {
// Set a javascript URL that doesn't do anything. We don't expect this to be invoked
// because we'll preventDefault, but it can happen if a form is manually submitted or
// if someone calls stopPropagation before React gets the event.
// If CSP is used to block javascript: URLs that's fine too. It just won't show this
// error message but the URL will be logged.
domElement.setAttribute(
key,
// eslint-disable-next-line no-script-url
"javascript:throw new Error('" +
'A React form was unexpectedly submitted. If you called form.submit() manually, ' +
"consider using form.requestSubmit() instead. If you're trying to use " +
'event.stopPropagation() in a submit event handler, consider also calling ' +
'event.preventDefault().' +
"')",
);
break;
}
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
if (__DEV__) {
Expand Down Expand Up @@ -1138,7 +1182,7 @@ export function setInitialProperties(
break;
}
default: {
setProp(domElement, tag, propKey, propValue, props);
setProp(domElement, tag, propKey, propValue, props, null);
}
}
}
Expand Down Expand Up @@ -1169,7 +1213,7 @@ export function setInitialProperties(
break;
}
default: {
setProp(domElement, tag, propKey, propValue, props);
setProp(domElement, tag, propKey, propValue, props, null);
}
}
}
Expand Down Expand Up @@ -1935,7 +1979,14 @@ export function updatePropertiesWithDiff(
break;
}
default: {
setProp(domElement, tag, propKey, propValue, nextProps, null);
setProp(
domElement,
tag,
propKey,
propValue,
nextProps,
lastProps[propKey],
);
}
}
}
Expand Down Expand Up @@ -2010,7 +2061,14 @@ export function updatePropertiesWithDiff(
}
// defaultValue are ignored by setProp
default: {
setProp(domElement, tag, propKey, propValue, nextProps, null);
setProp(
domElement,
tag,
propKey,
propValue,
nextProps,
lastProps[propKey],
);
}
}
}
Expand Down Expand Up @@ -2045,7 +2103,14 @@ export function updatePropertiesWithDiff(
}
// defaultValue is ignored by setProp
default: {
setProp(domElement, tag, propKey, propValue, nextProps, null);
setProp(
domElement,
tag,
propKey,
propValue,
nextProps,
lastProps[propKey],
);
}
}
}
Expand All @@ -2066,7 +2131,14 @@ export function updatePropertiesWithDiff(
break;
}
default: {
setProp(domElement, tag, propKey, propValue, nextProps, null);
setProp(
domElement,
tag,
propKey,
propValue,
nextProps,
lastProps[propKey],
);
}
}
}
Expand Down Expand Up @@ -2105,7 +2177,14 @@ export function updatePropertiesWithDiff(
}
// defaultChecked and defaultValue are ignored by setProp
default: {
setProp(domElement, tag, propKey, propValue, nextProps, null);
setProp(
domElement,
tag,
propKey,
propValue,
nextProps,
lastProps[propKey],
);
}
}
}
Expand All @@ -2122,7 +2201,7 @@ export function updatePropertiesWithDiff(
propKey,
propValue,
nextProps,
null,
lastProps[propKey],
);
}
return;
Expand All @@ -2134,7 +2213,7 @@ export function updatePropertiesWithDiff(
for (let i = 0; i < updatePayload.length; i += 2) {
const propKey = updatePayload[i];
const propValue = updatePayload[i + 1];
setProp(domElement, tag, propKey, propValue, nextProps, null);
setProp(domElement, tag, propKey, propValue, nextProps, lastProps[propKey]);
}
}

Expand Down Expand Up @@ -2709,6 +2788,18 @@ function diffHydratedGenericElement(
const hasFormActionURL = serverValue === EXPECTED_FORM_ACTION_URL;
if (typeof value === 'function') {
extraAttributes.delete(propKey.toLowerCase());
// The server can set these extra properties to implement actions.
// So we remove them from the extra attributes warnings.
if (propKey === 'formAction') {
extraAttributes.delete('name');
extraAttributes.delete('formenctype');
extraAttributes.delete('formmethod');
extraAttributes.delete('formtarget');
} else {
extraAttributes.delete('enctype');
extraAttributes.delete('method');
extraAttributes.delete('target');
}
if (hasFormActionURL) {
// Expected
continue;
Expand Down
1 change: 0 additions & 1 deletion packages/react-dom-bindings/src/client/ReactDOMInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ export function updateInput(
// Submit/reset inputs need the attribute removed completely to avoid
// blank-text buttons.
node.removeAttribute('value');
return;
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Apr 23, 2023

Choose a reason for hiding this comment

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

I noticed that this return was problematic because we don't restore the name after setting it to empty.

}

if (disableInputAttributeSyncing) {
Expand Down
Loading