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

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

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?

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.

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?

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

Aside: Should this be testing currentInputValue ?

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

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.

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.

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

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.

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."

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