From ebd9c986d459c8cfed8e2c1525aa34d09b07aaaf Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Wed, 7 Oct 2020 15:27:14 -0700 Subject: [PATCH 1/4] avoid overwriting drag gesture handler in InputControl --- .../src/input-control/input-field.js | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/packages/components/src/input-control/input-field.js b/packages/components/src/input-control/input-field.js index 3badce2388a338..6d63b3ae5aa641 100644 --- a/packages/components/src/input-control/input-field.js +++ b/packages/components/src/input-control/input-field.js @@ -102,19 +102,6 @@ function InputField( } }; - /* - * Works around the odd UA (e.g. Firefox) that does not focus inputs of - * type=number when their spinner arrows are pressed. - */ - let handleOnMouseDown; - if ( type === 'number' ) { - handleOnMouseDown = ( event ) => { - if ( event.target !== event.target.ownerDocument.activeElement ) { - event.target.focus(); - } - }; - } - const handleOnFocus = ( event ) => { onFocus( event ); setIsFocused( true ); @@ -164,7 +151,6 @@ function InputField( ( dragProps ) => { const { distance, dragging, event } = dragProps; - if ( ! isDragEnabled ) return; if ( ! distance ) return; event.stopPropagation(); @@ -192,10 +178,29 @@ function InputField( } ); + const { onMouseDown, onTouchStart } = isDragEnabled + ? dragGestureProps() + : {}; + let handleOnMouseDown = onMouseDown; + + /* + * Works around the odd UA (e.g. Firefox) that does not focus inputs of + * type=number when their spinner arrows are pressed. + */ + if ( type === 'number' ) { + handleOnMouseDown = ( event ) => { + if ( event.target !== event.target.ownerDocument.activeElement ) { + event.target.focus(); + } + if ( isDragEnabled ) { + onMouseDown( event ); + } + }; + } + return ( Date: Wed, 7 Oct 2020 16:43:06 -0700 Subject: [PATCH 2/4] revise InputControl state handling for commits on blur - adds wasDirtyOnBlur and its use in syncing logic - removes a bit of lint --- .../src/input-control/input-field.js | 21 ++++++++++++------- .../components/src/input-control/state.js | 10 ++++----- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/packages/components/src/input-control/input-field.js b/packages/components/src/input-control/input-field.js index 6d63b3ae5aa641..d4ec868172fc3c 100644 --- a/packages/components/src/input-control/input-field.js +++ b/packages/components/src/input-control/input-field.js @@ -7,7 +7,7 @@ import { useDrag } from 'react-use-gesture'; /** * WordPress dependencies */ -import { forwardRef } from '@wordpress/element'; +import { forwardRef, useRef } from '@wordpress/element'; import { UP, DOWN, ENTER } from '@wordpress/keycodes'; /** * Internal dependencies @@ -66,22 +66,26 @@ function InputField( } ); const { _event, value, isDragging, isDirty } = state; + const wasDirtyOnBlur = useRef( false ); const dragCursor = useDragCursor( isDragging, dragDirection ); /* - * Syncs value state using the focus state to determine the direction. - * Without focus it updates the value from the props. With focus it - * propagates the value and event through onChange. + * Handles syncronization 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 */ useUpdateEffect( () => { if ( valueProp === value ) { return; } - if ( ! isFocused ) { + if ( ! isFocused && ! wasDirtyOnBlur.current ) { update( valueProp ); } else if ( ! isDirty ) { onChange( value, { event: _event } ); + wasDirtyOnBlur.current = false; } }, [ value, isDirty, isFocused, valueProp ] ); @@ -94,8 +98,9 @@ function InputField( * the onChange callback. */ if ( isPressEnterToChange && isDirty ) { + wasDirtyOnBlur.current = true; if ( ! isValueEmpty( value ) ) { - handleOnCommit( { target: { value } }, event ); + handleOnCommit( event ); } else { reset( valueProp ); } @@ -116,10 +121,10 @@ function InputField( const nextValue = event.target.value; try { - onValidate( nextValue, { event } ); + onValidate( nextValue, event ); commit( nextValue, event ); } catch ( err ) { - invalidate( err, { event } ); + invalidate( err, event ); } }; diff --git a/packages/components/src/input-control/state.js b/packages/components/src/input-control/state.js index b2cd3cc5225d26..df4355528e7d5c 100644 --- a/packages/components/src/input-control/state.js +++ b/packages/components/src/input-control/state.js @@ -131,10 +131,8 @@ function inputControlStateReducer( composedStateReducers ) { break; case actionTypes.UPDATE: - if ( payload.value !== state.value ) { - nextState.value = payload.value; - nextState.isDirty = false; - } + nextState.value = payload.value; + nextState.isDirty = false; break; /** @@ -218,7 +216,7 @@ export function useInputControlStateReducer( * Actions for the reducer */ const change = createChangeEvent( actionTypes.CHANGE ); - const inValidate = createChangeEvent( actionTypes.INVALIDATE ); + const invalidate = createChangeEvent( actionTypes.INVALIDATE ); const reset = createChangeEvent( actionTypes.RESET ); const commit = createChangeEvent( actionTypes.COMMIT ); const update = createChangeEvent( actionTypes.UPDATE ); @@ -238,7 +236,7 @@ export function useInputControlStateReducer( drag, dragEnd, dragStart, - inValidate, + invalidate, pressDown, pressEnter, pressUp, From b6cad797acc10bc7af18859e0124fbeef44f99d2 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Fri, 9 Oct 2020 12:11:49 -0700 Subject: [PATCH 3/4] improve unit changing logic in UnitControl allows unit to change on parses even if parsed value is unchanged --- packages/components/src/unit-control/index.js | 109 ++++++++---------- 1 file changed, 50 insertions(+), 59 deletions(-) diff --git a/packages/components/src/unit-control/index.js b/packages/components/src/unit-control/index.js index 9bf870e4dd855c..782cce7d910ac3 100644 --- a/packages/components/src/unit-control/index.js +++ b/packages/components/src/unit-control/index.js @@ -8,6 +8,7 @@ import classnames from 'classnames'; * WordPress dependencies */ import { forwardRef, useRef } from '@wordpress/element'; +import { ENTER } from '@wordpress/keycodes'; /** * Internal dependencies @@ -48,11 +49,8 @@ function UnitControl( initial: initialUnit, } ); - /* - * Storing parsed unit changes to be applied during the onChange callback - * cycle. - */ - const nextParsedUnitRef = useRef(); + // Stores parsed value for hand-off in state reducer + const refParsedValue = useRef( null ); const classes = classnames( 'components-unit-control', className ); @@ -66,30 +64,9 @@ function UnitControl( * Customizing the onChange callback. * This allows as to broadcast a combined value+unit to onChange. */ - const [ parsedValue, parsedUnit ] = getValidParsedUnit( - next, - units, - value, - unit - ); - const nextParsedUnit = nextParsedUnitRef.current; + next = getValidParsedUnit( next, units, value, unit ).join( '' ); - /* - * If we've noticed a (parsed) unit change, which would have been - * stored in nextParsedUnitRef, we'll update our local unit set, - * as well as fire the onUnitChange callback. - */ - if ( nextParsedUnit ) { - onUnitChange( nextParsedUnit, changeProps ); - setUnit( nextParsedUnit ); - // We have to reset this ref value to properly track new changes. - nextParsedUnitRef.current = null; - } - - const nextUnit = nextParsedUnit || parsedUnit; - const nextValue = `${ parsedValue }${ nextUnit }`; - - onChange( nextValue, changeProps ); + onChange( next, changeProps ); }; const handleOnUnitChange = ( next, changeProps ) => { @@ -107,6 +84,42 @@ function UnitControl( setUnit( next ); }; + const mayUpdateUnit = ( event ) => { + if ( ! isNaN( event.target.value ) ) { + refParsedValue.current = null; + return; + } + const [ parsedValue, parsedUnit ] = getValidParsedUnit( + event.target.value, + units, + value, + unit + ); + + refParsedValue.current = parsedValue; + + if ( isPressEnterToChange && parsedUnit !== unit ) { + const data = units.find( + ( option ) => option.value === parsedUnit + ); + const changeProps = { event, data }; + + onChange( `${ parsedValue }${ parsedUnit }`, changeProps ); + onUnitChange( parsedUnit, changeProps ); + + setUnit( parsedUnit ); + } + }; + + const handleOnBlur = mayUpdateUnit; + + const handleOnKeyDown = ( event ) => { + const { keyCode } = event; + if ( keyCode === ENTER ) { + mayUpdateUnit( event ); + } + }; + /** * "Middleware" function that intercepts updates from InputControl. * This allows us to tap into actions to transform the (next) state for @@ -117,39 +130,15 @@ function UnitControl( * @return {Object} The updated state to apply to InputControl */ const unitControlStateReducer = ( state, action ) => { - const { type, payload } = action; - const event = payload?.event; - /* - * Customizes the commit interaction. - * - * This occurs when pressing ENTER to fire a change. - * By intercepting the state change, we can parse the incoming - * value to determine if the unit needs to be updated. + * On commits (when pressing ENTER and on blur if + * isPressEnterToChange is true), if a parse has been performed + * then use that result to update the state. */ - if ( type === inputControlActionTypes.COMMIT ) { - const valueToParse = event?.target?.value; - - const [ parsedValue, parsedUnit ] = getValidParsedUnit( - valueToParse, - units, - value, - unit - ); - - state.value = parsedValue; - - // Update unit if the incoming parsed unit is different. - if ( unit !== parsedUnit ) { - /* - * We start by storing the next parsedUnit value in our - * nextParsedUnitRef. We can't set the unit during this - * stateReducer callback as it cause state update - * conflicts within React's render cycle. - * - * https://github.com/facebook/react/issues/18178 - */ - nextParsedUnitRef.current = parsedUnit; + if ( action.type === inputControlActionTypes.COMMIT ) { + if ( refParsedValue.current !== null ) { + state.value = refParsedValue.current; + refParsedValue.current = null; } } @@ -179,6 +168,8 @@ function UnitControl( disableUnits={ disableUnits } isPressEnterToChange={ isPressEnterToChange } label={ label } + onBlur={ handleOnBlur } + onKeyDown={ handleOnKeyDown } onChange={ handleOnChange } ref={ ref } size={ size } From 3f788c4b6b07d6f85dfa2ed2399116f0f4ab17a0 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 12 Oct 2020 09:27:55 -0700 Subject: [PATCH 4/4] correct an oversight in NumberControl --- packages/components/src/number-control/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/components/src/number-control/index.js b/packages/components/src/number-control/index.js index 15adad91b628a2..b3b7944123d25a 100644 --- a/packages/components/src/number-control/index.js +++ b/packages/components/src/number-control/index.js @@ -150,11 +150,11 @@ export function NumberControl( } /** - * Handles ENTER key press and submit + * Handles commit (ENTER key press or on blur if isPressEnterToChange) */ if ( type === inputControlActionTypes.PRESS_ENTER || - type === inputControlActionTypes.SUBMIT + type === inputControlActionTypes.COMMIT ) { state.value = roundClamp( currentValue, min, max ); }