Fluentui: 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]

Sorry about not replying on this sooner. I also started working on this bug last month and have a partial fix, but I've been working on a lot of other things since then and will need some time to read through this thread and your change and compare with what I was thinking.

One comment so far, I think we should use the standard name onChange (since it's expected that a form component should have an onChange handler) and note in the comments that it's only called when it's time to validate the value.

Edit: Thinking about it more, properly adding an onChange handler would be much more complex than what you're trying to do (just access the value once it's been validated). So I guess I can see the place for this onValidValueUpdated as an intermediate solution, but the downside is that it's yet another method to deprecate and help people move away from once we have a proper onChange handler and proper controlled/uncontrolled behavior.

[lijunle]

I could say the onValidValueUpdated is a quick fix to let the parent component to align with new validated/normalized value. The change does not turn the SpinButton to be controlled, but it is still very helpful.

For the long term, I am thinking about what is the properly controlled SpinButton behavior. I think we need to distinguish SpinButton into two modes - controlled and uncontrolled.

In uncontrolled mode:

• Key press in SpinButton -> SpinButton setState -> show WYSIWYG in SpinButton
• Key blur in SpinButton -> validate/normalize the showing value -> SpinButton setState to show the validated/normalized value

In controlled mode, the SpinButton should change the value according to parent:

• Key press in SpinButton -> SpinButton setState -> show WYSIWYG in SpinButton
• Key blur in SpinButton -> validate/normalize the showing value -> trigger onChange callback -> parent setState -> SpinButton accept the value and show it

However, the parent may pass invalid value to a SpinButton:

• Parent setState -> pass an invalid value to SpinButton -> SpinButton validate/normalize the passing value ->
  
  
  
  • (If SpinButton is uncontrolled) -> SpinButton setState -> show the validated/normalized passing value
  
  
  • (If SpinButton is controlled) -> ? trigger callback to ask the parent to re-setState with valid value?
  
  

BTW, generally a component is controlled according to if value prop is passed. But if we follow this rule, this will be a breaking change. Another acceptable indicator is to check if onChange passed.

[lijunle]

BTW, the current implementation does not handle correctly on the situation that a parent is passing an invalid value. SpinButton's constructor assumes the value is valid and assigned to _lastValidValue. but the value may be invalid.

https://github.com/OfficeDev/office-ui-fabric-react/blob/d3baf56050d036612bdb6a600b6ca6fbe4516ca6/packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.tsx#L65-L73

[dzearing]

Thank you @lijunle and @ecraig12345 for the background here. I think regarding @jspurlin comments; he's right in that the onChange event might work differently than an input element. It works more like a Dropdown; yes you can arrow through the options without comitting a change, but onChange only fires when a valid state is reached.

Specifically in a controlled scenario:

As you type, it should set an internal state that is is in an editing state, where we allow the internal state to be mismatched with the value passed in. This state is finalized via blur or enter, and the "commit" phase occurs. That is, onChange is fired and the parent can re-render with a new value.

Clicking on the buttons while in an editing state should first commit and then increment. (this might need some thinking.) E.g. typing 10 and clicking up should result in 11.

If the user types an out of bounds value, the onChange fires with the closest in-bounds value. It is still up to the caller to re-render with that new value.

In the edge case that there is no re-render, the value replaces the previous inner state on the next render.

[lijunle]

@dzearing Thanks for your input! I want to make it clear.

As you type, it should set an internal state that is is in an editing state, where we allow the internal state to be mismatched with the value passed in. This state is finalized via blur or enter, and the "commit" phase occurs. That is, onChange is fired and the parent can re-render with a new value.

Suppose a case, the user types "123" in input box. When blurs, the parent receive the "123" but set back "456", the SpinButton will show "456". Correct?

If the user types an out of bounds value, the onChange fires with the closest in-bounds value. It is still up to the caller to re-render with that new value.

When a SpinButon has min=0,max=100. The caller set value with "105". What value will the SpinButton shows? Will onChange be called before showing this value?

[aminezemzemi]

