SpinButton onChange receives extra events because event handler is passed to wrapper div

jtbandes commented 3 years ago

An onChange passed to SpinButton is incorrectly passed to a wrapper div. The result is that React delivers the onchange event to the noOp function and also to the user's onChange handler, which was supposed to be called only via useControllableValue.

The onChange is not excluded from native props here:

https://github.com/microsoft/fluentui/blob/e0b7d62f54c1b6e7ff8847ec60307ebac692ff7e/packages/react/src/components/SpinButton/SpinButton.base.tsx#L185-L189

cc @ecraig12345 as SpinButton onChange author 🙂

Environment Information

Package version(s): @fluentui/react: 8.14.5
Browser and OS versions: Chrome / macOS

Please provide a reproduction of the bug in a codepen:

https://codesandbox.io/s/proud-shape-fjhzc?file=/src/index.tsx

Open console and type in the SpinButton input field.

Actual behavior:

console.log is called with "value = undefined" while typing, and later with the final correct value when the user commits the edit.

onChange is called with undefined while typing because the event listener is added on the wrapper div, and the noOp handler on the input does not stopPropagation().

Expected behavior:

console.log should not show "value = undefined"; onChange should not be passed to wrapper div.

Doc comment says:

/**
 * Callback for when the committed/validated value changes. This is called *after* `onIncrement`,
 * `onDecrement`, or `onValidate`, on the following events:
 * - User presses the up/down buttons (on single press or every spin)
 * - User presses the up/down arrow keys (on single press or every spin)
 * - User *commits* edits to the input text by focusing away (blurring) or pressing enter.
 *   Note that this is NOT called for every key press while the user is editing.
 */
onChange?: (event: React.SyntheticEvent<HTMLElement>, newValue?: string) => void;

Priorities and help requested:

Are you willing to submit a PR to fix? Maybe, depending on the complexity of the correct fix

jtbandes commented 3 years ago

Related issue: the TypeScript typings for nullability of the event parameter are incorrect — note that useControllableValue calls onChange with ev!, assuming it is not undefined:

https://github.com/microsoft/fluentui/blob/e0b7d62f54c1b6e7ff8847ec60307ebac692ff7e/packages/react-hooks/src/useControllableValue.ts#L66

Where ev may in fact be undefined (note no 2nd argument is passed):

https://github.com/microsoft/fluentui/blob/e0b7d62f54c1b6e7ff8847ec60307ebac692ff7e/packages/react/src/components/SpinButton/SpinButton.base.tsx#L205-L206

ecraig12345 commented 3 years ago

Thanks for reporting this! @TristanWatanabe I the first issue is because onChange is missing from the list here. (While working on this, you might also check for any other native props that are already being applied elsewhere.) https://github.com/microsoft/fluentui/blob/e0b7d62f54c1b6e7ff8847ec60307ebac692ff7e/packages/react/src/components/SpinButton/SpinButton.base.tsx#L185-L189

jtbandes commented 3 years ago

Thanks for the quick response!

I'm noticing some other usability issues as I work with SpinButton — I'll drop some notes here in case it is helpful (not sure these are fully-formed enough to file separate tickets for).

onChange is not called on blur if the field is empty (enteredValue === ""). The use case here is that I'd like to allow the field to be empty, which my app will translate to a default value. It looks like I can work around this by providing a custom onValidate that returns the empty string instead of undefined.
TypeScript thinks the placeholder prop is available on SpinButton, but it doesn't actually work. inputProps={{placeholder: ...}} does work though.
It's a bit confusing that TextField has onGetErrorMessage, while SpinButton has onValidate. TextField seems to run validation for every keystroke, while SpinButton only runs it on blur/enter. It would be nice if SpinButton were basically "TextField, but for numbers" so they should have a largely similar API.
I'd like some way to receive updates on each keystroke (so the user can see the effect of their change as they're typing in the field), but there's no onInput prop.
When the label is too long and wraps onto the next line, it butts right up against the input field with no spacing in between:

jtbandes commented 3 years ago

Thank you @TristanWatanabe! For the other feedback above, would you prefer if I filed separate tickets for these requests?

msft-fluent-ui-bot commented 3 years ago

:tada:This issue was addressed in #18221, which has now been successfully released as @fluentui/react@v8.14.14.:tada:

Handy links:

ecraig12345 commented 3 years ago

@jtbandes Sorry for the delayed response! Answering some of your questions:

onChange is not called on blur if the field is empty (enteredValue === ""). The use case here is that I'd like to allow the field to be empty, which my app will translate to a default value. It looks like I can work around this by providing a custom onValidate that returns the empty string instead of undefined.

I think right now, SpinButton is not intended to allow an empty value. This one would be worth filing a separate issue to discuss further (please tag me if you do that).

TypeScript thinks the placeholder prop is available on SpinButton, but it doesn't actually work. inputProps={{placeholder: ...}} does work though.

ISpinButtonProps extends React.HTMLAttributes, which for some reason includes placeholder and various other form field props (defaultValue, defaultChecked) even though they won't work on all elements. This one might also be worth filing an issue about, since it's not totally obvious how it ought to be handled.

It's a bit confusing that TextField has onGetErrorMessage, while SpinButton has onValidate. TextField seems to run validation for every keystroke, while SpinButton only runs it on blur/enter. It would be nice if SpinButton were basically "TextField, but for numbers" so they should have a largely similar API.

Agreed that the difference in validation callback names is confusing--this is one example of where the library kind of evolved over time without entirely intentional/consistent design in all areas. It's not something we're likely to fix in version 8 at this point, but we'll definitely try to make this better for the new "converged" components we're currently working on.

I'd like some way to receive updates on each keystroke (so the user can see the effect of their change as they're typing in the field), but there's no onInput prop.

You could also make a feature request for this, though it's unlikely anyone from the team would get to it any time soon. It might need a different name though--onInput and onChange actually work the same in React, at least for <input> (this is different from how the native browser events work), so it's probably best to avoid that name for custom callbacks.

When the label is too long and wraps onto the next line, it butts right up against the input field with no spacing in between:

I'm guessing you could work around this by adding style overrides, or possibly file an issue about improving this (though again it likely wouldn't be high priority).

microsoft / fluentui