Skip to content

Finish making InputControl et al. more controllable #40568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stokesman merged 28 commits into from
May 11, 2022
Merged

Finish making InputControl et al. more controllable #40568

Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
f943b5b
Update `RangeControl` to play nice with revised `InputControl`
stokesman Apr 23, 2022
e662bde
Update `UnitControl` to play nice with revised `InputControl`
stokesman Apr 23, 2022
0077735
Restore controlled mode to `RangeControl`
stokesman Apr 25, 2022
295035c
Add missing ;
ciampo Apr 25, 2022
739372f
Add comment after deleting `onChange`
ciampo Apr 25, 2022
cd1b4df
Update test of `RangeControl` to also test controlled mode
stokesman Apr 25, 2022
9a3804f
Remove separate onChange call from reset handling in `RangeControl`
stokesman Apr 25, 2022
4dc4674
Refine RESET logic of `InputControl` reducer
stokesman Apr 25, 2022
302096d
Simplify refined RESET logic of `InputControl` reducer
stokesman Apr 26, 2022
f3c881f
Restore initial position of `RangeControl` when without value
stokesman Apr 26, 2022
19fd100
Differentiate state sync effect hooks by event existence
stokesman Apr 26, 2022
68a02ea
Add and use type `SecondaryReducer`
stokesman Apr 26, 2022
d6b43bb
Cleanup legacy `event.perist()`
stokesman Apr 26, 2022
b3e9d88
Simplify update from props in reducer
stokesman Apr 26, 2022
547a7c7
Ensure event is cleared after drag actions
stokesman Apr 26, 2022
89affab
Avoid declaration of potential unused variable
stokesman Apr 26, 2022
06d16fd
Add more reset unit tests for `RangeControl`
stokesman Apr 28, 2022
ce0ac42
Run `RangeControl` unit test in both controlled/uncontrolled modes
stokesman Apr 28, 2022
3800d28
Make “keep invaid values” test async
stokesman May 2, 2022
ff6e10a
Prevent interference of value entry in number input
stokesman May 2, 2022
e427a0e
Remove unused `floatClamp` function
stokesman May 2, 2022
38c0158
Fix reset to `initialPosition`
stokesman May 2, 2022
8930905
Fix a couple tests for controlled `RangeControl`
stokesman May 2, 2022
cb00749
Fix `RangeControl` reset
stokesman May 2, 2022
fa2723d
Ensure `InputControl`’s state syncing works after focus changes
stokesman May 2, 2022
085fd03
Comment
stokesman May 10, 2022
29634d7
Ignore NaN values in `useUnimpededRangedNumberEntry`
stokesman May 10, 2022
2b14b0f
Refine use of event existence in state syncing effect hooks
stokesman May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/components/src/input-control/input-field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ function InputField(
value: valueProp,
isPressEnterToChange,
},
onChange
onChange,
isFocused
);

const { value, isDragging, isDirty } = state;
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/input-control/reducer/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const PRESS_UP = 'PRESS_UP';
export const RESET = 'RESET';

interface EventPayload {
event?: SyntheticEvent;
event: SyntheticEvent;
}

interface Action< Type, ExtraPayload = {} > {
Expand Down
57 changes: 26 additions & 31 deletions packages/components/src/input-control/reducer/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@youknowriad youknowriad May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this happen (action without type)?

Copy link
Contributor Author

@stokesman stokesman May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be in what I'm calling the intake effect

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

Lines 218 to 226 in 2b14b0f

useLayoutEffect( () => {
if (
initialState.value !== currentState.current.value &&
! currentState.current.isDirty
) {
dispatch( { value: initialState.value } );
}
if ( refEvent.current ) refEvent.current = null;
}, [ initialState.value ] );

That was dispatching via reset in the base PR but in looking for a way to distinguish the update from props I landed on making the bare dispatch with no action type. I'd considered implementing another action (or bringing back update that used to exist and was used only for this purpose (props coming in)) but I went with the bare dispatch because I thought it would make for a little less changes.

Copy link
Contributor

@youknowriad youknowriad May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have just used another action but it's not important :)

if ( ! ( 'type' in action ) ) {
return {
...state,
value: `${ action.value ?? '' }`,
};
}
const nextState = { ...state };

switch ( action.type ) {
Expand Down Expand Up @@ -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;

/**
Expand All @@ -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
Expand All @@ -135,12 +139,14 @@ function inputControlStateReducer(
* @param stateReducer An external state reducer.
* @param initialState The initial state for the reducer.
* @param onChangeHandler A handler for the onChange event.
* @param isFocused Focus state.
* @return State, dispatch, and a collection of actions.
*/
export function useInputControlStateReducer(
stateReducer: StateReducer = initialStateReducer,
initialState: Partial< InputState > = initialInputControlState,
onChangeHandler: InputChangeCallback
onChangeHandler: InputChangeCallback,
isFocused: Boolean
) {
const [ state, dispatch ] = useReducer< StateReducer >(
inputControlStateReducer( stateReducer ),
Expand All @@ -151,15 +157,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 },
Expand All @@ -169,30 +167,25 @@ 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 } );
};

