TextField: Call onChange when cleared

[doytch]

Ran into an interesting bug when fixing a separate bug today.

When you hit the clear icon (X) at the end of a TextField, React Final Form (and I believe Redux Form as well) don't actually get notified of this. onClearField is called, but that's not provided by either of the form libraries.

Note how onChange is called in TextField.handleChange. In handleClear, we use the pass-value way of calling onChange because otherwise we'd have to persist the event to have it available in the setState callback.

[zburke]

This makes sense to me. Thanks for the careful explanation here, and for the fix!

That said: I am the least-expert person when it comes to the internal workings of our components and I'd love to get @JohnC-80's eyes on this before committing it just in case there is some special case where passing an empty string to onChange kills a puppy or something similar.

[rasmuswoelk]

Looks good to me. But I think @JohnC-80's suggestion would make sense. And we could maybe even allow for the onClearField-callback passed as a prop to still work.

Maybe pass in the input-prop as a parameter if a developer has passed the callback to a react-final-form-field. And if not; just default to resetting the value.

onClearField: typeof onClearField === 'function' ? () => onClearField(input) : () => input.onChange(''),

[doytch]

@JohnC-80, I think your suggestion breaks supplying an onClearField though. Because then, the parent has to notify RF/RFF about the change. And they don't have the ability to (easily) do that since they don't have access to the input prop.

@rasmuswoelk's way of passing the input to the onClearField is a start, but it'd technically have to be the onChange since input gets destructured in FormField.

So really, I think we'd have to change the API so that it's onClearField(onChange). But then developers need to call onChange in their callback which feels like we're bleeding responsibilities across component levels. Why is a parent (eg, form section) having to call onChange for a child Field when that's not a pattern we use anywhere else?

[JohnC-80]

Yeah, I definitely don't like having to call onChange('') again to handle the change.

The original approach that you have, calling onChange(value) breaks conventional use of onChange with an event - it works with RF/RFF, but it's a concession that form-state-management libs have made to cope with the way the react component universe likes to do these things sometimes and it makes situations like this particularly ugly to deal with and maintain compatibility.

[JohnC-80]

You're right, @doytch my original suggestion wouldn't work anyways since if the app was using the onClearField prop it would get overwritten when the spread is applied in formField. Gotta be a cleaner way...

We could apply the config last in formField, see if it that breaks anything... and pass props through to the config so that it could account for props from the app.

[doytch]

Makes sense, @JohnC-80.

I've pushed up a change that just persists React's synthetic element and passes it along instead of using the pass-value shorthand. Also works 👍

[JohnC-80]

I can handle this. I had reserve that the input wouldn't contain the '' value yet when this onChange was called... but if it works, it works!

[doytch]

I can handle this. I had reserve that the input wouldn't contain the '' value yet when this onChange was called... but if it works, it works!

Double-checked the API; apparently React guarantees the setState callback will be fired not just after the update is applied, but also after the component has re-rendered.

[doytch]

That said, I went ahead and traced through the execution. The event target is actually the clear button. So this actually works by virtue of the fact that a button doesn't have a value.

Soooo, yeah.

[JohnC-80]

🤣 and 😭

[JohnC-80]

Give this branch a go...
https://github.com/folio-org/stripes-components/tree/native-textfield-clearing

In short, this changes the value of the input natively and then dispatches the change event, which catches the handleChange callback and goes with the flow that this component already has in place. Actual events, no spoofing, no imposters, no overloaded onChange for values. This should work fine with React Final Form and Redux Form... This reduces, possibly eliminates the necessity of the onClearField prop.

This has been the adopted approach in a few places, by a few people but I wouldn't want to do this all the time.

In this branch, the clear functionality is handled as follows:

  clearField() {
    const { onClearField } = this.props;

    // Fire callback
    if (typeof onClearField === 'function') {
      onClearField();
    }

    // Clear value on input natively, dispatch an event to be picked up by handleChange, and focus on input again
    if (this.input.current) {
      const nativeInputValueSetter = Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype, 'value').set;
      nativeInputValueSetter.call(this.input.current, '');

      const ev = new Event('change', { bubbles: true });
      this.input.current.dispatchEvent(ev);
      this.input.current.focus();
    }
  }

Previously, we were changing the state and attempting to dispatch within a callback. I believe this didn't work because React does a little bit of reducing/batching/deduping when it comes to change event handling. Since the state was updated, the field was re-rendered with the '' value already present, when the dispatched change came through, react-dom determined that it didn't need to happen since the field already had the same value.

Additionally, the long-windedness of that whole nativeInputValueSetter is necessary since you apparently cannot set the value of an input through the ref/DOM node anymore (this.input.current.value=foo).

[doytch]

That looks like it works well. Tested in RFF (Agreements), RF (Users), and uncontrolled inputs (SearchField). My initial squeemishness remains in place, but the amount of people deploying this workaround and underlying logic leads me to believe we're relatively safe with it.

Want me to update this PR with that option or do you have a branch going that you want to use? Diff leads me to believe you may have some tests that you wrote already?

[JohnC-80]

Go ahead and update this PR.. I think the workspace I was using for the other might have been stale. I haven't added any tests. I assume existing tests for clearing a TextField would still run/pass as usual so the coverage remains.