Fix NumberControl validation

packages/components/src/input-control/index.tsx

@@ -25,9 +25,11 @@ function useUniqueId( idProp?: string ) {
 	return idProp || id;
 }
 
+const passThrough = ( state: any ) => state;
+
 export function InputControl(
 	{
-		__unstableStateReducer: stateReducer = ( state ) => state,
+		__unstableStateReducer: stateReducer = passThrough,
 		__unstableInputWidth,
 		className,
 		disabled = false,

packages/components/src/input-control/input-field.tsx

@@ -47,7 +47,7 @@ function InputField(
 		onValidate = noop,
 		size = 'default',
 		setIsFocused,
-		stateReducer = ( state: any ) => state,
+		stateReducer,
 		value: valueProp,
 		type,
 		...props
@@ -75,42 +75,42 @@ function InputField(
 		isPressEnterToChange,
 	} );
 
-	const { _event, value, isDragging, isDirty } = state;
-	const wasDirtyOnBlur = useRef( false );
+	const { _event, value, isDragging, isDirty, error } = state;
+	const wasPendentOnBlur = useRef( false );
 
 	const dragCursor = useDragCursor( isDragging, dragDirection );
 
 	/*
-	 * Handles synchronization of external and internal value state.
-	 * If not focused and did not hold a dirty value[1] on blur
-	 * updates the value from the props. Otherwise if not holding
-	 * a dirty value[1] propagates the value and event through onChange.
-	 * [1] value is only made dirty if isPressEnterToChange is true
+	 * Handles switching modes between controlled – while not focused – and
+	 * uncontrolled – while focused. Exceptions are made when the internal
+	 * value is either dirty or invalid. In such cases, the value does not
+	 * propagate while focused and the first render while not focused ignores
+	 * the value from props and instead propagates the internal value.
 	 */
 	useUpdateEffect( () => {
 		if ( valueProp === value ) {
 			return;
 		}
-		if ( ! isFocused && ! wasDirtyOnBlur.current ) {
+		if ( ! isFocused && ! wasPendentOnBlur.current ) {
 			update( valueProp, _event as SyntheticEvent );
-		} else if ( ! isDirty ) {
+		} else if ( ! isDirty && ! error ) {
 			onChange( value, {
 				event: _event as ChangeEvent< HTMLInputElement >,
 			} );
-			wasDirtyOnBlur.current = false;
+			wasPendentOnBlur.current = false;
 		}
-	}, [ value, isDirty, isFocused, valueProp ] );
+	}, [ value, isDirty, isFocused, valueProp, error ] );
 
 	const handleOnBlur = ( event: FocusEvent< HTMLInputElement > ) => {
 		onBlur( event );
 		setIsFocused?.( false );
 
 		/**
-		 * If isPressEnterToChange is set, this commits the value to
-		 * the onChange callback.
+		 * Commits the value in cases where it has not yet propagated. In case
+		 * of an error the commit will resolve the value before propagation.
 		 */
-		if ( isPressEnterToChange && isDirty ) {
-			wasDirtyOnBlur.current = true;
+		if ( isDirty || error ) {
+			wasPendentOnBlur.current = true;
 			handleOnCommit( event );
 		}
 	};
@@ -120,20 +120,31 @@ function InputField(
 		setIsFocused?.( true );
 	};
 
+	const validateAction = (
+		inputAction: ( nextValue: string, event: SyntheticEvent ) => void,
+		nextValue: string,
+		event: SyntheticEvent< HTMLInputElement >
+	) => {
+		try {
+			onValidate( nextValue, event );
+			inputAction( nextValue, event );
+		} catch ( err ) {
+			invalidate( err, event );
+		}
+	};
+
 	const handleOnChange = ( event: ChangeEvent< HTMLInputElement > ) => {
 		const nextValue = event.target.value;
-		change( nextValue, event );
+		if ( isPressEnterToChange ) {
+			change( nextValue, event );
+		} else {
+			validateAction( change, nextValue, event );
+		}
 	};
 
 	const handleOnCommit = ( event: SyntheticEvent< HTMLInputElement > ) => {
 		const nextValue = event.currentTarget.value;
-
-		try {
-			onValidate( nextValue );
-			commit( nextValue, event );
-		} catch ( err ) {
-			invalidate( err, event );
-		}
+		validateAction( commit, nextValue, event );
 	};
 
 	const handleOnKeyDown = ( event: KeyboardEvent< HTMLInputElement > ) => {

packages/components/src/input-control/reducer/reducer.ts

@@ -7,7 +7,7 @@ import type { SyntheticEvent } from 'react';
 /**
  * WordPress dependencies
  */
-import { useReducer } from '@wordpress/element';
+import { useCallback, useReducer } from '@wordpress/element';
 
 /**
  * Internal dependencies
@@ -166,8 +166,11 @@ export function useInputControlStateReducer(
 	stateReducer: StateReducer = initialStateReducer,
 	initialState: Partial< InputState > = initialInputControlState
 ) {
+	const reducer = useCallback( inputControlStateReducer( stateReducer ), [
+		stateReducer,
+	] );
 	const [ state, dispatch ] = useReducer< StateReducer >(
-		inputControlStateReducer( stateReducer ),
+		reducer,
 		mergeInitialState( initialState )
 	);
 

packages/components/src/number-control/index.js

@@ -7,7 +7,7 @@ import classNames from 'classnames';
 /**
  * WordPress dependencies
  */
-import { forwardRef } from '@wordpress/element';
+import { forwardRef, useCallback } from '@wordpress/element';
 import { isRTL } from '@wordpress/i18n';
 
 /**
@@ -19,53 +19,35 @@ import { composeStateReducers } from '../input-control/reducer/reducer';
 import { add, subtract, roundClamp } from '../utils/math';
 import { isValueEmpty } from '../utils/values';
 
-export function NumberControl(
-	{
-		__unstableStateReducer: stateReducer = ( state ) => state,
-		className,
-		dragDirection = 'n',
-		hideHTMLArrows = false,
-		isDragEnabled = true,
-		isShiftStepEnabled = true,
-		label,
-		max = Infinity,
-		min = -Infinity,
-		required = false,
-		shiftStep = 10,
-		step = 1,
-		type: typeProp = 'number',
-		value: valueProp,
-		...props
-	},
-	ref
-) {
-	const isStepAny = step === 'any';
-	const baseStep = isStepAny ? 1 : parseFloat( step );
-	const baseValue = roundClamp( 0, min, max, baseStep );
-	const constrainValue = ( value, stepOverride ) => {
-		// When step is "any" clamp the value, otherwise round and clamp it.
-		return isStepAny
-			? Math.min( max, Math.max( min, value ) )
-			: roundClamp( value, min, max, stepOverride ?? baseStep );
-	};
-
-	const autoComplete = typeProp === 'number' ? 'off' : null;
-	const classes = classNames( 'components-number-control', className );
-
-	/**
-	 * "Middleware" function that intercepts updates from InputControl.
-	 * This allows us to tap into actions to transform the (next) state for
-	 * InputControl.
-	 *
-	 * @param {Object} state  State from InputControl
-	 * @param {Object} action Action triggering state change
-	 * @return {Object} The updated state to apply to InputControl
-	 */
-	const numberControlStateReducer = ( state, action ) => {
+// Creates a state reducer for specialization of InputControl.
+const useNumberControlStateReducer = ( props ) => {
+	const {
+		dragDirection,
+		isDragEnabled,
+		isShiftStepEnabled,
+		min,
+		max,
+		required,
+		shiftStep,
+		step,
+	} = props;
+
+	return useCallback( ( state, action ) => {
 		const { type, payload } = action;
-		const event = payload?.event;
+		const event = state._event;
 		const currentValue = state.value;
 
+		const isStepAny = step === 'any';
+		const baseStep = isStepAny ? 1 : parseFloat( step );
+		const baseValue = roundClamp( 0, min, max, baseStep );
+
+		const constrainValue = ( value, stepOverride ) => {
+			// When step is "any" clamp the value, otherwise round and clamp it.
+			return isStepAny
+				? Math.min( max, Math.max( min, value ) )
+				: roundClamp( value, min, max, stepOverride ?? baseStep );
+		};
+
 		/**
 		 * Handles custom UP and DOWN Keyboard events
 		 */
@@ -147,21 +129,82 @@ export function NumberControl(
 		}
 
 		/**
-		 * Handles commit (ENTER key press or on blur if isPressEnterToChange)
+		 * Handles ENTER key press or commits. The latter originates from blur
+		 * events or ENTER key presses when isPressEnterToChange is true).
 		 */
 		if (
-			type === inputControlActionTypes.PRESS_ENTER ||
+			( type === inputControlActionTypes.PRESS_ENTER &&
+				! state.isPressEnterToChange ) ||
 			type === inputControlActionTypes.COMMIT
 		) {
 			const applyEmptyValue = required === false && currentValue === '';
 
 			state.value = applyEmptyValue
 				? currentValue
 				: constrainValue( currentValue );
+
+			state.error = null;
+		}
+
+		/**
+		 * Handles changes when isPressEnterToChange is false in order to skip
+		 * propagation of invalid values through onChange.
+		 */
+		if (
+			type === inputControlActionTypes.CHANGE &&
+			! state.isPressEnterToChange
+		) {
+			// console.log('NC', event.target.value, state.value )
+			const { valid } = event.target.validity;
+			if ( ! valid ) {
+				state.error = 'invalid';
+			}
 		}
 
 		return state;
-	};
+	}, Object.keys( props ) );
+};
+
+const passThrough = ( state ) => state;
+
+export function NumberControl(
+	{
+		__unstableStateReducer: stateReducer = passThrough,
+		className,
+		dragDirection = 'n',
+		hideHTMLArrows = false,
+		isDragEnabled = true,
+		isShiftStepEnabled = true,
+		label,
+		max = Infinity,
+		min = -Infinity,
+		required = false,
+		shiftStep = 10,
+		step = 1,
+		type: typeProp = 'number',
+		value: valueProp,
+		...props
+	},
+	ref
+) {
+	const autoComplete = typeProp === 'number' ? 'off' : null;
+	const classes = classNames( 'components-number-control', className );
+
+	const numberControlStateReducer = useNumberControlStateReducer( {
+		dragDirection,
+		isDragEnabled,
+		isShiftStepEnabled,
+		min,
+		max,
+		required,
+		shiftStep,
+		step,
+	} );
+
+	const reducer = useCallback(
+		composeStateReducers( numberControlStateReducer, stateReducer ),
+		[ numberControlStateReducer, stateReducer ]
+	);
 
 	return (
 		<Input
@@ -175,15 +218,13 @@ export function NumberControl(
 			label={ label }
 			max={ max }
 			min={ min }
+			// onValidate={ onValidate }
 			ref={ ref }
 			required={ required }
 			step={ step }
 			type={ typeProp }
 			value={ valueProp }
-			__unstableStateReducer={ composeStateReducers(
-				numberControlStateReducer,
-				stateReducer
-			) }
+			__unstableStateReducer={ reducer }
 		/>
 	);
 }

packages/components/src/number-control/test/index.js

@@ -60,9 +60,27 @@ describe( 'NumberControl', () => {
 			const input = getInput();
 			input.focus();
 			fireEvent.change( input, { target: { value: 10 } } );
-
 			expect( spy ).toHaveBeenCalledWith( '10' );
 		} );
+
+		it( 'should not call onChange callback with invalid values', () => {
+			const spy = jest.fn();
+
+			render(
+				<NumberControl
+					value={ 5 }
+					min={ 2 }
+					max={ 10 }
+					onChange={ ( v ) => spy( v ) }
+				/>
+			);
+
+			const input = getInput();
+			input.focus();
+			fireEvent.change( input, { target: { value: 1 } } );
+			expect( input.value ).toBe( '1' );
+			expect( spy ).not.toHaveBeenCalled();
+		} );
 	} );
 
 	describe( 'Validation', () => {
@@ -78,10 +96,20 @@ describe( 'NumberControl', () => {
 			 * This is zero because the value has been adjusted to
 			 * respect the min/max range of the input.
 			 */
-
 			expect( input.value ).toBe( '0' );
 		} );
 
+		it( 'should clamp value within range on blur', () => {
+			render( <NumberControl value={ 0 } min={ -5 } max={ 5 } /> );
+
+			const input = getInput();
+			input.focus();
+			fireEvent.change( input, { target: { value: -10 } } );
+			input.blur();
+
+			expect( input.value ).toBe( '-5' );
+		} );
+
 		it( 'should parse to number value on ENTER keypress when required', () => {
 			render( <NumberControl value={ 5 } required={ true } /> );