kobaltedev / kobalte

A UI toolkit for building accessible web apps and design systems with SolidJS.
https://kobalte.dev
MIT License
1.2k stars 61 forks source link

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

Open jcmonnin opened 3 months ago

jcmonnin commented 3 months ago

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).

1) 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.

1) 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.

1) 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.

jer3m01 commented 2 weeks ago

The onChange and onRawValueChange get called whenever any async events cause them to change internally. There might be a way to predict exactly when they should and shouldn't be called but it would be highly speculative.

456 fixes the call with NaN on construction.

The component works if both are controlled, although might be a bit hard to predict, but it's the only way currently to access those values (until contexts are exposed). For example if you need to display the formatted value elsewhere on the screen and also need the number value for processing.

This is definitely something I'd like to improve however so I'll keep this open while I consider what to do. Thanks for the issue!