SpinButton: Add onChange handler

[just-joshing]

Describe the feature that you would like added
Make SpinButton useful by adding an OnChange handler.
As it is right now you have to figure out some way to accomplish the same effect using a combination of onIncrement, onDecrement, and onValidate which is really frustrating because it makes useful features of the control, like setting max and min values, unavailable since you have to manage the control yourself.

What component or utility would this be added to
SpinButton

Additional context/screenshots
Right now our team is trying to use a work-around of using TextField set with type="number", but this doesn't include the nice accessibility aria attribute handling of SpinButton and is missing the useful max/min value clamping.

I noticed the private _onChange method in the component has a comment saying there was a bug in React preventing onChange from being implemented. I assume that issue is no longer a blocker given that the issue is marked as closed and TextField has an onChange handler.

This affects VSTS.

Thanks!

[lafe]

The documentation makes this even more confusing, as it states for the value property (emphasis mine):

The value of the SpinButton. Use this if you intend to pass in a new value as a result of onChange events. This value is mutually exclusive to defaultValue. Use one or the other.

Unfortunately, the mentioned onChange is missing...

[ecraig12345]

@JllyGrnGiant Thanks for reporting this, and sorry we didn't get to it sooner. And thanks @lafe for reporting the documentation issue.

The similar current props are onIncrement, onDecrement, and onValidate. All three have signatures like (value: string) => string | void, and if a string is returned, it's used as the new value of the SpinButton.

I think all three of those should be deprecated and replaced with the standard onChange handler:

onChange?: (event: React.SyntheticEvent<HTMLElement>, newValue: string) => void;

@kkjeer Since you've worked on other issues related to onChange handlers, does that signature look right? The main thing I'm uncertain about is the type of the event parameter--in this case a change could be triggered by up/down buttons, up/down arrows, or typing. Would also be interested if you have any thoughts about the proposed deprecations, in particular onValidate.

[kkjeer]

@ecraig12345 sorry for the delayed response. I think the proposed onChange signature looks fine, although it could probably be made more specific by using React.SyntheticEvent<HTMLInputElement> instead since SpinButton uses <input ... to render.

I also think it makes sense to deprecate onIncrement, onDecrement, and onValidate.

[ecraig12345]

Looked at this some more and I don't see an easy way to combine onChange with the existing event handlers while preserving the current behavior. I may follow an approach similar to what I did for TextField in #7187 where the new behavior is opt-in--though in this case, rather than using a flag, you might opt in just by providing onChange.

[lijunle]

I suppose the request a onChanged callback. I don't want my customized increment/decrement/validate logic, but I want to access the valid value after increment/decrement/validated.

Currently, I am almost duplicating all logic in _defaultOnIncrement/_defaultOnDecrement/_defaultOnValidate to make the parent component manages the value.

<SpinButton
  {...this.props}
  componentRef={setRef}
  onBlur={(ev) => this._onChange(Math.min(minValue, Math.max(maxValue, +ev.target.value)))}
  onIncrement={(value) => this._onChange(Math.min(maxValue, +value + 1))}
  onDecrement={(value) => this._onChange(Math.max(minValue, +value - 1))}
/>

[lijunle]

I try to fix this issue and found some issue. We need to answer some design questions before coding:

• When user input some value and before onBlur, the spin button may be in an invalid state. Do we want to notify onChange with these invalid values?
• If we only when to get the valid values, is it acceptable to notify onChange when onBlur or onButtonClick happens?
• Why do we immediately turn the user's invalid input back to valid number range? Why until onBlur change?

@ecraig12345 Do you have more context about this SpinButton control?

[lijunle]

@ecraig12345 @micahgodbolt

Hello, happy new year! I am going to continue working on this issue.

The idea is to pass a onChange prop callback to SpinButton component. The callback will be called with valid and normalized number value when input blurs or increase/decrease button clicks.

How do you think about this solution?

[micahgodbolt]

I'd defer to @jspurlin about the interface design, but it sounds fine to me.

[jspurlin]

@lijunle what about if the user presses ENTER? Also, what about if the user is typing and then starts spinning the spin button (especially if why they are typing is garbage)? For the increment/decrement would you want to fire onChange on click, should it fire any time it spins (e.g. if someone mouseDowns on one of the buttons)? Also, what about spinning via the keybaord? You could imagine a case where a user types a value, starts spinning with the keyboard and then starts spinning via mouse; the "submit" behavior would be inconsistent when spinning via keybaord vs mouse. It doesn't sound like you are creating an onChange handler because that would fire on any change. It seems like you are wanting to know is some variation of setting a newValue (from validate) or when stop() is called for spinning. Potentially something like onValidValueUpdated. For example, you wouldn't want it to tell you the value was updated it was invalid and reverted to the original value

[lijunle]

what about if the user presses ENTER?

It will trigger the onChange too.

what about if the user is typing and then starts spinning the spin button (especially if why they are typing is garbage)?

When start to spin button, the input box blur events happens and trigger the onChange callback.

It seems like you are wanting to know is some variation of setting a newValue (from validate) or when stop() is called for spinning.

Yes. The usage of this callback is to setState in the parent component to make the component controlled. The current SpinButton is completely uncontrolled.

Potentially something like onValidValueUpdated. For example, you wouldn't want it to tell you the value was updated it was invalid and reverted to the original value

The name onValidValueUpdated is fine. I can change the name to it.

[lijunle]

