Carbon: NumberInput cannot be controlled in case an invalid value has been entered directly

[anschoen]

The NumberControl ignores the value property if invalid data has been entered in the input field directly

Detailed description

Using a simple

currentValue = 2;
<NumberInput
   min={0}
   max={10}
   value={currentValue}
   onChange={...}
/>

If a user enters something invalid like ,, into the input field directly, it is not possible to ignore such input in _onChange_ by keeping the currently used value property.
Which means that NumberControl cannot be properly controlled if it gets into _invalid_ state.

Additional information

Code wise this is due to the implementation of

static getDerivedStateFromProps({ min, value }, state) {
    const { prevValue } = state;
    return prevValue === value
      ? null
      : {
          value: isNaN(min) ? value : Math.max(min, value),
          prevValue: value,
        };
  }

This method is the only place where _state.prevValue_ is changed. Given the condition prevValue === value, this method will only ever update the state if prevValue !== value.
Which means it is not possible to ignore input because ignoring means to provide the same value property all the time, which causes prevValue === value to hold.

At the same time

 handleChange = evt => {
    if (!this.props.disabled) {
      evt.persist();
      evt.imaginaryTarget = this._inputRef;
      this.setState(
        {
          value: evt.target.value,
        },
        () => {
          this.props.onChange(evt);
        }
      );
    }
  };

ensures that even invalid data entered into the input field updates the internal state (which cannot be patched from outside due to the finding above).

Expected behavior: It should be possible to enforce a certain value using properties.

[anschoen]

I have found an easy workaround that allows to get rid of the internal state if necessary:

<NumberInput
   key={"KEY_PREFIX_" + this.state.resetCounter}
   ...
/>

I introduce a _key_ property that is incremented (thereby changed) each time a new component instance is needed in order to enforce that my value property actually used (and not ignored due to some internal state).

[emyarod]

hi @anschoen thank you for reporting this! As you pointed out, this issue may be due to our usage of derived state, which I believe stems from our need to provide a fully controlled as well as a fully uncontrolled component. For the time being, I believe we can proceed with your recommendation to use the key attribute to create new instances

on a side note, it may be worth it for us to look into provided separate fully controlled/uncontrolled implementations of our form components, but first we'll have to look into this further

slightly related https://github.com/IBM/carbon-components-react/issues/2106

[asudoh]

Hi @anschoen 👋 thank you for reporting - Dropped the following code into https://codesandbox.io/s/github/IBM/carbon-components-react/tree/master/examples/codesandbox but couldn't reproduce the issue (comma couldn't be entered there):

import React from 'react';
import { render } from 'react-dom';
import { NumberInput } from 'carbon-components-react';

let currentValue = 2;

const App = () => (
  <div>
    <NumberInput
      min={0}
      max={10}
      value={currentValue}
      onChange={evt => {
        console.log('Event:', evt);
      }}
    />
  </div>
);

render(<App />, document.getElementById('root'));

That said, do you want to create a reduced case like that? Thanks!

[yrue]

I have similar problem after input invalid number (ex. not allow empty but change to empty). The error shows and cannot set anything after that.

reproduce:
https://codesandbox.io/embed/codesandbox-44bwm

[asudoh]

@yrue Seems that your onChange passes along NaN to <NumberInput> when it receives NaN from <NumberInput>. Do you want to change the value to a valid number in such case?

[anschoen]

Just enter two , and the component will not handle any input any longer.
Maybe I should add a comment to my initial post. The method

static getDerivedStateFromProps(props, state)

provides the logic that would make sure that the current property value is used as state value.
But due to the condition prevValue === value and the way state.prevValue and state.value are updated, entering invalid content into the field causes the component to get into a state where no updates based on property value happen any longer as long as property value does not change due to the condition this.props.value === state.preValue.
The workaround is to create a new component using a new key value or to artificially change the property value twice in order to enforce the same value.

For example, enforcing new component instances using key to fix the controlled component scenario would be

class App extends Component {
  constructor(props) {
    super(props);
    this.state = {
      numberValue: 0,
      counter: 0
    };
  }
  render = () => (
    <div>
      <NumberInput
        id="TEST"
        key={String(this.state.counter)}
        min={0}
        max={10}
        allowEmpty={false}
        value={this.state.numberValue}
        onChange={evt => {
          const newValue = evt.imaginaryTarget.valueAsNumber;
          this.setState({ numberValue: newValue, counter: ++this.state.counter }, () => {});
        }}
      />
    </div>
  );
}

[asudoh]

@yrue Did you try this.setState({ numberValue: isNaN(newValue) ? 0 : newValue }, () => {}); are your line 22?

[austinbruch]

I am seeing a similar and likely related issue. I've recreated it here: https://codesandbox.io/s/codesandbox-8m68m
Essentially, if you type in -1 in the input field (which is below the configured minimum), the input field shows 0, but the last value used in onChange is -1, leading to an inconsistency in state and view.

[yrue]

@asudoh thanks, I tried your suggestion, and it fix the error, but another issue happens as @austinbruch said.

[asudoh]

Current code does not seem to do min/max capping before onChange() fires. I'd recommend min/max capping yourself given you specify min/max.

[stale[bot]]

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions.

[stale[bot]]

As there's been no activity since this issue was marked as stale, we are auto-closing it.