kobalte icon indicating copy to clipboard operation
kobalte copied to clipboard

number-field: inconsistencies when `onChange` and `onRawValueChange` is called

Open jcmonnin opened this issue 9 months ago • 1 comments

In text-field, onChange is only called when change originates from user interaction, but not when the controlled value changes. I think this is the right semantics. Some use cases need to be able to make the distinction if change comes from user action.

See this playground to illustrate the text-field behavior. Note that on the console, no onChange message appears when Set Text to "Test" button is pressed: https://playground.solidjs.com/anonymous/174c66df-0efe-4815-9469-abc6f0ecc552

number-field doesn't behave that way. Here are the examples:

  1. Controlled with text: https://playground.solidjs.com/anonymous/e72d7529-1ec1-43f6-903d-0bdefa35e4e1
  2. Controlled with number: https://playground.solidjs.com/anonymous/99c481fe-e885-40a4-a643-aebe8e99c3f8

In case 1) note that when inputting a number bigger than 1000, and then pressing Set Number to 10 it first calls onChange with the formatted value having the thousand separator, which shouldn't be called when value is discarded when controlled value changes.

Side Note: In practice the text mode isn't really suitable as I don't have access to formatter/parser from the outside and it's necessary to convert numbers. Note that as soon as value is formatted with separators, controlled value is wrong.

Case 2) is the one that matters in practice. At construction, onRawValueChange is called with NaN, which is debatable if it should be called. While typing the number bigger than 1000 onRawValueChange is called as expected. However when the Set Number to 10 button is pressed, onRawValueChange is called twice, once again with the typed number (after formatting) and then with 10. If wanting to reproduce text-field semantics, it should not be called at all since it just takes the externally controlled value.


I add some slightly subjective opinion about number-field here:

  1. There is following API to specify controlled value value: Accessor<string | number | undefined> and rawValue: Accessor<number>. It's not clear why two props are needed and if using always value is equivalent or not. What happens if two separate signals are passed? Maybe it's better to have a single source of truth for the controlled value (eg. remove rawValue prop).

  2. I think it's hard to have the correct behavior in edge cases when having controlled values for both text and number. In react spectrum NumberField, they have chose to just expose the number, which I think would be sufficient. I rather have a simpler API with the event semantics like text-field rather than more flexibility but unpredictable calls to events.

  3. When text of number field is empty, I prefer having undefined in the onRawValueChange callback, rather than NaN. The main reason I prefer to avoid it is the NaN != NaN behavior which can be nasty in change detection (in signals or other places). Note that this is quite subjective.

  4. Naming: I was initially a bit confused by the naming of rawValue. Given this component is about number, my brain associates value with the number and raw with the text form as typed. It's obviously not a big issue, especially since the documentation is great. I would find the inverse naming more logical, or alternatively some name that avoids raw like numberValue.

jcmonnin avatar May 17 '24 15:05 jcmonnin