components: Add baseline validation to NumberControl

[sarayourfriend]

What problem does this address?

It's currently possible to input invalid values into the NumberControl: #33285

To prevent this, you currently have to manage that in the onChange you pass. But we should be able to introspect into the min and max values at minimum to create a generic validation function which prevents invalid values from being entered.

What is your proposed solution?

Using the min and max props, verify that the input value is between those numbers, inclusive of both (to match the HTML API).

[stokesman]

To pick up from #33285 (comment):

gutenberg/packages/components/src/input-control/input-field.js

Lines 92 to 109 in ad0cc77

	const handleOnBlur = ( event ) => {
	onBlur( event );
	setIsFocused( false );
	/**
	* If isPressEnterToChange is set, this commits the value to
	* the onChange callback.
	*/
	if ( isPressEnterToChange && isDirty ) {
	wasDirtyOnBlur.current = true;
	if ( ! isValueEmpty( value ) ) {
	handleOnCommit( event );
	} else {
	reset( valueProp );
	}
	}
	};

Notice how it calls handleOnCommit which in turn "commits" the value, causing the NumberControl's reducer to kick in and do the clamping. But it only does this when isPressEnterToChange is true, which is counter intuitive to me. Perhaps that's where the bug is?

I know I've wondered about this very same thing. I suppose that handleOnCommit didn't seem necessary when isPressEnterToChange is false, because the value would have already propagated. It probably wouldn't hurt to always call 'handleOnCommit' from blur events. Yet doing so may not be enough to prevent an issue like #33270 because, as I understand it, that happens as soon as the value propagates. If so, it means the component would have to validate on input and omit propagation for any out-of-range values.

[michalczaplinski]

I wonder if we should also make it more explicit that the value that is passed to the onChange callback is a string rather than a number?

It's quite easy to forget that you should use parseInt to parse the value that is passed to the onChange callback because otherwise you might inadvertently change the value from number to string. I've noticed this in a couple places in the Query Loop block:

gutenberg/packages/block-library/src/query/edit/query-toolbar.js

Lines 64 to 75 in bbaeea2

	onChange={ ( value ) => {
	if (
	isNaN( value ) ||
	value < 1 ||
	value > 100
	) {
	return;
	}
	setQuery( {
	perPage: value,
	} );
	} }

gutenberg/packages/block-library/src/query/edit/query-toolbar.js

Lines 88 to 97 in bbaeea2

	onChange={ ( value ) => {
	if (
	isNaN( value ) ||
	value < 0 ||
	value > 100
	) {
	return;
	}
	setQuery( { offset: value } );
	} }

gutenberg/packages/block-library/src/query/edit/query-toolbar.js

Lines 115 to 120 in bbaeea2

	onChange={ ( value ) => {
	if ( isNaN( value ) || value < 0 ) {
	return;
	}
	setQuery( { pages: value } );
	} }

Loom video to explain in a bit more detail: https://www.loom.com/share/c7cf93b582fa4350b8562242adfa9133

---

One suggested solution could be to pass values as both string and number to the onChange callback. This is what Chakra UI is doing for example:

[ciampo]

Opened a tentative fix for this in #39186

I wonder if we should also make it more explicit that the value that is passed to the onChange callback is a string rather than a number?

Re. this suggestion, I made a note and I'm going to consider it when I'll resume the work on refactoring NumberControl to TypeScript (#38753)