WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

Many form components don't support uncontrolled usage #57004

Open manzoorwanijk opened 11 months ago

manzoorwanijk commented 11 months ago

Description

I was trying to play around with @wordpress/components for a plugin settings page in React for a demo for a WordCamp talk and I immediately noticed some problems in the current state of the components needed to build a form.

The package and some of the components mentioned here have been around for years but still do not support the fundamental web standards. Using the components as uncontrolled is nearly impossible. One is forced to make the components controlled which is not very performant for large and complex forms.

Also, someone may want to stick to web fundamentals for building forms for plugin/theme settings pages.

Someone would argue that we:

Avoid the "controlled to uncontrolled" warning by forcing the internal <input /> element to be always in controlled mode

It is not a valid argument because the responsibility of that warning goes to the consumer component and that is how the native form elements work.

Let us start:

Step-by-step reproduction instructions

TextControl

<TextControl
    name="some-name"
    label='Some label'
    defaultValue='Some default value'
/>

Issues:

TextareaControl

Same as TextControl

__experimentalInputControl

One would argue that we have a better replacement for TextControl in the name of __experimentalInputControl but it again fails:

<InputControl
    name="some-name"
    label='Some label'
    defaultValue='Some default value'
/>

Issues

[!Warning] Styled(input) contains an input of type undefined with both value and defaultValue props. Input elements must be either controlled or uncontrolled (specify either the value prop, or the defaultValue prop, but not both). Decide between using a controlled or uncontrolled input element and remove one of these props.

ToggleControl

<ToggleControl
    label="Fixed Background"
    defaultChecked={false}
/>

Issues

CheckboxControl

<CheckboxControl
    name='some-name'
    value='some-value'
    defaultChecked={false}
    label='Some label'
/>

Issues

RadioControl

<RadioControl
    name="some-name"
    label={'Something'}
    options={[
        { value: '1', label: 'One' },
        { value: '2', label: 'Two', defaultChecked: true },
    ]}
/>

Issues

Screenshots, screen recording, code snippet

Codesandbox: https://codesandbox.io/p/sandbox/wordpress-components-issues-h5lgy8?file=%2Fsrc%2FApp.tsx

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

bph commented 9 months ago

@ciampo @mirka this issue was part of the Extensibility Issues Triage today and it wasn't clear if this was still on your radar. It is a developer experience-related issue.

... removed it from the IGE board as it's not a blocker for extensibility.

ciampo commented 9 months ago

Hey @bph , thank you for the ping!

It is indeed true that many components only work in controlled mode. They were built this way to follow the needs of the editor, where all values of those controls are tied to the editor and block settings.

Ideally, we'd like consumers of the package to be able to use all components in controlled or uncontrolled mode. Unfortunately, it's not always an easy task, since some of the changes that are needed would introduce backward compatibility issues.

Since @mirka and I started to work on the components package, we recognised this gap and tried to improve it where possible — for example, we recently enhanced controlled/uncontrolled support for ToggleGroupControl, and we added controlled mode to TabPanel (by replacing it with a new component, Tabs).

Given all of the above, this doesn't represent a priority at the moment, although we're always open to find ways to improve the situation a step at the time.

manzoorwanijk commented 6 months ago

Even the new components like FormToggle don't work in uncontrolled mode 🤦

ciampo commented 4 months ago

new components like FormToggle

AFAIK FormToggle was added to the package 6 years ago, and prior to that was already part of the Gutenberg repository — so I wouldn't call it "new" necessarily.

As mentioned above, we can't do much about existing components, because it would often require a non-trivial amount of changes (usually breaking) and it would introduce API inconsistencies too.

The good news is that we are aware of this need, and we're making sure that new components (and new versions of existing components) support both controlled and uncontrolled mode.

manzoorwanijk commented 3 months ago

Unfortunately, it's not always an easy task, since some of the changes that are needed would introduce backward compatibility issues.

As mentioned above, we can't do much about existing components, because it would often require a non-trivial amount of changes (usually breaking) and it would introduce API inconsistencies too.

I don't agree with that at all.

  1. Why would making an input support uncontrolled mode break backward compatibility?
  2. Won't you still be able to make it controlled if you want?

I'm willing to work on those "non-trivial amount of changes" if the team is up for it.

The point is, instead of creating new components for "the" basic use case, why not fix the existing ones?

mirka commented 3 months ago

The breaking changes we're worrying about is around the fact that some components support an implicit controlled/uncontrolled switch through a single value prop (#47473).

If we adopt this pattern on components that don't currently have it, it would introduce behavior changes in controlled usage when the consumer passes an undefined value (to explicity signify an "empty" value, rather than to signify uncontrolled usage).

But I'm starting to feel strongly that not supporting uncontrolled mode is inacceptable, so I think we should consider using the defaultValue pattern if necessary. We'll have to investigate, but it is possible that this can be added onto existing components in a non-breaking and consistent way. Even on components that already have the implicit mode switching on value.

We will first need to audit the current form components to understand the feasibility and impact. I believe we would need to know:

Then, we can maybe test out adding a defaultValue prop on a component that already has the mode switching, ideally with already good test coverage, and see if there are any blockers.

@WordPress/gutenberg-components Any thoughts about this plan of action?

tyxla commented 3 months ago

@mirka your plan makes sense to me. I'd love to have a better idea what we're fighting with here, and the audit you proposed should help with that, and answer questions like:

And I agree with this sentiment:

But I'm starting to feel strongly that not supporting uncontrolled mode is inacceptable

mostly because the @wordpress/components package is turning into a general-purpose admin library to cover all the modern admin design use cases, and we need our components to be more flexible. With that in mind, supporting both controlled and uncontrolled versions sounds like an essential use case.

ciampo commented 3 months ago

Hey @manzoorwanijk, as Lena also explained, some older components were written only to support controlled mode, which means that they don't offer a way to easily differentiate "empty value but controlled" from "uncontrolled value" (see this issue that Lena also linked above).

As an example, you can take a look at the utility function that I wrote to "patch" ToggleGroupControl's behavior in a way that preserved backwards compatibility:

https://github.com/WordPress/gutenberg/blob/1e11103505416b4ca6a366b9afa1dd9962acdf69/packages/components/src/toggle-group-control/toggle-group-control/utils.ts#L21-L50


@tyxla and @mirka , I agree that supporting controlled/uncontrolled mode for a component should be a commodity in 2024, and is quite essential for a component library that wants to become more general purpose.

We should definitely gather some extra info, but my gut feeling tells me that we may need to introduce some breaking changes if we want to support the canonical value / defaultValue / onChange pattern, in particular around how passing an undefined value affects components.

manzoorwanijk commented 3 months ago

Thank you for the explanation. Yes, I think breaking change might be a step in the right direction.

mirka commented 3 months ago

My hope is that it turns out we can do it without breaking changes though. Leave the value behavior as is on all components, and using the new defaultValue prop would be an opt in to standard uncontrolled behavior that ignores value.

@manzoorwanijk If you're willing to do some initial investigation, it would be great if we could have a list of control components with the following information:

  • Does it support uncontrolled?
  • Does it have roughly equivalent unit tests for both controlled/uncontrolled? (If not, uncontrolled support may not be "complete")
  • How does it support uncontrolled? (useControlledValue, useControlledState, or custom implementation)

If you don't have the bandwidth, even just the first bullet point would be helpful!