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

Support useFormStatus in progressively-enhanced forms #29019

Merged
merged 2 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
153 changes: 109 additions & 44 deletions packages/react-dom-bindings/src/events/plugins/FormActionEventPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,61 @@ import type {EventSystemFlags} from '../EventSystemFlags';
import type {Fiber} from 'react-reconciler/src/ReactInternalTypes';
import type {FormStatus} from 'react-dom-bindings/src/shared/ReactDOMFormActions';

import {enableTrustedTypesIntegration} from 'shared/ReactFeatureFlags';
import {getFiberCurrentPropsFromNode} from '../../client/ReactDOMComponentTree';
import {startHostTransition} from 'react-reconciler/src/ReactFiberReconciler';
import {didCurrentEventScheduleTransition} from 'react-reconciler/src/ReactFiberRootScheduler';
import sanitizeURL from 'react-dom-bindings/src/shared/sanitizeURL';
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';

import {SyntheticEvent} from '../SyntheticEvent';

function coerceFormActionProp(
actionProp: mixed,
): string | (FormData => void | Promise<void>) | null {
// This should match the logic in ReactDOMComponent
if (
actionProp == null ||
typeof actionProp === 'symbol' ||
typeof actionProp === 'boolean'
) {
return null;
} else if (typeof actionProp === 'function') {
return (actionProp: any);
} else {
if (__DEV__) {
checkAttributeStringCoercion(actionProp, 'action');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kind of annoying to need these even though we've checked that it's not a symbol already.

}
return (sanitizeURL(
enableTrustedTypesIntegration ? actionProp : '' + (actionProp: any),
): any);
}
}

function createFormDataWithSubmitter(
form: HTMLFormElement,
submitter: HTMLInputElement | HTMLButtonElement,
) {
// The submitter's value should be included in the FormData.
// It should be in the document order in the form.
// Since the FormData constructor invokes the formdata event it also
// needs to be available before that happens so after construction it's too
// late. We use a temporary fake node for the duration of this event.
// TODO: FormData takes a second argument that it's the submitter but this
// is fairly new so not all browsers support it yet. Switch to that technique
// when available.
const temp = submitter.ownerDocument.createElement('input');
temp.name = submitter.name;
temp.value = submitter.value;
if (form.id) {
temp.setAttribute('form', form.id);
}
(submitter.parentNode: any).insertBefore(temp, submitter);
const formData = new FormData(form);
(temp.parentNode: any).removeChild(temp);
return formData;
}

/**
* This plugin invokes action functions on forms, inputs and buttons if
* the form doesn't prevent default.
Expand All @@ -42,16 +92,19 @@ function extractEvents(
}
const formInst = maybeTargetInst;
const form: HTMLFormElement = (nativeEventTarget: any);
let action = (getFiberCurrentPropsFromNode(form): any).action;
let submitter: null | HTMLInputElement | HTMLButtonElement =
let action = coerceFormActionProp(
(getFiberCurrentPropsFromNode(form): any).action,
);
let submitter: null | void | HTMLInputElement | HTMLButtonElement =
(nativeEvent: any).submitter;
let submitterAction;
if (submitter) {
const submitterProps = getFiberCurrentPropsFromNode(submitter);
submitterAction = submitterProps
? (submitterProps: any).formAction
: submitter.getAttribute('formAction');
if (submitterAction != null) {
? coerceFormActionProp((submitterProps: any).formAction)
: // The built-in Flow type is ?string, wider than the spec
((submitter.getAttribute('formAction'): any): string | null);
if (submitterAction !== null) {
// The submitter overrides the form action.
action = submitterAction;
// If the action is a function, we don't want to pass its name
Expand All @@ -60,10 +113,6 @@ function extractEvents(
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The submitterAction that's coming from formAction needs to be coerced too. Technically it was wrong before because it wasn't ignoring boolean/symbol and was treating them as the submitter taking over.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also the submitter being set to null in that branch is not correct in the case where it's not a function. It previously just assumed that if it wasn't a function it would've exited early anyway.


if (typeof action !== 'function') {
return;
}

const event = new SyntheticEvent(
'action',
'action',
Expand All @@ -74,44 +123,60 @@ function extractEvents(

function submitForm() {
if (nativeEvent.defaultPrevented) {
// We let earlier events to prevent the action from submitting.
return;
}
// Prevent native navigation.
event.preventDefault();
let formData;
if (submitter) {
// The submitter's value should be included in the FormData.
// It should be in the document order in the form.
// Since the FormData constructor invokes the formdata event it also
// needs to be available before that happens so after construction it's too
// late. We use a temporary fake node for the duration of this event.
// TODO: FormData takes a second argument that it's the submitter but this
// is fairly new so not all browsers support it yet. Switch to that technique
// when available.
const temp = submitter.ownerDocument.createElement('input');
temp.name = submitter.name;
temp.value = submitter.value;
if (form.id) {
temp.setAttribute('form', form.id);
// An earlier event prevented form submission. If a transition update was
// also scheduled, we should trigger a pending form status — even if
// no action function was provided.
if (didCurrentEventScheduleTransition()) {
// We're going to set the pending form status, but because the submission
// was prevented, we should not fire the action function.
const formData = submitter
? createFormDataWithSubmitter(form, submitter)
: new FormData(form);
const pendingState: FormStatus = {
pending: true,
data: formData,
method: form.method,
action: action,
};
if (__DEV__) {
Object.freeze(pendingState);
}
startHostTransition(
formInst,
pendingState,
// Pass `null` as the action
// TODO: Consider splitting up startHostTransition into two separate
// functions, one that sets the form status and one that invokes
// the action.
null,
formData,
);
} else {
// No earlier event scheduled a transition. Exit without setting a
// pending form status.
}
(submitter.parentNode: any).insertBefore(temp, submitter);
formData = new FormData(form);
(temp.parentNode: any).removeChild(temp);
} else {
formData = new FormData(form);
}
} else if (typeof action === 'function') {
// A form action was provided. Prevent native navigation.
event.preventDefault();

const pendingState: FormStatus = {
pending: true,
data: formData,
method: form.method,
action: action,
};
if (__DEV__) {
Object.freeze(pendingState);
// Dispatch the action and set a pending form status.
const formData = submitter
? createFormDataWithSubmitter(form, submitter)
: new FormData(form);
const pendingState: FormStatus = {
pending: true,
data: formData,
method: form.method,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed we were already reading the method from the form element here. Should I change this to coerce the React prop like we're doing with action? Should this be getAttribute instead? Unsure of all the implications.

Copy link
Collaborator

@sebmarkbage sebmarkbage May 8, 2024

Choose a reason for hiding this comment

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

Yea, it's tricky. In this case it's good because we get the right default and it gets normalized to something that's easy to compare. E.g. you don't have to think of this enum having different cases.

However, for the action if it gets normalized the same thing is actually kind of annoying because now you have to think first normalizing your own url string to whatever the browser does.

So I could see the value in the normalization diverging here.

action: action,
};
if (__DEV__) {
Object.freeze(pendingState);
}
startHostTransition(formInst, pendingState, action, formData);
} else {
// No earlier event prevented the default submission, and no action was
// provided. Exit without setting a pending form status.
}
startHostTransition(formInst, pendingState, action, formData);
}

dispatchQueue.push({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ type FormStatusPending = {|
pending: true,
data: FormData,
method: string,
action: string | (FormData => void | Promise<void>),
action: string | (FormData => void | Promise<void>) | null,
|};

export type FormStatus = FormStatusPending | FormStatusNotPending;
Expand Down
Loading