diff --git a/packages/components/src/input-control/reducer/actions.ts b/packages/components/src/input-control/reducer/actions.ts index d66eb5778aad8..343e652b2190c 100644 --- a/packages/components/src/input-control/reducer/actions.ts +++ b/packages/components/src/input-control/reducer/actions.ts @@ -20,7 +20,7 @@ export const PRESS_UP = 'PRESS_UP'; export const RESET = 'RESET'; interface EventPayload { - event?: SyntheticEvent; + event: SyntheticEvent; } interface Action< Type, ExtraPayload = {} > { diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 478b99873f5c5..7521a567981c1 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -52,6 +52,14 @@ function inputControlStateReducer( composedStateReducers: StateReducer ): StateReducer { return ( state, action ) => { + // Updates state and returns early when there's no action type. These + // are controlled updates and need no exposure to additional reducers. + if ( ! ( 'type' in action ) ) { + return { + ...state, + value: `${ action.value ?? '' }`, + }; + } const nextState = { ...state }; switch ( action.type ) { @@ -98,7 +106,7 @@ function inputControlStateReducer( case actions.RESET: nextState.error = null; nextState.isDirty = false; - nextState.value = action.payload.value || state.initialValue; + nextState.value = action.payload.value ?? state.initialValue; break; /** @@ -109,10 +117,6 @@ function inputControlStateReducer( break; } - if ( action.payload.event ) { - nextState._event = action.payload.event; - } - /** * Send the nextState + action to the composedReducers via * this "bridge" mechanism. This allows external stateReducers @@ -151,15 +155,7 @@ export function useInputControlStateReducer( nextValue: actions.ChangeEventAction[ 'payload' ][ 'value' ], event: actions.ChangeEventAction[ 'payload' ][ 'event' ] ) => { - /** - * Persist allows for the (Synthetic) event to be used outside of - * this function call. - * https://reactjs.org/docs/events.html#event-pooling - */ - if ( event && event.persist ) { - event.persist(); - } - + refEvent.current = event; dispatch( { type, payload: { value: nextValue, event }, @@ -169,21 +165,14 @@ export function useInputControlStateReducer( const createKeyEvent = ( type: actions.KeyEventAction[ 'type' ] ) => ( event: actions.KeyEventAction[ 'payload' ][ 'event' ] ) => { - /** - * Persist allows for the (Synthetic) event to be used outside of - * this function call. - * https://reactjs.org/docs/events.html#event-pooling - */ - if ( event && event.persist ) { - event.persist(); - } - + refEvent.current = event; dispatch( { type, payload: { event } } ); }; const createDragEvent = ( type: actions.DragEventAction[ 'type' ] ) => ( payload: actions.DragEventAction[ 'payload' ] ) => { + refEvent.current = payload.event; dispatch( { type, payload } ); }; @@ -191,8 +180,10 @@ export function useInputControlStateReducer( * Actions for the reducer */ const change = createChangeEvent( actions.CHANGE ); - const invalidate = ( error: unknown, event: SyntheticEvent ) => + const invalidate = ( error: unknown, event: SyntheticEvent ) => { + refEvent.current = event; dispatch( { type: actions.INVALIDATE, payload: { error, event } } ); + }; const reset = createChangeEvent( actions.RESET ); const commit = createChangeEvent( actions.COMMIT ); @@ -206,17 +197,19 @@ export function useInputControlStateReducer( const currentState = useRef( state ); const currentValueProp = useRef( initialState.value ); + const refEvent = useRef< SyntheticEvent | null >( null ); useLayoutEffect( () => { currentState.current = state; currentValueProp.current = initialState.value; } ); useLayoutEffect( () => { if ( + refEvent.current && state.value !== currentValueProp.current && ! currentState.current.isDirty ) { onChangeHandler( state.value ?? '', { - event: currentState.current._event as + event: refEvent.current as | ChangeEvent< HTMLInputElement > | PointerEvent< HTMLInputElement >, } ); @@ -227,11 +220,9 @@ export function useInputControlStateReducer( initialState.value !== currentState.current.value && ! currentState.current.isDirty ) { - reset( - initialState.value, - currentState.current._event as SyntheticEvent - ); + dispatch( { value: initialState.value } ); } + if ( refEvent.current ) refEvent.current = null; }, [ initialState.value ] ); return { diff --git a/packages/components/src/input-control/reducer/state.ts b/packages/components/src/input-control/reducer/state.ts index be7dd3547300b..c7201d12a9df7 100644 --- a/packages/components/src/input-control/reducer/state.ts +++ b/packages/components/src/input-control/reducer/state.ts @@ -9,22 +9,24 @@ import type { Reducer } from 'react'; import type { InputAction } from './actions'; export interface InputState { - _event: Event | {}; error: unknown; - initialValue?: string; + initialValue: string; isDirty: boolean; isDragEnabled: boolean; isDragging: boolean; isPressEnterToChange: boolean; - value?: string; + value: string; } -export type StateReducer = Reducer< InputState, InputAction >; +export type StateReducer = Reducer< + InputState, + InputAction | Partial< InputState > +>; +export type SecondaryReducer = Reducer< InputState, InputAction >; export const initialStateReducer: StateReducer = ( state: InputState ) => state; export const initialInputControlState: InputState = { - _event: {}, error: null, initialValue: '', isDirty: false, diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js index fb64ec8ea9b6c..4018d37cfadc0 100644 --- a/packages/components/src/range-control/index.js +++ b/packages/components/src/range-control/index.js @@ -18,8 +18,8 @@ import { useInstanceId } from '@wordpress/compose'; import BaseControl from '../base-control'; import Button from '../button'; import Icon from '../icon'; -import { COLORS } from '../utils'; -import { floatClamp, useControlledRangeValue } from './utils'; +import { COLORS, useControlledState } from '../utils'; +import { useUnimpededRangedNumberEntry } from './utils'; import InputRange from './input-range'; import RangeRail from './rail'; import SimpleTooltip from './tooltip'; @@ -70,13 +70,10 @@ function RangeControl( }, ref ) { - const [ value, setValue ] = useControlledRangeValue( { - min, - max, - value: valueProp, - initial: initialPosition, - } ); const isResetPendent = useRef( false ); + const [ value, setValue ] = useControlledState( valueProp, { + fallback: null, + } ); if ( step === 'any' ) { // The tooltip and number input field are hidden when the step is "any" @@ -102,15 +99,15 @@ function RangeControl( const isThumbFocused = ! disabled && isFocused; const isValueReset = value === null; - const currentValue = value !== undefined ? value : currentInput; + const usedValue = isValueReset + ? resetFallbackValue ?? initialPosition + : value ?? currentInput; - const inputSliderValue = isValueReset ? '' : currentValue; - - const rangeFillValue = isValueReset ? ( max - min ) / 2 + min : value; - - const calculatedFillValue = ( ( value - min ) / ( max - min ) ) * 100; - const fillValue = isValueReset ? 50 : calculatedFillValue; - const fillValueOffset = `${ clamp( fillValue, 0, 100 ) }%`; + const fillPercent = `${ + usedValue === null || usedValue === undefined + ? 50 + : ( ( clamp( usedValue, min, max ) - min ) / ( max - min ) ) * 100 + }%`; const classes = classnames( 'components-range-control', className ); @@ -129,23 +126,20 @@ function RangeControl( onChange( nextValue ); }; - const handleOnChange = ( nextValue ) => { - nextValue = parseFloat( nextValue ); - setValue( nextValue ); - /* - * Calls onChange only when nextValue is numeric - * otherwise may queue a reset for the blur event. - */ - if ( ! isNaN( nextValue ) ) { - if ( nextValue < min || nextValue > max ) { - nextValue = floatClamp( nextValue, min, max ); + const someNumberInputProps = useUnimpededRangedNumberEntry( { + max, + min, + value: usedValue ?? '', + onChange: ( nextValue ) => { + if ( ! isNaN( nextValue ) ) { + setValue( nextValue ); + onChange( nextValue ); + isResetPendent.current = false; + } else if ( allowReset ) { + isResetPendent.current = true; } - onChange( nextValue ); - isResetPendent.current = false; - } else if ( allowReset ) { - isResetPendent.current = true; - } - }; + }, + } ); const handleOnInputNumberBlur = () => { if ( isResetPendent.current ) { @@ -155,30 +149,20 @@ function RangeControl( }; const handleOnReset = () => { - let resetValue = parseFloat( resetFallbackValue ); - let onChangeResetValue = resetValue; + const resetValue = parseFloat( resetFallbackValue ); if ( isNaN( resetValue ) ) { - resetValue = null; - onChangeResetValue = undefined; + setValue( null ); + /* + * If the value is reset without a resetFallbackValue, the onChange + * callback receives undefined as that was the behavior when the + * component was stablized. + */ + onChange( undefined ); + } else { + setValue( resetValue ); + onChange( resetValue ); } - - setValue( resetValue ); - - /** - * Previously, this callback would always receive undefined as - * an argument. This behavior is unexpected, specifically - * when resetFallbackValue is defined. - * - * The value of undefined is not ideal. Passing it through - * to internal elements would change it from a - * controlled component to an uncontrolled component. - * - * For now, to minimize unexpected regressions, we're going to - * preserve the undefined callback argument, except when a - * resetFallbackValue is defined. - */ - onChange( onChangeResetValue ); }; const handleShowTooltip = () => setShowTooltip( true ); @@ -197,7 +181,7 @@ function RangeControl( }; const offsetStyle = { - [ isRTL() ? 'right' : 'left' ]: fillValueOffset, + [ isRTL() ? 'right' : 'left' ]: fillPercent, }; return ( @@ -235,7 +219,7 @@ function RangeControl( onMouseLeave={ onMouseLeave } ref={ setRef } step={ step } - value={ inputSliderValue } + value={ usedValue ?? '' } /> @@ -285,13 +269,10 @@ function RangeControl( disabled={ disabled } inputMode="decimal" isShiftStepEnabled={ isShiftStepEnabled } - max={ max } - min={ min } onBlur={ handleOnInputNumberBlur } - onChange={ handleOnChange } shiftStep={ shiftStep } step={ step } - value={ inputSliderValue } + { ...someNumberInputProps } /> ) } { allowReset && ( diff --git a/packages/components/src/range-control/rail.js b/packages/components/src/range-control/rail.js index 34caa88a73622..39496bfc989f4 100644 --- a/packages/components/src/range-control/rail.js +++ b/packages/components/src/range-control/rail.js @@ -16,7 +16,7 @@ export default function RangeRail( { min = 0, max = 100, step = 1, - value = 0, + value, ...restProps } ) { return ( @@ -29,7 +29,7 @@ export default function RangeRail( { min={ min } max={ max } step={ step } - value={ value } + value={ value ?? ( max - min ) / 2 + min } /> ) } diff --git a/packages/components/src/range-control/test/index.js b/packages/components/src/range-control/test/index.js index c60f12b3b7072..5c1c41e48521b 100644 --- a/packages/components/src/range-control/test/index.js +++ b/packages/components/src/range-control/test/index.js @@ -1,7 +1,12 @@ /** * External dependencies */ -import { fireEvent, render } from '@testing-library/react'; +import { fireEvent, render, waitFor } from '@testing-library/react'; + +/** + * WordPress dependencies + */ +import { useState } from '@wordpress/element'; /** * Internal dependencies @@ -15,14 +20,26 @@ const getNumberInput = ( container ) => const getResetButton = ( container ) => container.querySelector( '.components-range-control__reset' ); -describe( 'RangeControl', () => { +function ControlledRangeControl( props ) { + const [ value, setValue ] = useState( props.value ); + const onChange = ( v ) => { + setValue( v ); + props.onChange?.( v ); + }; + return ; +} + +describe.each( [ + [ 'uncontrolled', RangeControl ], + [ 'controlled', ControlledRangeControl ], +] )( 'RangeControl %s', ( ...modeAndComponent ) => { + const [ mode, Component ] = modeAndComponent; + describe( '#render()', () => { it( 'should trigger change callback with numeric value', () => { const onChange = jest.fn(); - const { container } = render( - - ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -39,10 +56,7 @@ describe( 'RangeControl', () => { it( 'should render with icons', () => { const { container } = render( - + ); const beforeIcon = container.querySelector( @@ -59,7 +73,7 @@ describe( 'RangeControl', () => { describe( 'validation', () => { it( 'should not apply if new value is lower than minimum', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -71,7 +85,7 @@ describe( 'RangeControl', () => { } ); it( 'should not apply if new value is greater than maximum', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -85,7 +99,7 @@ describe( 'RangeControl', () => { it( 'should not call onChange if new value is invalid', () => { const onChange = jest.fn(); const { container } = render( - + ); const numberInput = getNumberInput( container ); @@ -96,10 +110,10 @@ describe( 'RangeControl', () => { expect( onChange ).not.toHaveBeenCalled(); } ); - it( 'should keep invalid values in number input until loss of focus', () => { + it( 'should keep invalid values in number input until loss of focus', async () => { const onChange = jest.fn(); const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -108,7 +122,7 @@ describe( 'RangeControl', () => { numberInput.focus(); fireEvent.change( numberInput, { target: { value: '-1.1' } } ); - expect( numberInput.value ).toBe( '-1.1' ); + await waitFor( () => expect( numberInput.value ).toBe( '-1.1' ) ); expect( rangeInput.value ).toBe( '-1' ); fireEvent.blur( numberInput ); @@ -118,7 +132,7 @@ describe( 'RangeControl', () => { it( 'should validate when provided a max or min of zero', () => { const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -133,7 +147,7 @@ describe( 'RangeControl', () => { it( 'should validate when min and max are negative', () => { const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -154,11 +168,7 @@ describe( 'RangeControl', () => { it( 'should take into account the step starting from min', () => { const onChange = jest.fn(); const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -179,9 +189,7 @@ describe( 'RangeControl', () => { describe( 'initialPosition / value', () => { it( 'should render initial rendered value of 50% of min/max, if no initialPosition or value is defined', () => { - const { container } = render( - - ); + const { container } = render( ); const rangeInput = getRangeInput( container ); @@ -190,7 +198,7 @@ describe( 'RangeControl', () => { it( 'should render initialPosition if no value is provided', () => { const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -200,7 +208,7 @@ describe( 'RangeControl', () => { it( 'should render value instead of initialPosition is provided', () => { const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -211,7 +219,7 @@ describe( 'RangeControl', () => { describe( 'input field', () => { it( 'should render an input field by default', () => { - const { container } = render( ); + const { container } = render( ); const numberInput = getNumberInput( container ); @@ -220,7 +228,7 @@ describe( 'RangeControl', () => { it( 'should not render an input field, if disabled', () => { const { container } = render( - + ); const numberInput = getNumberInput( container ); @@ -229,7 +237,7 @@ describe( 'RangeControl', () => { } ); it( 'should render a zero value into input range and field', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -239,7 +247,7 @@ describe( 'RangeControl', () => { } ); it( 'should update both field and range on change', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -258,7 +266,7 @@ describe( 'RangeControl', () => { } ); it( 'should reset input values if next value is removed', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -274,14 +282,31 @@ describe( 'RangeControl', () => { } ); describe( 'reset', () => { - it( 'should reset to a custom fallback value, defined by a parent component', () => { + it.concurrent.each( [ + [ + 'initialPosition if it is defined', + { initialPosition: 21 }, + [ '21', undefined ], + ], + [ + 'resetFallbackValue if it is defined', + { resetFallbackValue: 34 }, + [ '34', 34 ], + ], + [ + 'resetFallbackValue if both it and initialPosition are defined', + { initialPosition: 21, resetFallbackValue: 34 }, + [ '34', 34 ], + ], + ] )( 'should reset to %s', ( ...all ) => { + const [ , propsForReset, [ expectedValue, expectedChange ] ] = all; const spy = jest.fn(); const { container } = render( - ); @@ -291,19 +316,20 @@ describe( 'RangeControl', () => { fireEvent.click( resetButton ); - expect( rangeInput.value ).toBe( '33' ); - expect( numberInput.value ).toBe( '33' ); - expect( spy ).toHaveBeenCalledWith( 33 ); + expect( rangeInput.value ).toBe( expectedValue ); + expect( numberInput.value ).toBe( expectedValue ); + expect( spy ).toHaveBeenCalledWith( expectedChange ); } ); it( 'should reset to a 50% of min/max value, of no initialPosition or value is defined', () => { const { container } = render( - ); diff --git a/packages/components/src/range-control/utils.js b/packages/components/src/range-control/utils.js index e2a758a6b4f1c..aab35cf09c0f6 100644 --- a/packages/components/src/range-control/utils.js +++ b/packages/components/src/range-control/utils.js @@ -2,7 +2,7 @@ /** * External dependencies */ -import { clamp, noop } from 'lodash'; +import { noop } from 'lodash'; /** * WordPress dependencies @@ -10,61 +10,45 @@ import { clamp, noop } from 'lodash'; import { useCallback, useRef, useEffect, useState } from '@wordpress/element'; /** - * Internal dependencies - */ -import { useControlledState } from '../utils/hooks'; - -/** - * A float supported clamp function for a specific value. - * - * @param {number|null} value The value to clamp. - * @param {number} min The minimum value. - * @param {number} max The maximum value. - * - * @return {number} A (float) number - */ -export function floatClamp( value, min, max ) { - if ( typeof value !== 'number' ) { - return null; - } - - return parseFloat( clamp( value, min, max ) ); -} - -/** - * Hook to store a clamped value, derived from props. + * Enables entry of out-of-range and invalid values that diverge from state. * - * @param {Object} settings Hook settings. - * @param {number} settings.min The minimum value. - * @param {number} settings.max The maximum value. - * @param {number} settings.value The current value. - * @param {any} settings.initial The initial value. + * @param {Object} props Props + * @param {number|null} props.value Incoming value. + * @param {number} props.max Maximum valid value. + * @param {number} props.min Minimum valid value. + * @param {(next: number) => void} props.onChange Callback for changes. * - * @return {[*, Function]} The controlled value and the value setter. + * @return {Object} Assorted props for the input. */ -export function useControlledRangeValue( { - min, - max, - value: valueProp, - initial, -} ) { - const [ state, setInternalState ] = useControlledState( - floatClamp( valueProp, min, max ), - { initial, fallback: null } - ); - - const setState = useCallback( - ( nextValue ) => { - if ( nextValue === null ) { - setInternalState( null ); - } else { - setInternalState( floatClamp( nextValue, min, max ) ); - } - }, - [ min, max ] - ); - - return [ state, setState ]; +export function useUnimpededRangedNumberEntry( { max, min, onChange, value } ) { + const ref = useRef(); + const isDiverging = useRef( false ); + /** @type {import('../input-control/types').InputChangeCallback}*/ + const changeHandler = ( next ) => { + next = parseFloat( next ); + if ( next < min || next > max ) { + isDiverging.current = true; + next = Math.max( min, Math.min( max, next ) ); + } + onChange( next ); + }; + // When the value entered in the input is out of range then a clamped value + // is sent through onChange and that goes on to update the input. In such + // circumstances this effect overwrites the input value with the entered + // value to avoid interfering with typing. E.g. Without this effect, if + // `min` is 20 and the user intends to type 25, as soon as 2 is typed the + // input will update to 20 and likely lead to an entry of 205. + useEffect( () => { + if ( ref.current && isDiverging.current ) { + const input = ref.current; + const entry = input.value; + const { defaultView } = input.ownerDocument; + defaultView.requestAnimationFrame( () => ( input.value = entry ) ); + isDiverging.current = false; + } + }, [ value ] ); + + return { max, min, ref, value, onChange: changeHandler }; } /** diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index e97bc6ff955f6..089155b9d2ffd 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -34,7 +34,7 @@ import { } from './utils'; import { useControlledState } from '../utils/hooks'; import type { UnitControlProps, UnitControlOnChangeCallback } from './types'; -import type { StateReducer } from '../input-control/reducer/state'; +import type { SecondaryReducer } from '../input-control/reducer/state'; function UnforwardedUnitControl( unitControlProps: WordPressComponentProps< @@ -178,10 +178,7 @@ function UnforwardedUnitControl( : undefined; const changeProps = { event, data }; - onChangeProp?.( - `${ validParsedQuantity ?? '' }${ validParsedUnit }`, - changeProps - ); + // The `onChange` callback already gets called, no need to call it explicitely. onUnitChange?.( validParsedUnit, changeProps ); setUnit( validParsedUnit ); @@ -209,7 +206,7 @@ function UnforwardedUnitControl( * @param action Action triggering state change * @return The updated state to apply to InputControl */ - const unitControlStateReducer: StateReducer = ( state, action ) => { + const unitControlStateReducer: SecondaryReducer = ( state, action ) => { const nextState = { ...state }; /* @@ -229,7 +226,7 @@ function UnforwardedUnitControl( return nextState; }; - let stateReducer: StateReducer = unitControlStateReducer; + let stateReducer: SecondaryReducer = unitControlStateReducer; if ( stateReducerProp ) { stateReducer = ( state, action ) => { const baseState = unitControlStateReducer( state, action ); diff --git a/packages/components/src/unit-control/test/index.js b/packages/components/src/unit-control/test/index.js index d595f6171076b..615d6dcd7af84 100644 --- a/packages/components/src/unit-control/test/index.js +++ b/packages/components/src/unit-control/test/index.js @@ -215,7 +215,7 @@ describe( 'UnitControl', () => { expect( input.value ).toBe( '300px' ); expect( state ).toBe( 50 ); - user.keyboard( '{Escape}' ); + await user.keyboard( '{Escape}' ); expect( input.value ).toBe( '50' ); expect( state ).toBe( 50 );