mui / base-ui

Base UI is an open-source library of accessible, unstyled UI components for React.
https://mui.com/base-ui/
MIT License
281 stars 46 forks source link

[NumberField] onValueChange not being called when input value is changed #579

Open foobarvaz opened 2 months ago

foobarvaz commented 2 months ago

Steps to reproduce

Link to live example: https://codesandbox.io/p/sandbox/wpqjmg?file=%2Fsrc%2FApp.tsx%3A10%2C43-10%2C56

Steps:

  1. Focus on input and change it's value

Current behavior

onValueChange not being called when input value is changed

You can see that the text below the input will remain value: 0 until you focus out of the input. The blur leads to onValueChange being called.

Expected behavior

onValueChange being called when input value is changed

Context

Thanks for adding support for decimals in the new version of the number input ❤️

I'm working on an interactive calculator in which changes to input values should be reflected in calculations that are shown in the UI.

I tried to debug the issue and I found that on each change I reach this part of the code:

https://github.com/mui/base-ui/blob/78d4abd6a72b0e3d783aa7a09bfda8c7a7518b88/packages/mui-base/src/NumberField/Root/useNumberFieldRoot.ts#L607

Because event.isTrusted === true in this case, we call only setInputValue without calling setValue (which internally calls onValueChange)

I'm not sure if it's by design or not. So forgive me if I classified this issues as bug 🐛 by mistake.

Your environment

npx @mui/envinfo ``` Don't forget to mention which browser you used. Output from `npx @mui/envinfo` goes here. ```

Search keywords: onValueChange

atomiks commented 2 months ago

I'm not sure if it's by design or not. So forgive me if I classified this issues as bug 🐛 by mistake.

Yes it's by design. The reason is partial input may not be parseable as a valid number, so it can't properly be called in realtime/on change.

bengry commented 2 months ago

I'm not sure if it's by design or not. So forgive me if I classified this issues as bug 🐛 by mistake.

Yes it's by design. The reason is partial input may not be parseable as a valid number, so it can't properly be called in realtime/on change.

Can this potentially be made available on the hook variant of NumberField? Or an optional prop that defaults to false (= immediate change behavior disabled).

I understand the concern, but it gives a worse UX (less immediate feedback) because of an edge case, for a lack of a better term - when a used enters invalid values. In the happy flow path, the UX is just worse without any benefit. E.g. react-number-format deals fine with this - demo.

atomiks commented 2 months ago

I think the main concern is with compositional inputs such as pinyin, which depends on the client's locale/number system. The input already blocks most invalid input before it can be typed in, but composition events aren't blocked yet represent incomplete data that can't be parsed?

This behavior matches the RA library, though it doesn't state what "partial input":

The onChange event is triggered whenever the number value updates. This happens when the user types a value and blurs the input, or when incrementing or decrementing the value. It does not happen as the user types because partial input may not be parseable to a valid number.

Open to suggestions or PRs to improve this behavior however

bengry commented 2 months ago

The onChange event is triggered whenever the number value updates. This happens when the user types a value and blurs the input, or when incrementing or decrementing the value. It does not happen as the user types because partial input may not be parseable to a valid number.

Open to suggestions or PRs to improve this behavior however

I suggest the onValueChange fires only if the parsed input is valid. e.g. 1onValueChange(1) 2onValueChange(12) . → onValueChange not fired 3onValueChange(12.3)

oliviertassinari commented 1 month ago

Maybe an onInputChange event like in https://mui.com/material-ui/api/autocomplete/#autocomplete-prop-onInputChange would cover both needs (!== onChange)?


A side note, made me think of #600.

bengry commented 1 month ago

Maybe an onInputChange event like in mui.com/material-ui/api/autocomplete#autocomplete-prop-onInputChange would cover both needs (!== onChange)?

This means that the component must be controlled though, no? Or at the very least - moves the burden of parsing the data to see if the number is valid (yet) to the consumer. When onInputChange can update on every keystroke, and not just on blur, since it already handles the parsing. LMK if I missed something though.

atomiks commented 1 month ago

@bengry it does seem ok to me that onValueChange is called as soon as it can, as long as the current number string is parseable/valid.

@oliviertassinari In our API, instead of onInputChange you can just use onChange on the Input subcomponent to read the string value in realtime.