/**
* 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 );

Expand All @@ -206,31 +199,33 @@ 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 ) return;

if (
state.value !== currentValueProp.current &&
! currentState.current.isDirty
) {
onChangeHandler( state.value ?? '', {
event: currentState.current._event as
event: refEvent.current as
| ChangeEvent< HTMLInputElement >
| PointerEvent< HTMLInputElement >,
} );
}
}, [ state.value ] );
refEvent.current = null;
}, [ state.value, state.isDragging, isFocused ] );
Copy link
Contributor

@youknowriad youknowriad May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why we need to pass "isFocused" to this hook. For me, isFocused is more a "component" problem in the sense that if we want to trigger onChangeHandler when the component loses focus, we should just call commit (which will change the state.value and trigger the change handler.

Adding isFocused and state.isDragging here is a code smell to me and potentially something that will create hidden bugs.

Copy link
Contributor Author

@stokesman stokesman May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional dependencies were added only in the interest of executing refEvent.current = null not onChangeHandler. That's done as part of making sure the effect hook used to intake values doesn't dispatch (via reset in the base PR) more than necessary.

Without using the existence of the event to avoid some dispatches, there was an issue with FocalPointPicker. That component is surely doing it wrong but it seemed the shorter and more defensive path to reduce the dispatches.

I'm glad to take another look and see how it might be improved upon. I have some ideas.

Copy link
Contributor Author

@stokesman stokesman May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed 2b14b0f, which removes the additional dependencies. The event ref (if it exists) is now cleared when the intake effect hook runs. This is much better. Thank you for the critique @youknowriad 🙇

useLayoutEffect( () => {
if (
! refEvent.current &&
initialState.value !== currentState.current.value &&
! currentState.current.isDirty
) {
reset(
initialState.value,
currentState.current._event as SyntheticEvent
);
dispatch( { value: initialState.value } );
}
}, [ initialState.value ] );

Expand Down
12 changes: 7 additions & 5 deletions packages/components/src/input-control/reducer/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
103 changes: 42 additions & 61 deletions packages/components/src/range-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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"
Expand All @@ -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 );

Expand All @@ -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 ) {
Expand All @@ -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 <input /> 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 );
Expand All @@ -197,7 +181,7 @@ function RangeControl(
};

const offsetStyle = {
[ isRTL() ? 'right' : 'left' ]: fillValueOffset,
[ isRTL() ? 'right' : 'left' ]: fillPercent,
};

return (
Expand Down Expand Up @@ -235,7 +219,7 @@ function RangeControl(
onMouseLeave={ onMouseLeave }
ref={ setRef }
step={ step }
value={ inputSliderValue }
value={ usedValue ?? '' }
/>
<RangeRail
aria-hidden={ true }
Expand All @@ -245,13 +229,13 @@ function RangeControl(
min={ min }
railColor={ railColor }
step={ step }
value={ rangeFillValue }
value={ usedValue }
/>
<Track
aria-hidden={ true }
className="components-range-control__track"
disabled={ disabled }
style={ { width: fillValueOffset } }
style={ { width: fillPercent } }
trackColor={ trackColor }
/>
<ThumbWrapper style={ offsetStyle } disabled={ disabled }>
Expand Down Expand Up @@ -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 && (
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/range-control/rail.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export default function RangeRail( {
min = 0,
max = 100,
step = 1,
value = 0,
value,
...restProps
} ) {
return (
Expand All @@ -29,7 +29,7 @@ export default function RangeRail( {
min={ min }
max={ max }
step={ step }
value={ value }
value={ value ?? ( max - min ) / 2 + min }
/>
) }
</>
Expand Down
Loading