@jspurlin @micahgodbolt @ecraig12345

I implemented the first version of the callback in #7577 . A new example is added. Could you please help to review it?

I could like to write some unit test for it. What spec should we want? Here is my thoughts:

It should call onValidValueUpdated with normalized value when input box blurs.
It should call onValidValueUpdated when click increase or decrease button.
It should revert to the original valid value if user inputs some random string.

[ecraig12345]

We plan to implement this in version 8--very sorry for the delay!!

[ecraig12345]

Finally coming back to this--I made a detailed spec based on this discussion which we'll be implementing in version 8. https://hackmd.io/cLn40UjfT_KOM6r4yi2CUQ

Here's a dump of the spec as it was originally written (in case it's helpful for reference later if the hackmd goes away). I included a condensed and updated version in the PR.

Intermediate vs. committed values

SpinButton can have two types of values: intermediate and committed.

A value is intermediate at any of the following times:

• User is editing the text field
• (possibly) User is spinning via mouse or keyboard

The value is committed when any of the following happens:

• Blur
• Enter key press (while editing field)
• On each spin (see discussion later)

The commit phase consists of these steps:

1. Call onIncrement, onDecrement, or onValidate as appropriate
2. Only if the value changed, call onChange (if provided)
3. Clear the intermediate value
4. Switch to displaying the committed value:
   • If uncontrolled, update the stored value and display that
   • If controlled, display props.value (which the user likely updated when onChange was called; we do NOT store the value in any other internal state in this case)

Determining and storing the displayed value

Where is the value stored?

• Controlled: use props.value except while editing
• Uncontrolled: uncontrolledValue state value returned by useControllableValue
• While editing: intermediateValue state (or a better name)

What is the initial value? (in order of precedence)

1. props.value (no validation)
2. defaultValue (no validation)
3. min (which defaults to 0)

What is the currently displayed value?

1. intermediateValue if set
2. props.value if provided (controlled)
3. uncontrolledValue (uncontrolled)

What is the value of ISpinButton.value?

1. props.value if provided (controlled)
2. uncontrolledValue (uncontrolled)
3. Probably should NOT ever reflect intermediateValue here

Edge cases:

• If the user is typing and deletes the value (without committing), don't replace it with the previous value or anything else (be careful with falsy checks)
• Possibly: While editing, if the parent re-renders with the same props.value we should retain intermediateValue (remain in edit mode). But if it re-renders with a different props.value, we should clear intermediateValue.

When is onChange called?

onChange should only be called in the commit phase as described above, not on every keystroke or spin.

This is the sequence of events each time onChange is called:

1. If value changed, call onChange
2. If applicable, clear intermediateValue
   • If controlled, this should cause displayed value to revert to props.value, whether or not it's been updated
3. If uncontrolled, update uncontrolledValue

Details of each case where onChange can be called:

Blur

1. Call onValidate
2. onChange sequence (see below)

Enter keypress: same as blur

Arrow button click or spin

1. If the user was already typing (intermediateValue is set):
   1. onBlur should be triggered, which should call onValidate + onChange sequence and likely a value update
   2. Wait for the value to be updated (either setting uncontrolledValue state or the parent updating props.value)
      • Will need to think about how to implement this for controlled: must ensure that if the parent updates props.value based on onChange, we use that new value for the arrow button action
      • (This handles the case where the user types garbage and starts spinning)
2. On each increment, call onIncrement/onDecrement followed by onChange sequence
   • Open question: should we call onChange at this point? (maybe not? though that's harder to implement and validate, and different from previous behavior)
3. (if we decide not to call every spin) After spinning stops, onChange sequence

Arrow key press or spin

1. If the user was already typing (intermediateValue is set):
   1. Call onValidate
   2. onChange sequence
   3. Wait for the value to be updated (same race condition issue as buttons)
2. Same behavior as with buttons

Questions

• Should we ever validate props.value?
  • I think not, to follow pattern of other components?
• Should we validate defaultValue?
  • I think not
• Should onChange be fired for every incremental value while spinning?
  • After discussion with David, yes we should call it every spin. One reason being because it's not uncommon that on every press, something in the app will react (kind of like with a slider) -- like a font size going up and down
  • Previous thoughts:
    • Might prefer not to, but that would be inconsistent with when onIncrement/onDecrement are called and inconsistent with previous behavior. (Also a bit harder to implement.)
      • The reason it's inconsistent with previous behavior is because we'd only update ISpinButton.value at the same times as onChange is called. Right now ISpinButton.value is updated on every spin.
• What should ISpinButton.value return? Probably the committed value, but is there any scenario where we ought to provide access to the intermediate value? (besides tests)
• Should we make the component fully controllable with just onIncrement/onDecrement/onValidate?
  • There are likely some people who have it implemented this way today since that was the only way in the past to have a fully controlled component (verify). But it would probably be hard to make both approaches work simultaneously.

Not supported

• Implementing all of increment/decrement/validate using just onChange. This would cause the onChange signature to be very convoluted (and it would be hard to provide default inc/dec/val handlers).
• Calling onChange for any other reason besides changes from the user (it shouldn't be called on internal validation or updates of props or state)

Notes

• Release notes
  • Providing value now results in a controlled component (probably wasn't the case before; this is one reason it's a breaking change)

Tests (see #7577):

• It should call onChange with normalized value when input box blurs.
• It should call onChange when click increase or decrease button.
• It should revert to the original valid value if user inputs some random string.