@cds/react: controlled form inputs

[coryrylan]

Describe the bug

Right now in @cds/react certain form inputs do not work well with controlled input types in React. In this example the second checkbox event is ignored and not updated.

https://stackblitz.com/edit/react-cds-checkbox-onchange-bgcacb?file=index.tsx

This seems specific to how React handles Controlled vs Uncontrolled input types as the plain html/js works without issue
https://stackblitz.com/edit/typescript-2ysgfw?file=index.html

https://reactjs.org/docs/forms.html#controlled-components

How to reproduce

Please provide a link to a reproduction scenario using one of the Clarity Stackblitz templates. Reports without a clear reproduction may not be prioritized until one is provided.

Steps to reproduce the behavior:

1. Go to https://stackblitz.com/edit/react-cds-checkbox-onchange-bgcacb?file=index.tsx
2. Click on the first checkbox
3. Click on the second checkbox
4. See missing log from second checkbox

Expected behavior

Controlled inputs should work with the custom element.

Versions

Clarity project:

• Clarity Core
• Clarity Angular/UI

Clarity version:

• v3.x
• v4.x
• v5.x

Framework:

• Angular
• React
• Vue
• Other:

Framework version:
ie: Angular 11

Device:

• Type: [e.g. MacBook]
• OS: [e.g. iOS]
• Browser [e.g. chrome, safari]
• Version [e.g. 22]

Additional notes

Add any other notes about the problem here.

[ashleyryan]

It looks like this is fixed, do you know when your next release will be?

[coryrylan]

@ashleyryan likely sometime this week

[ashleyryan]

Hi @coryrylan

I just updated to the newest version of Clarity Core, and the inputs still have different behavior than vanilla inputs. If I set a CdsCheckbox's value programmatically, the onChange event will fire, even though the user didn't interact with it. A regular input only fires onChange when the input is clicked.

Normally this isn't a huge deal, because if you call setState again but the value doesn't change, nothing re-renders, nbd. However, we have a couple of gnarly multiselect/combo box components that assume onChange means that the value has changed manually and toggles the results between true/false without checking for the value of e.target.checked. This is resulting in an infinite loop that's crashing the browser. For a number of reasons, this behavior isn't so easy to change and we would expect the behavior to mimic that of a normal input element in react.

Updated stackblitz: https://stackblitz.com/edit/react-cds-checkbox-onchange-bgcacb-vowfux?file=index.tsx

If you look in the console, you'll see when the events fire

[coryrylan]

Ok, we will need to revisit this. The primary issue we are running into is React overrides the setters on the native input when using controlled inputs. This breaks the native setter call which was causing the original issue. When the user interacts with the input React ignores the native DOM event and skips a render cycle since it uses its own setter and internal event. Here is a demo of the issue

https://stackblitz.com/edit/react-ts-nwvy1e

https://hustle.bizongo.in/simulate-react-on-change-on-controlled-components-baa336920e04
https://stackoverflow.com/questions/23892547/what-is-the-best-way-to-trigger-onchange-event-in-react-js#46012210
facebook/react#11488

[ashleyryan]

Another example, this time for the radio group. While you can avoid unnecessary re-renders in checkboxes by making sure to use the e.target.checked property, radios are going to result in multiple re-renders since onChange ends up being called n+1 times, with different values

Screen.Recording.2021-03-05.at.10.18.28.AM.mov

[ashleyryan]

I accidentally ended up in a twitter thread about React Components + Web Components and one of the maintainers from LitElement asked me about the issues we were facing, so I described the issue and he agreed that controlled components are tricky and suggested a "controlled" component: https://twitter.com/justinfagnani/status/1369071959175090176

I was playing around in Core and tried something like this in the update event of control-inline.element.ts, but then it just ended up immediately undoing my selection since the action seemed to be firing twice:

    if (this.controlled && props.get('checked') !== undefined && props.get('checked') !== this.checked) {
      this.checked = props.get('checked');
    }

I suspect some of the event listeners created in firstUpdated need to be tweaked as well, but explicitly telling the web component that it's controlled might be a good approach here.

Of course, then there's the problem that it's way too easy to leave controlled off of CdsCheckbox when creating your component. And with react, using the checked property instead of defaultChecked is already how you denote controlled vs uncontrolled, so this feels like an extra step that shouldn't be necessary.

[coryrylan]

@ashleyryan thanks for the update and link to the twitter thread! I'm working on this throughout this week so if I make progress I'll be sure to post back here.

[coryrylan]

@ashleyryan I have a potential fix, I am not yet 100% sure this is covering all the use cases. As far as I can test it seems to get the components in sync. #5718

[ashleyryan]

It fixes the stackblitz issue anyway, I copied the contents into App.tsx in the clarity react folder and it works as expected. Also tested out making sure indeterminate still sets like it should.

[coryrylan]

Merged #5718 but going to keep this open until its published so we can follow up

[coryrylan]

Closing for now, will reopen if needed

[github-actions]

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.