NumberField - too late onChange call

[tomekancu]

Hi, according to the documentation of NumberField the onChange handler is only called in the onBlur event.
This leads to unwanted behaviour in the form with appearing fields.

For example we could have a form that displays an additional field after checking the value of our NumberField.
In such a view, the user can press the next button without even seeing the additional checkbox.

This problem wouldn't occur if onChange was called on each key event.

I thought about a possible solution and I came up with something that doesn't need to change the current cursor position.

1. For uncontrolled components the solution is quite simple - just call onChange with the current value of numberValue from state - we don't have to worry about the formatting the value because this can still be done on the blur event.
2. The main problem is for the controlled components, when the value changes we should:
   • when value === state.numberValue - do nothing, not even formatting to keep the current cursor position, the formatting can be done on blur
   • when value !== state.numberValue - set the numberValue and move the cursor to the end of the text(the controlled value is the most important)

To make sure this works, numberValue should have a value that would be emitted with onBlur for the current inputValue state.

What do you think about this solution?

[snowystinger]

Thanks for your thoughts. This is definitely a tricky situation.

I think this runs into an issue where the value from onChange may not match the value the user thinks they entered. For instance, if the value entered is not parseable to a number, then some nullish value would be in the onChange and it'd only be revealed onBlur when the formatter fails or parses to something else.

Instead, I'd consider always showing your fields so that users don't accidentally skip them. Users can also be prepared for all the fields they may encounter.

Finally, I think hiding and showing form fields can be problematic. There wouldn't be an announcement for a screen reader necessarily for something like this. You could validate the age on submit and create an error on that field. I would also recommend a description on the numberfield which tells users the acceptable range. You could also make use of the min/max value.

[tomekancu]

this behaviour is also present in React's native input

What I meant was that using onChange in a simple <input /> could cause the same problems with cursor, as you did with your setV function.

As long as a user can meet the controlled/uncontrolled paradigm requirements, I don't see any misuse of onChange.

Keeping example with just an <input />. Let's assume that someone want's to trim spaces from ends of the text(like you did with keeping value <= 50).

function Component() {
  const [v, setV] = React.setState("");
  const handleChange = ev => setV(ev.target.value.trim());

  return <input value={ v } onChange={ handleChange } />
}

Such an implementation would prevent the user from using spaces in the middle of the text while typing.
That's what I meant by misusing of onChange and React's native behaviour of onChange(you should not change value during onChange)

[snowystinger]

That's what I meant by misusing of onChange and React's native behaviour of onChange (you should not change value during onChange)

Right, so we're saying the same thing again.

I see a couple issues with using:

onInputValue: (val: number) => void
This can result in a val of NaN depending on partial parsing, or it might just be another value completely.

onInputValue: (val: string) => void
The user would need to parse this, which would be error prone due to the locales and numbering systems we support.

In addition, both could be misused for a controlled state and opens us to bugs if there are two sources of truth to control the component.

I did have another thought, though. If the Tab key is pressed, we could possibly commit during that, much like we do for the 'Return' key, and get ahead of the blur. Do you think that would work for your use case? I haven't thought through the implications of this fully yet.

[tomekancu]

This can result in a val of NaN depending on partial parsing, or it might just be another value completely.

I cannot reproduce the case. In all my tests, the number was parsed correctly - even in different numeric systems.
I was only allowed to use allowed characters(thousand separator, decimal separator, number), additional thousand separators or decimal separators were skipped during parsing.
The only situation where I got NaN, was an empty string.

If the Tab key is pressed, we could possibly commit during

This will help a little bit, but only for users who only use the keyboard to move through the form.

[snowystinger]

The failed parsing usually comes in with partial currency or units, you can check out our tests here

react-spectrum/packages/@internationalized/number/test/NumberParser.test.js

Line 105 in b2d25ef

expect(new NumberParser('en-US', {currency: 'USD', style: 'currency', currencyDisplay: 'code'}).parse('EU 10.50')).toBe(NaN);

Admittedly, it is hard to type out the currency/units right now, but that's something we'd like to address. #2806

[tomekancu]

If I understand correctly:
When you fix the issue #2806, users will be able to delete part of the currency sign producing NaNs, but they won't be able to write the currency by themselves.
Users still won't be able to write other characters by themselves - only delete them.

Without the fix it is hard to say how my solution would work. Maybe skipping this extra onChange for NaNs would be enough.