Hi @lijunle, thank you for adding events code to SpinnerButton.
I think that there is a mistake in OnBlur Event.
It should be like this :
onBlur={(ev) => this.onChange(Math.max(minValue, Math.min(maxValue, +ev.target.value)))

[lijunle]

Hi @aminerahmouni, which code do you reference? The current implementation is much more complicated than that.

https://github.com/OfficeDev/office-ui-fabric-react/blob/d3baf56050d036612bdb6a600b6ca6fbe4516ca6/packages/office-ui-fabric-react/src/components/SpinButton/SpinButton.tsx#L257-L263

[dzearing]

Suppose a case, the user types "123" in input box. When blurs, the parent receive the "123" but set back "456", the SpinButton will show "456". Correct?

If 456 is a valid value according to min/max rules, yes, 456 will display.

When a SpinButon has min=0,max=100. The caller set value with "105". What value will the SpinButton shows? Will onChange be called before showing this value?

The SpinButton should fire onChange with 100, asking the caller to re-render with a valid value. Let the SpinButton decide how to interpret the user info into a valid value, and pass that along.

If the last value was 100, and they typed 0000100, the SpinButton should translate this into 100, which isn't different, so no onChange should fire. Likewise if max is 100 and they typed 101, which gets clamped to 100, no change is made and therefore no onChange fires.

At this point, using the component would be quite simple:

Use defaultValue or no defaultValue/value for an uncontrolled component. Monitor onChange calls for new values.

Or, use value, and monitor onChange calls for when you need to re-render with a new value.

There is no need for validation work for the user to do.

[dzearing]

onChange signature should probably be (ev: InputEventArgs, value: number): void

[lijunle]

I had an discussion with @dzearing and made an interesting experiments.

In the codepen, the MyComponent's setState is changing the value and set back. The setting value is different from the controlled component's internal state one. However, the controlled component always respect the passed in value.

Another interesting point is, the input's min/max props are not fixing the parent passing value. We can input value out of range. However, when use up/down arrow key to adjust the value. The value goes back to range.

I have two understandings in the experiment:

1. A controlled component should always respect the value from parent, even the passed in value is incorrect. I don't think we can avoid the parent doing some crazy things to destroy the validated value. Maybe we can warn them but not really necessary.

2. Never trigger onChange callback when parent pass value (during componentWillRecieveProps lifecycle). Event handler is triggered by user event, not program events.

---

There is my proposal:

The SpinButton has two modes - uncontrolled mode and controlled mode - according to if the value prop is passed in. (Yes, this will be a major breaking change on SpinButton!)

In uncontrolled mode:

• The parent passes the defaultValue to initialize the SpinButton's value. If the onChange callback is provided, it will be called when value is committed. The value is committed when blurs or press enter or press up/down arrow key.

In controlled mode:

• The parent passes the value and onChange props. When value is committed, SpinButton calls onChange with validated value to notify parent to setState. If the parent no setState, change the SpinButton value to the previous value prop. When the parent setState to trigger componentWillRecieveProps, always respect the props.value. (It means, in value committed moment, SpinButton always respect and show props.value in its box.)

[lijunle]

Hey @dzearing @ecraig12345 @jspurlin

What are your ideas on my proposal above after data?

[dzearing]

Adding @aneeshack4 .

Uncontrolled sounds good to me.

Minor thought on controlled;

when the user is editing and has not committed the change, and the parent re-renders for whichever reason, If they pass in the exact same value as previous, do nothing. Let the user proceed to edit.

If they pass in a different value, apply the new value and reset the "editing" state to be committed.

The reason is that I think the former is a common case. User is editing and something around the spinbutton needs a re-reneder. We should avoid confusing the user here.

[dzearing]

@aneeshack4 has a similar bug which is that uncontrolled just doesn't work. This component needs work.

[dzearing]

BTW one think I want to make clear;

if the user starts typing and clears it out, we do not replace their text with a "valid value". That is, if they delete everything, it should not replace the text with "0". It's a confusing and annoying behavior. This is what's happening in the codepen you linked to. But it's pretty easy to make sure that's not the case, assuming we have a "non-committed" mode that lets the SpinButton manage the state temporarily while a value is being derived.

[dzearing]

@lijunle Make sense? Any concerns?

[lijunle]

when the user is editing and has not committed the change, and the parent re-renders for whichever reason. If they pass in a different value, apply the new value and reset the "editing" state to be committed.

This makes sense in technical aspect but may be scary for user experience. The user may be struggling with their losing value and losing focus. However, I think the website engineer should fix it, not us - why they are passing a different value async without locking user input. It means, from SpinButton aspect, the behavior is okey.

But it's pretty easy to make sure that's not the case, assuming we have a "non-committed" mode that lets the SpinButton manage the state temporarily while a value is being derived.

I do agree. During the non-committed mode, the user is free to input anything in SpinButton. We do the validation and normalization only when commit the value.

@dzearing I have started my Chinese New Year vacation. There are a couple of family affairs during the new year holiday. I hope to find time to work on this but no promises. I will back on work in Feb 13.

[lijunle]

@dzearing @ecraig12345 @jspurlin I have implemented the onChange prop in SpinButton and necessary refactors in the PR #8060. Could you please help to review?

[ecraig12345]

@lijunle Thanks for all your work on this, and sorry we weren't quicker to review the PR. Before making another PR targeting Fabric 7, would you mind outlining the desired behavior in the Fabric 7 SpinButton to make sure we're all on the same page? (Basically a summary of the issues discussed here and on the PR, keeping in mind that the major version allows us to remove broken/odd behaviors.) Also any props we might want to consider deprecating?

[asith-w]

@ecraig12345 is there any update and is it included in Fabric 7 alpha version ?

[zhr1130]

Any update on it?

[ZhehooMorgen]

This is a very good idea, but any update on it?

[ecraig12345]

At this point we probably won't be implementing it in version 7, but it's something we could consider for version 8 or definitely for the converged SpinButton. We'll be reviewing the backlog of "Fabric Next" issues soon to determine what's actually targeted for version 8, so stay tuned.

[RenzHoly]

2 years passed, no onChange handler yet

[ecraig12345]

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