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

Uncontrolled TextField/Input maintains value after it's cleared #5504

Closed
mike-wheel opened this issue Nov 30, 2023 · 12 comments
Closed

Uncontrolled TextField/Input maintains value after it's cleared #5504

mike-wheel opened this issue Nov 30, 2023 · 12 comments

Comments

@mike-wheel
Copy link

mike-wheel commented Nov 30, 2023

Provide a general summary of the issue here

Calling inputRef.current.value = "" on an uncontrolled TextField/Input should clear the value.

🤔 Expected Behavior?

The value should be cleared and never come back.

😯 Current Behavior

The value is (kindof) cleared, but will return after hovering/focusing over the <Input> with the mouse.

💁 Possible Solution

It might have something to do with the introduction of useControlledState in useTextField.

https://github.com/adobe/react-spectrum/pull/5288/files#diff-d45d481a028632c7e3b8d7238d268bd29e01a39e39f2a2bbcbd481991de6f46cR119

🔦 Context

No response

🖥️ Steps to Reproduce

https://stackblitz.com/edit/stackblitz-starters-cvmr1x

  1. Enter a value in to the text field
  2. Click the clear button, the value should be cleared
  3. Hover or focus the input field

Expected: nothing should happen

Actual: the cleared value is restored

Version

react-aria-components@1.0.0-rc.0

What browsers are you seeing the problem on?

Firefox, Chrome, Safari, Microsoft Edge

If other, please specify.

No response

What operating system are you using?

macOS Sonoma 14.1.1

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

@snowystinger
Copy link
Member

snowystinger commented Nov 30, 2023

I am not quite sure how this could ever work well with React, there is no onChange event if you set the value directly.
https://codesandbox.io/p/sandbox/prod-paper-h4lrzy?file=%2Fsrc%2FApp.js%3A7%2C11

Could you use the textfield in a controlled way instead? then you could just update the state to an empty string. Otherwise you could use a button type 'reset' for uncontrolled.

@mike-wheel
Copy link
Author

mike-wheel commented Nov 30, 2023

Hi @snowystinger, the provided code is a minimal reproducible example to demonstrate the bug. In our use-case, we need the Input to be uncontrolled.

I forked your example and used a TextField and Input from react-aria-components, and I'm still able to reproduce the issue.

@snowystinger
Copy link
Member

I feel like we're discussing a solution without discussing the problem. Can you provide more information as to why this must be uncontrolled, yet also clearable? What is the use case here?

Here's what I mean with the reset button btw https://codesandbox.io/p/sandbox/great-neumann-n92t7c?file=%2Fsrc%2FApp.js%3A12%2C33

@mike-wheel
Copy link
Author

We're building a component library that needs to support both controlled and uncontrolled text inputs with a clear button.

But our particular use-case doesn't seem relevant to me, since the example has an uncontrolled component that is displaying unexpected behavior. Is this not something that react-aria-components should support because it's an edge case?

@mike-wheel
Copy link
Author

I'm looking at the PR that I linked to, and this is the part that potentially introduced this behavior:

-  value: props.value,
-  defaultValue: props.value ? undefined : props.defaultValue,
-  onChange: (e: ChangeEvent<HTMLInputElement>) => onChange(e.target.value),
+  value,
+  onChange: (e: ChangeEvent<HTMLInputElement>) => setValue(e.target.value),

Is there a reason why the controlled value needs to be passed back through? I'm wondering if updating the onChange to intercept the value is enough and keep the value and defaultValue how they were, like this:

value: props.value,
defaultValue: props.value ? undefined : props.defaultValue,
-  onChange: (e: ChangeEvent<HTMLInputElement>) => onChange(e.target.value),
+  onChange: (e: ChangeEvent<HTMLInputElement>) => setValue(e.target.value),

@snowystinger
Copy link
Member

Yeah, so the reason for that was to handle forms. Reset buttons also don't fire change events, so in order to make sure that React is aware that a reset happened, we have to listen for the reset, then explicitly update the state of the textfield to match.

We support the usage of our components in controlled and uncontrolled ways, but we control the underlying component. You would be looking at doing the same thing I think. It is not very "react"-y to call something like input.value = 'foo' because then React has no idea that the value has changed and if there was any state hooked up to it, then on the next render it would "revert" as you've noted.

Here's a little sketch of how to make a component which can be either controlled or uncontrolled to a user of the component.

function ClearableTextField (props) {
  let isControlled = props.value !== undefined;
  let [value, _setValue] = useState(props.defaultValue);
  function setValue(val) {
    if (!isControlled) {
      _setValue(val);
    }
    props.onChange(val);
  }
  
  return (
    <RACTextfield value={isControlled ?  props.value : value} onChange={setValue} >...</RAC...>
  )
}

