Fix: use checkValidity() to perform the validation in RangeControl

[jorgefilipecosta]

Description

Fixes:#14319

During the changes I did in #12952, I missed the fact that RangeControl may be used with floats. This created a regression where the component did work as expected when float values are used.
Props to @kadencethemes for reporting this issue in https://github.com/WordPress/gutenberg/pull/12952#issuecomment-470285981.\

This PR proposes an alternative approach where we make use of checkValidity() function instead of redoing our own validation.

How has this been tested?

I pasted the following code block inside the latest posts block to try the component with floats:

const MyRangeControl1 = withState( {
	columns: 2,
} )( ( { columns, setState } ) => (
	<RangeControl
		label="Columns"
		value={ columns }
		onChange={ ( columns ) => setState( { columns } ) }
		min={ 0.1 }
		max={ 0.9 }
		step={ 0.125 }
	/>
) );

const MyRangeControl2 = withState( {
	columns: 2,
} )( ( { columns, setState } ) => (
	<RangeControl
		label="Columns"
		value={ columns }
		onChange={ ( columns ) => setState( { columns } ) }
		max={ 0.9 }
		step={ 0.125 }
	/>
) );

...

	<MyRangeControl1 />
	<MyRangeControl2 />

...

I created the following "codepen" to verify the results of checkValidity() are the ones I expected https://codepen.io/anon/pen/drvgjj?editors=1111.

I checked the columns block continues to work as expected.

[chrisvanpatten]

FYI @kadencethemes opened another option at #14320.

[kadencewp]

I tested this and fixes the issue for me, thanks!

[aduth]

The validation works well. Glad we can lean on a DOM API. TIL about checkValidity!

Not sure what the required change is about, though. Need more context.

[aduth]

Can you explain how this is related to the changes, and why we're suddenly setting required as the default?

[jorgefilipecosta]

Hi @aduth, required ensures that an empty value is treated as false by checkValidity. We are not changing the behavior, previously by default empty values were already treated as invalid:

const newValue = event.target.value;
const newNumericValue = parseInt( newValue, 10 );
isNaN( newNumericValue ) === true

[aduth]

required ensures that an empty value is treated as false by checkValidity. We are not changing the behavior, previously by default empty values were already treated as invalid:

I suppose that's true, though it seems like something which ought to be opted in by the renderer. It feels a bit strange that if I rendered <RangeControl />, the element is immediately considered in a state of being invalid in whichever form its placed. I'm failing to recall why we chose to implement it the way we did, but it seems like an empty value should either be considered explicitly as an additional condition with ! checkValidity || value === '', or signalled immediately as a valid value equivalent to onChange( { value: undefined } ). I think this latter one is what we chose to avoid previously because it made some blocks need to be aware to reset their default attributes?

[jorgefilipecosta]

I suppose that's true, though it seems like something which ought to be opted in by the renderer. It feels a bit strange that if I rendered , the element is immediately considered in a state of being invalid in whichever form its placed.

Hi @aduth, the situation is equivalent to any case where we have a required input field.
The following field is also invalid by default until the user types something.
e.g:

<input type="text" name="usrname" required>

It seems invalid fields at the start before the user types something are a normal and expected situation.
Given the way the input field is used inside the RangeControl it also seems fine that is invalid by default. We give the option to the user to set required to false and make the component valid. But in all cases where we use RangeControl, we want a value so if we don't pass one we are in an invalid state. In most cases, the value comes from an attribute that has a default value so we will not have the range control in an invalid state.

! checkValidity || value === ''

We can follow this approach, do you think it would make sense to still have a flag that allows undefined/empty values?

or signalled immediately as a valid value equivalent to onChange( { value: undefined } ). I think this latter one is what we chose to avoid previously because it made some blocks need to be aware to reset their default attributes?

Exactly, most of the times the value comes from an attribute with a default value, and setting the attribute to undefined would not reset to the default value making the blocks not work as expected. But if we set the default values the result would also not be great. Imagine we value a RangeControl with current value 1, the attribute has a default value of 2, and the user wants to set the value to 3. If we called onChange( { value: undefined } ) as soon as the form was empty, and the default attribute was set when the attribute was undefined, when the user deleted the 1 to type 3, a value of 2 would immediately appear.

[aduth]

The following field is also invalid by default until the user types something.
e.g:

<input type="text" name="usrname" required>

It seems invalid fields at the start before the user types something are a normal and expected situation.

! checkValidity || value === ''

We can follow this approach, do you think it would make sense to still have a flag that allows >undefined/empty values?

To me, the main thing is that required be opt-in. In your first example, you had to literally write required for that behavior to take effect. What we're talking about in the current state of this pull request is that behavior taking effect by the default rendering of a <RangeControl />.

To your second point, I guess it depends if we can make it work where the control would signal undefined when the field is empty, and in order to get around the previous issues we had with block implementations, those usages should be updated to use required (in which case, undefined would not be passed as the value).

Would that work? Is it sensible?

[jorgefilipecosta]

Hi @aduth I updated the code to follow your suggestion, required is now opt-in and in our own usages, we use this option.

[aduth]

Aside: Should this be testing currentInputValue ?

[jorgefilipecosta]

Yes while checking the code it seems currentInputValue should be used. I updated the code.

[aduth]

I've been toying with native inputs to see how well this aligns, and I think this updated direction is sensible. In using the following CodePen, for example, you can see that validity of an empty input is contingent on the input having the required attribute. In other words, an empty value is perfectly valid for a bounded input, if the input doesn't also mark itself as required.

https://codepen.io/aduth/pen/aMYxaB

I've left some feedback which I think should be considered. Behaviorally, it all works well.

Additionally, we should consider what impact this has on developers who may be using RangeInput in their blocks. I note that the implementation included in WordPress 5.1.x is the one prior to the previous revisions in #12952, so as far as validation is concerned, it should still be considered an improvement, not a breaking change?

[aduth]

Is it even possible that newValue have a value of undefined here? I would think this could never possibly be the result of event.target.value.

[jorgefilipecosta]

I checked this documentation page https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement, value has type string and the docs have a not "Note: If the user enters a value different from the value expected, this may return an empty string.". I guess it is safe to conclude that undefined will never be the result of event.target.value. The code was updated.

[aduth]

Do we want to pass the empty string to onChange ? Or should we be passing undefined always here?

[jorgefilipecosta]

Nice catch, given that the value we components users receive is number it does not make any sense to pass an empty string. I updated the code to pass undefined.

[aduth]

Minor:

• Does it even need to be specified as a prop? Isn't it enough that we pass through ...props to the rendered inputs via spread?
  • If we do specify it, does it need to have a default? I don't see any practical impact between the default false and the non-defaulted undefined.
  • If we choose not to specify it, we should remove it from the documentation as well. It's covered well enough by the existing note "Props not included in this set will be applied to the input elements."

[jorgefilipecosta]

I chosen not to specify and removed it from the documentation.

[jorgefilipecosta]

Thank you @aduth for your review and deep analysis of this problem.

Additionally, we should consider what impact this has on developers who may be using RangeInput in their blocks. I note that the implementation included in WordPress 5.1.x is the one prior to the previous revisions in #12952, so as far as validation is concerned, it should still be considered an improvement, not a breaking change?

I guess it is not a breaking change, but the fact that now we have validation may have some impact. I guess the impact is positive considering that the addition of validation solved some bugs in the editor e.g. columns block.