ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.92k stars 13.51k forks source link

feat: Ionic form controls should support `defaultValue` and `defaultChecked` properties #28193

Open chawes13 opened 1 year ago

chawes13 commented 1 year ago

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

When using uncontrolled form inputs, it is not possible to set default values. The defaultValue prop is ostensibly available according to the types, but setting it does not result in the value prop initially getting set during the first render. This differs from React's expected behavior for providing an initial value.

Expected Behavior

defaultValue is initially set to the input's value on first render

Steps to Reproduce

  1. Add defaultValue as a prop to an input
  2. Render the component
  3. Value is not set

Code Reproduction URL

https://stackblitz.com/edit/vxete3

Ionic Info

Ionic: Ionic CLI : 5.4.16

Utility: cordova-res : not installed native-run : not installed

System: NodeJS : v16.20.0 npm : 9.4.2 OS : Linux 5.0

(this is from Stackblitz)

Additional Information

Technically defaultValue is not listed as a property in the Ionic docs (https://ionicframework.com/docs/api/input#properties), but it is an accepted value according to the component's types (React.ForwardRefExoticComponent<JSX.IonInput & Omit<React.HTMLAttributes<HTMLIonInputElement>, "style"> & StyleReactProps & React.RefAttributes<HTMLIonInputElement>>).

If there's another way to use uncontrolled inputs in @ionic/react, please let me know!

sean-perkins commented 1 year ago

@chawes13 thanks for opening this issue.

The correct way to assign the value to IonInput is with the value property: https://stackblitz.com/edit/vxete3-xvbksm?file=src%2Fmain.tsx

The type definition files for React globally augments the type of HTMLAttributes:

interface HTMLAttributes<T> extends AriaAttributes, DOMAttributes<T> {
        // React-specific Attributes
        defaultChecked?: boolean | undefined;
        defaultValue?: string | number | ReadonlyArray<string> | undefined;
        suppressContentEditableWarning?: boolean | undefined;
        suppressHydrationWarning?: boolean | undefined;

      // Rest of built-in attributes
}

Source: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/3090530f3f230f7cfb2920b28bf49a938cd8aa59/types/react/index.d.ts#L1920-L1926

I'm going to close this as this isn't really an issue with Ionic. defaultValue is a react-specific implementation pattern, but for built-in controls. IonInput is a custom control with an API pattern that matches the web API.

chawes13 commented 1 year ago

Thanks for the quick response! I'm a little confused by your response about the types. If defaultValue is a React-specific attribute that doesn't work with IonInput, then shouldn't that property be omitted from the types that Ionic provides?

With regards to the implementation, I do think there are some potential footguns – especially now that the default recommendation in the React docs is to use uncontrolled inputs.

See this forked repo here: https://stackblitz.com/edit/vxete3-ojhbpn. If you only rely on value to set the default value of the input, you can still update the value (which aligns with the web API and not React), but the value will reset if anything causes the component to re-render. In the forked example, I can choose to opt out of the newsletter, but that choice will get reset after pressing "Read more..." 😱.

If the components have to be controlled (i.e., setting value and onIonChange explicitly), is there somewhere in the documentation where that can be communicated to try to mitigate the issue raised above? And does this possibly affect your types?

sean-perkins commented 1 year ago

@chawes13 appreciate the follow-up!

I looked into this more and defaultValue is part of the instance properties for certain built-in form controls. I'll need to see how it's actually used to see if it should be something we inherit from the host and pass to the inner form control element.

Can you elaborate on your forked repo? If I bind the value to ion-input, I don't see the value reset while interacting with your example: forked example.

Perhaps I'm miss understanding controlled vs. uncontrolled form controls? Reading this article: https://dmitripavlutin.com/controlled-inputs-using-react-hooks/#1-the-controlled-input, I can do the same approach with Ionic: https://stackblitz.com/edit/vxete3-ugkdag?file=src%2Fmain.tsx

chawes13 commented 1 year ago

Sorry about that! I wasn't very clear in my first example.

Here's a video showing the value resetting when the component is re-rendered (triggered by pressing the controlled lifetime membership checkbox and by pressing "Read more..."). You'll notice that the React component (set with defaultValue) and the controlled input (i.e., the lifetime membership checkbox that has both value and onIonChange set do not have their values reset when a render is triggered.

Video: https://share.zight.com/z8udqA6P Sandbox: https://stackblitz.com/edit/vxete3-ojhbpn

What I was trying to communicate in my previous comment was that this can happen pretty easily, but it's easy to miss. If you're going to set the value of the input, then you have to set the onIonChange handler or else you will be susceptible to this defect if the component re-renders for any reason. In React-land, you'll actually get a console warning if you forget to apply the onChange handler when value is included:

Lastly, you can see that the defaultValue attribute is accepted as part of the web API for text inputs. The element will keep track of this, even as the value changes: https://codepen.io/chawes13/pen/gOZorqq (fired on blur).

sean-perkins commented 1 year ago

@chawes13 wonderful, thank you for the video and the reproduction!

After more investigation this is where I'm at on the issue:

  1. I was wrong in my original assessment.
  2. Ionic form controls are only compatible as controlled inputs today, meaning developers need to bind both value and onIonChange or onIonInput.
  3. Ionic should support defaultValue and defaultChecked on the appropriate form controls. Due to the nature of adding new API members and the implications these attributes have, I think we should track this as a feature request.
  4. This issue has likely the most impact in React environments, but does apply to the web standard, where a developer may want to compare if the value of a control is different than it's initial value. a. The React type declarations could probably be modified to not mention that defaultValue and defaultChecked are React specific. This is a low priority.

I created a dev-build for a prototype of these changes for ion-input, available with: 7.4.2-dev.11695324357.11675a9b.

Forked reproduction with the dev-build: https://stackblitz.com/edit/vxete3-qw7tav?file=package.json,src%2Fmain.tsx

The defaultValue and defaultChecked attributes need to reflect to value and checked respectively. For the input element (our ion-input) this is described in more detail within the HTML spec.

Thanks again for the technical details to get this point. Let me know if I missed anything or if you have any additional feedback!

chawes13 commented 12 months ago
  1. Thank you for such an incredibly thoughtful response
  2. Let me know if there's anything I can do to help!!