Note, you can replace most of that with our useControlledState, this is mostly what it's doing.

Honestly, TextField has always been a little weird because its state wasn't controlled internally previously. Every other one of our inputs is, so you'd have run into this pretty quickly.

@mike-wheel
Copy link
Author

mike-wheel commented Dec 6, 2023

Hi @snowystinger, I'm confused about why reset needs to be handled explicitly like that.

If the TextField is controlled, the author wouldn't expect a native reset button to clear the value. They would instead hook up a reset button to clear the values in whatever they use for their state management.

If the TextField is not controlled, the native reset would work as expected, right?

(see example here)

@snowystinger
Copy link
Member

Reset is a bit of a difficult one. Even for an uncontrolled textfield, onChange should still work.
See the onChanges which are called for your example https://stackblitz.com/edit/stackblitz-starters-qxe1oe?description=React%20%20%20TypeScript%20starter%20project&file=src%2FApp.tsx,src%2Fstyle.css&title=React%20Starter
vs how ours works
https://stackblitz.com/edit/stackblitz-starters-qz2h6q?description=React%20%20%20TypeScript%20starter%20project&file=src%2FApp.tsx,package.json&title=React%20Starter

And this is primarily needed on our side of things so we can monitor and attach/clear error messages at the correct time. It's also useful though for anyone trying to stay in sync with their fields, people may not be aware that reset will not fire the onChange and they may forget to hook up the reset to all of their state.

Here's some history of work/issues on it:
#2825
#4795
#2824
#589

I still don't think we've answered the question though, which is, why do you need to be able to clear the input like this input.value = 'foo', which would not be recommended when writing a react component. The preferred method being to have some state which controls the value, then the clear button would setValue(''). And if the component needs to appear to users to be uncontrolled, you can do that with the code I outlined above.

@mike-wheel
Copy link
Author

Ahh, I see, the behavior of onChange for TextField is different than a native input. The new behavior is supposed to fire onChange when the native <input type="reset" /> is clicked.

Going back to my comment above, maybe the custom onChange behavior would still work if the value wasn't passed for uncontrolled components? Something like:

value: isControlled ? value : undefined

why do you need to be able to clear the input like this input.value = 'foo'

I did answer this above, we're creating a component library that needs to support both controlled and uncontrolled text inputs with a clear button. We don't know what a future use case of this component library would be, but we need to support the clear button for uncontrolled components.

I had to disable the clear button for uncontrolled components only until this issue is resolved.

But in general, uncontrolled input components can be useful in certain situations if you have a one way data flow (input value to callback) and only want to "commit" a change if a certain condition is true. For example, we have a page that autosaves changes, and we don't want to trigger that autosave if the user input is invalid, but we still want the change to show in the input.

@snowystinger
Copy link
Member

I'm really sorry, but I still don't follow your explanation for needing to clear it via input.value = 'foo'. Why can't you keep state (as I outlined above) and expose it to your users as both uncontrolled and controllable, while you always control it internally?

You should be able to hold invalid state and you should also be able to perform validation on it and see if you should be autosaving or not. See https://react-spectrum.adobe.com/react-aria/forms.html#realtime-validation

Maybe a more realistic example would be helpful?

Because we control the value internally, probably the best way to expose it would be through useImperativeHandle, that way we could update the state. However, I'm still not convinced this can't be solved with state. I also think this would be a problem for your requirement with every input field we have.

@mike-wheel
Copy link
Author

Please don't get hung up on specific use cases for uncontrolled inputs. The fact is, the example above has unpredictable behavior (whether or not it's the best way to solve a problem in react).

Because we control the value internally

My last post suggested that you may be able to just keep track of the value internally without actually controlling it. Is this a possibility?

value: isControlled ? value : undefined

@devongovett
Copy link
Member

devongovett commented Dec 12, 2023

I think this request is fundamentally at odds with how React works. If you mutate the DOM directly, whether on an input or any other element, React won't know about it and your changes might get reset. In your example, you are controlling the value when you reset it, just not via React. This goes behind React's back and subverts the one-way dataflow that React expects.

An uncontrolled input is one where you never need to programmatically update the value. As soon as you need to update it, you need to make the input controlled. In React this is done with useState, passing the current value to the value prop. The source of truth should be in your component's state, not in the DOM. This way, React and React Aria are aware of the current value as well, and can synchronize their internal logic. For example, validation needs to know the current value so that it can update accordingly. If you mutate the DOM yourself, this won't work correctly.

I'm going to close this issue because I don't think there is any action we can take here. I'd recommend reading the React docs to learn more about this topic if you're new to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants