InputControl: Fix undo when changing padding values #40518
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
the
[Type] Bug
mtias
added
the
[Feature] UI Components
|
Hey @youknowriad , thank you for the ping.
I can definitely reproduce the bug, and your exploration is very useful in understanding what's causing it.
Just to clarify, the actual chain of components is Having said that, I also personally find that the way To give some more context (even though I wasn't around when this component was initially created), I believe that this complex logic was written this way in order to:
I don't think they're using it wrong . To me, it feels quite the opposite — some of these tests are very specific on the behaviour is expected. I'll try to take a deeper look at the code and see if I can come up with a fix that doesn't break the tests. In case this fix was urgent, I wonder if disabling the drag gestures for the time being could be considered an option ? For the medium-long term, I would personally really like to simplify this component — understand if certain features can be removed and/or simplified (e.g. cc'ing also a few folks who have worked on this component recently to collect a few more opinions — @mirka @stokesman @andrewserong @aaronrobertshaw |
|
To reproduce this you don't have to drag to change the value or even use the input itself. The main thing is that the the input is focused after a change. A great example would be the Spacer block. Drag the block’s in-canvas resize control, then focus the input that controls the height. Then try undo and note that it won't work. The same applies for redo as well. It might help to clarify the fundamental change in this branch is that This change is the only way I can imagine to fix the bug and due to the tight coupling of some other components they require changes as well. I've opened up #40568 with what seems a good start on finishing this up. We can merge to this branch if it looks good. I might have some comments or changes to add there after I test some more. |
My reasoning is that the components should be always controlled if a "value" prop is provided. If the "value" prop is undefined, the components should be uncontrolled. Is that what you meant with "controllable"? |
|
I agree with that reasoning and was trying to point out how the component’s exception to being controlled while focused is the condition in which the bug presents. I figured you understood as much, but wrote it in case it would help anyone else having a look here. Also, If it wasn't clear, I support this change 💯. |
|
Thank you @stokesman for helping out! The changes proposed in your PR make sense to me, and I'd be in favour or merging it into this PR as soon as it's ready. We'd be then able to give a final round of review to this PR and hopefully merge the fix into |
There was a problem hiding this comment.
Thanks for the hard work on this @youknowriad and @stokesman 👍
I've given this a bit of a test now #40568 has been merged into this PR.
✅ Original issue could be replicated on trunk
✅ Issue has been resolved after applying this PR
✅ Tested Input, Range and Unit controls via editor and Storybook
✅ Unit tests pass for affected controls and no typing errors
There was one edge case with the RangeControl I encountered that I don't believe has been handled yet.
To replicate:
- Fire up the Storybook, visit the RangeControl page, and in the controls allow resets and set an initial position
- Manually type a value in the input field well above the
maxlimit - Without pressing enter or switching focus elsewhere, click the reset button
- Notice the slider position is correct but the input field is showing the clamped max value
- Focus the input field and note that it now displays the correct value
Screen.Recording.2022-05-16.at.4.07.14.pm.mp4
I don't think this is a blocker to this PR and could be addressed separately.
|
Thanks for testing and great catch of that reset issue @aaronrobertshaw. I agree it could be addressed separately as it's a definite edge case and no Update unimpeded ranged entry hook to account for blur eventdiff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js
index 4018d37cfa..606d53895f 100644
--- a/packages/components/src/range-control/index.js
+++ b/packages/components/src/range-control/index.js
@@ -139,15 +139,14 @@ function RangeControl(
isResetPendent.current = true;
}
},
+ onBlur: () => {
+ if ( isResetPendent.current ) {
+ handleOnReset();
+ isResetPendent.current = false;
+ }
+ },
} );
- const handleOnInputNumberBlur = () => {
- if ( isResetPendent.current ) {
- handleOnReset();
- isResetPendent.current = false;
- }
- };
-
const handleOnReset = () => {
const resetValue = parseFloat( resetFallbackValue );
@@ -269,7 +268,6 @@ function RangeControl(
disabled={ disabled }
inputMode="decimal"
isShiftStepEnabled={ isShiftStepEnabled }
- onBlur={ handleOnInputNumberBlur }
shiftStep={ shiftStep }
step={ step }
{ ...someNumberInputProps }
diff --git a/packages/components/src/range-control/utils.js b/packages/components/src/range-control/utils.js
index aab35cf09c..2984434080 100644
--- a/packages/components/src/range-control/utils.js
+++ b/packages/components/src/range-control/utils.js
@@ -20,7 +20,13 @@ import { useCallback, useRef, useEffect, useState } from '@wordpress/element';
*
* @return {Object} Assorted props for the input.
*/
-export function useUnimpededRangedNumberEntry( { max, min, onChange, value } ) {
+export function useUnimpededRangedNumberEntry( {
+ max,
+ min,
+ onBlur,
+ onChange,
+ value,
+} ) {
const ref = useRef();
const isDiverging = useRef( false );
/** @type {import('../input-control/types').InputChangeCallback}*/
@@ -32,6 +38,10 @@ export function useUnimpededRangedNumberEntry( { max, min, onChange, value } ) {
}
onChange( next );
};
+ const blurHandler = () => {
+ isDiverging.current = false;
+ onBlur?.();
+ };
// 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
@@ -48,7 +58,14 @@ export function useUnimpededRangedNumberEntry( { max, min, onChange, value } ) {
}
}, [ value ] );
- return { max, min, ref, value, onChange: changeHandler };
+ return {
+ max,
+ min,
+ ref,
+ value,
+ onBlur: blurHandler,
+ onChange: changeHandler,
+ };
}
/**
|
aaronrobertshaw
approved these changes
May 17, 2022
There was a problem hiding this comment.
Appreciate the quick fix for the reset issue @stokesman 🚀
I've retested this PR with the suggested patch and everything now appears to be functioning well. I can confirm the reset issue is resolved.
Screen.Recording.2022-05-17.at.10.33.21.am.mp4
I agree it could be addressed separately as it's a definite edge case and no RangeControls in the repo even allowReset
I did look to see if allowReset was used at all in Gutenberg. Despite it not being used, the RangeControl is exported as a stable component for 3rd party use. While we are changing the way it works under the hood it makes sense to me to include your suggested patch as well, particularly given its simplicity.
I'm happy to approve this with or without the patch fixing the reset behaviour. It would probably be a good idea to get a second set of eyes on this before merging though.
stokesman
force-pushed
the
fix/input-field-reset-behavior
branch
from
c8a8013
to
5150ade
Compare
👍 Thanks again for testing! I went ahead and
|
|
Size Change: +131 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
Great work here @stokesman I think it's probably ready to merge right? |
|
Actually it turns out the e2e test failure here is legit. |
|
I've discovered another thing that this PR was breaking but wasn't being caught by current tests. regressed-blur-commit-empty-box-control.mp4I've got some PRs to update unit tests that will ensure this would be caught. I also have a branch that fixes the issue and obviates the changes introduced from #40568. I think I'll go ahead and revert those here and get the new branch merged into this one. |
* Update `RangeControl` to play nice with revised `InputControl` * Update `UnitControl` to play nice with revised `InputControl` * Restore controlled mode to `RangeControl` * Add missing ; * Add comment after deleting `onChange` * Update test of `RangeControl` to also test controlled mode * Remove separate onChange call from reset handling in `RangeControl` * Refine RESET logic of `InputControl` reducer * Simplify refined RESET logic of `InputControl` reducer * Restore initial position of `RangeControl` when without value * Differentiate state sync effect hooks by event existence * Add and use type `SecondaryReducer` * Cleanup legacy `event.perist()` * Simplify update from props in reducer Co-authored-by: Lena Morita <lena@jaguchi.com> * Ensure event is cleared after drag actions * Avoid declaration of potential unused variable * Add more reset unit tests for `RangeControl` * Run `RangeControl` unit test in both controlled/uncontrolled modes * Make “keep invaid values” test async * Prevent interference of value entry in number input * Remove unused `floatClamp` function * Fix reset to `initialPosition` * Fix a couple tests for controlled `RangeControl` * Fix `RangeControl` reset * Ensure `InputControl`’s state syncing works after focus changes * Comment * Ignore NaN values in `useUnimpededRangedNumberEntry` * Refine use of event existence in state syncing effect hooks Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> Co-authored-by: Lena Morita <lena@jaguchi.com>
… digits" This reverts commit bbdbfc8.
This reverts commit 769e69d.
stokesman
force-pushed
the
fix/input-field-reset-behavior
branch
from
5150ade
to
3ac6020
Compare
stokesman
reviewed
May 30, 2022
There was a problem hiding this comment.
I wanted to provide some explanation of the latest commits and provide a diff for extended unit testing.
A rather large diff with the unit test changes from #41421 and ##41422 and #40568 I used to test this
diff --git a/packages/components/src/box-control/test/index.js b/packages/components/src/box-control/test/index.js
index 4515c4885c..c98dd3b48d 100644
--- a/packages/components/src/box-control/test/index.js
+++ b/packages/components/src/box-control/test/index.js
@@ -1,19 +1,29 @@
/**
* External dependencies
*/
-import { render, fireEvent, screen } from '@testing-library/react';
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';
-import { ENTER } from '@wordpress/keycodes';
/**
* Internal dependencies
*/
import BoxControl from '../';
+const setupUser = () =>
+ userEvent.setup( {
+ advanceTimers: jest.advanceTimersByTime,
+ } );
+
+const getInput = () =>
+ screen.getByLabelText( 'Box Control', { selector: 'input' } );
+const getSelect = () => screen.getByLabelText( 'Select unit' );
+const getReset = () => screen.getByText( /Reset/ );
+
describe( 'BoxControl', () => {
describe( 'Basic rendering', () => {
it( 'should render', () => {
@@ -23,42 +33,41 @@ describe( 'BoxControl', () => {
expect( input ).toBeTruthy();
} );
- it( 'should update values when interacting with input', () => {
- const { container } = render( <BoxControl /> );
- const input = container.querySelector( 'input' );
- const unitSelect = container.querySelector( 'select' );
+ it( 'should update values when interacting with input', async () => {
+ const user = setupUser();
+ render( <BoxControl /> );
+ const input = getInput();
+ const select = getSelect();
- input.focus();
- fireEvent.change( input, { target: { value: '100%' } } );
- fireEvent.keyDown( input, { keyCode: ENTER } );
+ await user.type( input, '100%' );
+ await user.keyboard( '{Enter}' );
- expect( input.value ).toBe( '100' );
- expect( unitSelect.value ).toBe( '%' );
+ expect( input ).toHaveValue( '100' );
+ expect( select ).toHaveValue( '%' );
} );
} );
describe( 'Reset', () => {
- it( 'should reset values when clicking Reset', () => {
- const { container, getByText } = render( <BoxControl /> );
- const input = container.querySelector( 'input' );
- const unitSelect = container.querySelector( 'select' );
- const reset = getByText( /Reset/ );
+ it( 'should reset values when clicking Reset', async () => {
+ const user = setupUser();
+ render( <BoxControl /> );
+ const input = getInput();
+ const select = getSelect();
+ const reset = getReset();
- input.focus();
- fireEvent.change( input, { target: { value: '100px' } } );
- fireEvent.keyDown( input, { keyCode: ENTER } );
+ await user.type( input, '100px' );
+ await user.keyboard( '{Enter}' );
- expect( input.value ).toBe( '100' );
- expect( unitSelect.value ).toBe( 'px' );
+ expect( input ).toHaveValue( '100' );
+ expect( select ).toHaveValue( 'px' );
- reset.focus();
- fireEvent.click( reset );
+ await user.click( reset );
- expect( input.value ).toBe( '' );
- expect( unitSelect.value ).toBe( 'px' );
+ expect( input ).toHaveValue( '' );
+ expect( select ).toHaveValue( 'px' );
} );
- it( 'should reset values when clicking Reset, if controlled', () => {
+ it( 'should reset values when clicking Reset, if controlled', async () => {
const Example = () => {
const [ state, setState ] = useState();
@@ -69,26 +78,25 @@ describe( 'BoxControl', () => {
/>
);
};
- const { container, getByText } = render( <Example /> );
- const input = container.querySelector( 'input' );
- const unitSelect = container.querySelector( 'select' );
- const reset = getByText( /Reset/ );
+ const user = setupUser();
+ render( <Example /> );
+ const input = getInput();
+ const select = getSelect();
+ const reset = getReset();
- input.focus();
- fireEvent.change( input, { target: { value: '100px' } } );
- fireEvent.keyDown( input, { keyCode: ENTER } );
+ await user.type( input, '100px' );
+ await user.keyboard( '{Enter}' );
- expect( input.value ).toBe( '100' );
- expect( unitSelect.value ).toBe( 'px' );
+ expect( input ).toHaveValue( '100' );
+ expect( select ).toHaveValue( 'px' );
- reset.focus();
- fireEvent.click( reset );
+ await user.click( reset );
- expect( input.value ).toBe( '' );
- expect( unitSelect.value ).toBe( 'px' );
+ expect( input ).toHaveValue( '' );
+ expect( select ).toHaveValue( 'px' );
} );
- it( 'should reset values when clicking Reset, if controlled <-> uncontrolled state changes', () => {
+ it( 'should reset values when clicking Reset, if controlled <-> uncontrolled state changes', async () => {
const Example = () => {
const [ state, setState ] = useState();
@@ -106,70 +114,78 @@ describe( 'BoxControl', () => {
/>
);
};
- const { container, getByText } = render( <Example /> );
- const input = container.querySelector( 'input' );
- const unitSelect = container.querySelector( 'select' );
- const reset = getByText( /Reset/ );
+ const user = setupUser();
+ render( <Example /> );
+ const input = getInput();
+ const select = getSelect();
+ const reset = getReset();
- input.focus();
- fireEvent.change( input, { target: { value: '100px' } } );
- fireEvent.keyDown( input, { keyCode: ENTER } );
+ await user.type( input, '100px' );
+ await user.keyboard( '{Enter}' );
- expect( input.value ).toBe( '100' );
- expect( unitSelect.value ).toBe( 'px' );
+ expect( input ).toHaveValue( '100' );
+ expect( select ).toHaveValue( 'px' );
- reset.focus();
- fireEvent.click( reset );
+ await user.click( reset );
- expect( input.value ).toBe( '' );
- expect( unitSelect.value ).toBe( 'px' );
+ expect( input ).toHaveValue( '' );
+ expect( select ).toHaveValue( 'px' );
} );
- it( 'should persist cleared value when focus changes', () => {
- render( <BoxControl /> );
+ it( 'should persist cleared value when focus changes', async () => {
+ const user = setupUser();
+ const spyChange = jest.fn();
+ render( <BoxControl onChange={ ( v ) => spyChange( v ) } /> );
const input = screen.getByLabelText( 'Box Control', {
selector: 'input',
} );
const unitSelect = screen.getByLabelText( 'Select unit' );
- input.focus();
- fireEvent.change( input, { target: { value: '100%' } } );
- fireEvent.keyDown( input, { keyCode: ENTER } );
+ await user.type( input, '100%' );
+ await user.keyboard( '{Enter}' );
- expect( input.value ).toBe( '100' );
- expect( unitSelect.value ).toBe( '%' );
+ expect( input ).toHaveValue( '100' );
+ expect( unitSelect ).toHaveValue( '%' );
- fireEvent.change( input, { target: { value: '' } } );
- fireEvent.blur( input );
+ await user.clear( input );
+ expect( input ).toHaveValue( '' );
+ // Clicking document.body to trigger a blur event on the input.
+ await user.click( document.body );
- expect( input.value ).toBe( '' );
+ expect( input ).toHaveValue( '' );
+ expect( spyChange ).toHaveBeenLastCalledWith( {
+ top: undefined,
+ right: undefined,
+ bottom: undefined,
+ left: undefined,
+ } );
} );
} );
describe( 'Unlinked Sides', () => {
- it( 'should update a single side value when unlinked', () => {
+ it( 'should update a single side value when unlinked', async () => {
let state = {};
const setState = ( newState ) => ( state = newState );
- const { container, getByLabelText } = render(
+ const { getAllByLabelText, getByLabelText } = render(
<BoxControl
values={ state }
onChange={ ( next ) => setState( next ) }
/>
);
-
+ const user = setupUser();
const unlink = getByLabelText( /Unlink Sides/ );
- fireEvent.click( unlink );
- const input = container.querySelector( 'input' );
- const unitSelect = container.querySelector( 'select' );
+ await user.click( unlink );
+
+ const input = getByLabelText( /Top/ );
+ const select = getAllByLabelText( /Select unit/ )[ 0 ];
- input.focus();
- fireEvent.change( input, { target: { value: '100px' } } );
- fireEvent.keyDown( input, { keyCode: ENTER } );
+ await user.type( input, '100px' );
+ await user.keyboard( '{Enter}' );
- expect( input.value ).toBe( '100' );
- expect( unitSelect.value ).toBe( 'px' );
+ expect( input ).toHaveValue( '100' );
+ expect( select ).toHaveValue( 'px' );
expect( state ).toEqual( {
top: '100px',
@@ -179,30 +195,30 @@ describe( 'BoxControl', () => {
} );
} );
- it( 'should update a whole axis when value is changed when unlinked', () => {
+ it( 'should update a whole axis when value is changed when unlinked', async () => {
let state = {};
const setState = ( newState ) => ( state = newState );
- const { container, getByLabelText } = render(
+ const { getAllByLabelText, getByLabelText } = render(
<BoxControl
values={ state }
onChange={ ( next ) => setState( next ) }
splitOnAxis={ true }
/>
);
-
+ const user = setupUser();
const unlink = getByLabelText( /Unlink Sides/ );
- fireEvent.click( unlink );
- const input = container.querySelector( 'input' );
- const unitSelect = container.querySelector( 'select' );
+ await user.click( unlink );
+
+ const input = getByLabelText( /Vertical/ );
+ const select = getAllByLabelText( /Select unit/ )[ 0 ];
- input.focus();
- fireEvent.change( input, { target: { value: '100px' } } );
- fireEvent.keyDown( input, { keyCode: ENTER } );
+ await user.type( input, '100px' );
+ await user.keyboard( '{Enter}' );
- expect( input.value ).toBe( '100' );
- expect( unitSelect.value ).toBe( 'px' );
+ expect( input ).toHaveValue( '100' );
+ expect( select ).toHaveValue( 'px' );
expect( state ).toEqual( {
top: '100px',
@@ -214,36 +230,34 @@ describe( 'BoxControl', () => {
} );
describe( 'Unit selections', () => {
- it( 'should update unlinked controls unit selection based on all input control', () => {
+ it( 'should update unlinked controls unit selection based on all input control', async () => {
// Render control.
render( <BoxControl /> );
+ const user = setupUser();
// Make unit selection on all input control.
- const allUnitSelect = screen.getByLabelText( 'Select unit' );
- allUnitSelect.focus();
- fireEvent.change( allUnitSelect, { target: { value: 'em' } } );
+ const allUnitSelect = getSelect();
+ await user.selectOptions( allUnitSelect, [ 'em' ] );
// Unlink the controls.
- const unlink = screen.getByLabelText( /Unlink Sides/ );
- fireEvent.click( unlink );
+ await user.click( screen.getByLabelText( /Unlink Sides/ ) );
// Confirm that each individual control has the selected unit
const unlinkedSelects = screen.getAllByDisplayValue( 'em' );
expect( unlinkedSelects.length ).toEqual( 4 );
} );
- it( 'should use individual side attribute unit when available', () => {
+ it( 'should use individual side attribute unit when available', async () => {
// Render control.
const { rerender } = render( <BoxControl /> );
+ const user = setupUser();
// Make unit selection on all input control.
- const allUnitSelect = screen.getByLabelText( 'Select unit' );
- allUnitSelect.focus();
- fireEvent.change( allUnitSelect, { target: { value: 'vw' } } );
+ const allUnitSelect = getSelect();
+ await user.selectOptions( allUnitSelect, [ 'vw' ] );
// Unlink the controls.
- const unlink = screen.getByLabelText( /Unlink Sides/ );
- fireEvent.click( unlink );
+ await user.click( screen.getByLabelText( /Unlink Sides/ ) );
// Confirm that each individual control has the selected unit
const unlinkedSelects = screen.getAllByDisplayValue( 'vw' );
@@ -261,18 +275,15 @@ describe( 'BoxControl', () => {
} );
describe( 'onChange updates', () => {
- it( 'should call onChange when values contain more than just CSS units', () => {
+ it( 'should call onChange when values contain more than just CSS units', async () => {
const setState = jest.fn();
render( <BoxControl onChange={ setState } /> );
+ const user = setupUser();
+ const input = getInput();
- const input = screen.getByLabelText( 'Box Control', {
- selector: 'input',
- } );
-
- input.focus();
- fireEvent.change( input, { target: { value: '7.5rem' } } );
- fireEvent.keyDown( input, { keyCode: ENTER } );
+ await user.type( input, '7.5rem' );
+ await user.keyboard( '{Enter}' );
expect( setState ).toHaveBeenCalledWith( {
top: '7.5rem',
@@ -282,14 +293,14 @@ describe( 'BoxControl', () => {
} );
} );
- it( 'should not pass invalid CSS unit only values to onChange', () => {
+ it( 'should not pass invalid CSS unit only values to onChange', async () => {
const setState = jest.fn();
render( <BoxControl onChange={ setState } /> );
+ const user = setupUser();
- const allUnitSelect = screen.getByLabelText( 'Select unit' );
- allUnitSelect.focus();
- fireEvent.change( allUnitSelect, { target: { value: 'rem' } } );
+ const allUnitSelect = getSelect();
+ await user.selectOptions( allUnitSelect, 'rem' );
expect( setState ).toHaveBeenCalledWith( {
top: undefined,
diff --git a/packages/components/src/input-control/test/index.js b/packages/components/src/input-control/test/index.js
index c7d18217eb..fe3668db2b 100644
--- a/packages/components/src/input-control/test/index.js
+++ b/packages/components/src/input-control/test/index.js
@@ -1,13 +1,19 @@
/**
* External dependencies
*/
-import { render, screen, fireEvent } from '@testing-library/react';
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
/**
* Internal dependencies
*/
import BaseInputControl from '../';
+const setupUser = () =>
+ userEvent.setup( {
+ advanceTimers: jest.advanceTimersByTime,
+ } );
+
const getInput = () => screen.getByTestId( 'input' );
describe( 'InputControl', () => {
@@ -42,48 +48,52 @@ describe( 'InputControl', () => {
} );
describe( 'Ensurance of focus for number inputs', () => {
- it( 'should focus its input on mousedown events', () => {
+ it( 'should focus its input on mousedown events', async () => {
+ const user = setupUser();
const spy = jest.fn();
render( <InputControl type="number" onFocus={ spy } /> );
+ const target = getInput();
- const input = getInput();
- fireEvent.mouseDown( input );
+ // Hovers the input and presses (without releasing) primary button.
+ await user.pointer( [
+ { target },
+ { keys: '[MouseLeft]', target },
+ ] );
expect( spy ).toHaveBeenCalledTimes( 1 );
} );
} );
describe( 'Value', () => {
- it( 'should update value onChange', () => {
+ it( 'should update value onChange', async () => {
+ const user = setupUser();
const spy = jest.fn();
render( <InputControl value="Hello" onChange={ spy } /> );
-
const input = getInput();
- input.focus();
- fireEvent.change( input, { target: { value: 'There' } } );
- expect( input.value ).toBe( 'There' );
- expect( spy ).toHaveBeenCalledTimes( 1 );
+ await user.type( input, ' there' );
+
+ expect( input ).toHaveValue( 'Hello there' );
+ expect( spy ).toHaveBeenCalledTimes( 6 );
} );
- it( 'should work as a controlled component', () => {
+ it( 'should work as a controlled component', async () => {
+ const user = setupUser();
const spy = jest.fn();
const { rerender } = render(
<InputControl value="one" onChange={ spy } />
);
-
const input = getInput();
- input.focus();
- fireEvent.change( input, { target: { value: 'two' } } );
+ await user.type( input, '2' );
// Ensuring <InputControl /> is controlled.
- fireEvent.blur( input );
+ await user.click( document.body );
- // Updating the value.
+ // Updating the value via props.
rerender( <InputControl value="three" onChange={ spy } /> );
- expect( input.value ).toBe( 'three' );
+ expect( input ).toHaveValue( 'three' );
/*
* onChange called only once. onChange is not called when a
@@ -98,7 +108,6 @@ describe( 'InputControl', () => {
const { rerender } = render(
<InputControl value="Original" onChange={ spy } />
);
-
const input = getInput();
// Assuming <InputControl /> is controlled (not focused)
@@ -106,12 +115,12 @@ describe( 'InputControl', () => {
// Updating the value.
rerender( <InputControl value="New" onChange={ spy } /> );
- expect( input.value ).toBe( 'New' );
+ expect( input ).toHaveValue( 'New' );
// Change it back to the original value.
rerender( <InputControl value="Original" onChange={ spy } /> );
- expect( input.value ).toBe( 'Original' );
+ expect( input ).toHaveValue( 'Original' );
expect( spy ).toHaveBeenCalledTimes( 0 );
} );
} );
diff --git a/packages/components/src/range-control/test/index.js b/packages/components/src/range-control/test/index.js
index c60f12b3b7..f27759012a 100644
--- a/packages/components/src/range-control/test/index.js
+++ b/packages/components/src/range-control/test/index.js
@@ -3,6 +3,11 @@
*/
import { fireEvent, render } 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 <RangeControl { ...props } onChange={ onChange } value={ value } />;
+}
+
+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(
- <RangeControl onChange={ onChange } />
- );
+ const { container } = render( <Component onChange={ onChange } /> );
const rangeInput = getRangeInput( container );
const numberInput = getNumberInput( container );
@@ -39,10 +56,7 @@ describe( 'RangeControl', () => {
it( 'should render with icons', () => {
const { container } = render(
- <RangeControl
- beforeIcon="format-image"
- afterIcon="format-video"
- />
+ <Component beforeIcon="format-image" afterIcon="format-video" />
);
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( <RangeControl min={ 11 } /> );
+ const { container } = render( <Component min={ 11 } /> );
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( <RangeControl max={ 20 } /> );
+ const { container } = render( <Component max={ 20 } /> );
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(
- <RangeControl onChange={ onChange } min={ 10 } max={ 20 } />
+ <Component onChange={ onChange } min={ 10 } max={ 20 } />
);
const numberInput = getNumberInput( container );
@@ -99,7 +113,7 @@ describe( 'RangeControl', () => {
it( 'should keep invalid values in number input until loss of focus', () => {
const onChange = jest.fn();
const { container } = render(
- <RangeControl onChange={ onChange } min={ -1 } max={ 1 } />
+ <Component onChange={ onChange } min={ -1 } max={ 1 } />
);
const rangeInput = getRangeInput( container );
@@ -118,7 +132,7 @@ describe( 'RangeControl', () => {
it( 'should validate when provided a max or min of zero', () => {
const { container } = render(
- <RangeControl min={ -100 } max={ 0 } />
+ <Component min={ -100 } max={ 0 } />
);
const rangeInput = getRangeInput( container );
@@ -133,7 +147,7 @@ describe( 'RangeControl', () => {
it( 'should validate when min and max are negative', () => {
const { container } = render(
- <RangeControl min={ -100 } max={ -50 } />
+ <Component min={ -100 } max={ -50 } />
);
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(
- <RangeControl
- onChange={ onChange }
- min={ 0.1 }
- step={ 0.125 }
- />
+ <Component onChange={ onChange } min={ 0.1 } step={ 0.125 } />
);
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(
- <RangeControl min={ 0 } max={ 10 } />
- );
+ const { container } = render( <Component min={ 0 } max={ 10 } /> );
const rangeInput = getRangeInput( container );
@@ -190,7 +198,7 @@ describe( 'RangeControl', () => {
it( 'should render initialPosition if no value is provided', () => {
const { container } = render(
- <RangeControl initialPosition={ 50 } />
+ <Component initialPosition={ 50 } />
);
const rangeInput = getRangeInput( container );
@@ -200,7 +208,7 @@ describe( 'RangeControl', () => {
it( 'should render value instead of initialPosition is provided', () => {
const { container } = render(
- <RangeControl initialPosition={ 50 } value={ 10 } />
+ <Component initialPosition={ 50 } value={ 10 } />
);
const rangeInput = getRangeInput( container );
@@ -211,7 +219,7 @@ describe( 'RangeControl', () => {
describe( 'input field', () => {
it( 'should render an input field by default', () => {
- const { container } = render( <RangeControl /> );
+ const { container } = render( <Component /> );
const numberInput = getNumberInput( container );
@@ -220,7 +228,7 @@ describe( 'RangeControl', () => {
it( 'should not render an input field, if disabled', () => {
const { container } = render(
- <RangeControl withInputField={ false } />
+ <Component withInputField={ false } />
);
const numberInput = getNumberInput( container );
@@ -229,7 +237,7 @@ describe( 'RangeControl', () => {
} );
it( 'should render a zero value into input range and field', () => {
- const { container } = render( <RangeControl value={ 0 } /> );
+ const { container } = render( <Component value={ 0 } /> );
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( <RangeControl /> );
+ const { container } = render( <Component /> );
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( <RangeControl /> );
+ const { container } = render( <Component /> );
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(
- <RangeControl
- initialPosition={ 10 }
+ <Component
allowReset={ true }
onChange={ spy }
- resetFallbackValue={ 33 }
+ { ...propsForReset }
+ value={ mode === 'controlled' ? 89 : undefined }
/>
);
@@ -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(
- <RangeControl
+ <Component
initialPosition={ undefined }
min={ 0 }
max={ 100 }
allowReset={ true }
resetFallbackValue={ undefined }
+ value={ mode === 'controlled' ? 89 : undefined }
/>
);
8952d45 Introduces a useDraft hook only used by InputControl that re-implements its current behavior of keeping the entered value in favor of an update from props. The crucial difference is that it will do so only for a single render and so does not block undo/redo. This allows components that depend on the behavior to continue doing so with no changes.
32eb218 This fixes the issue I highlighted in #40518 (comment). The issue stems from the how the component is supposed to behave when isPressEnterToChange set to true. That is, its value is updated with every change but onChange is not called until the input is blurred (or Enter is pressed). Because the value has already updated the hook isn't triggered and onChange is not called. Triggering the effect when isDirty changes fixes it.
The diff I provided can be used while checking out this commit to see the unit tests that will fail.
bb6b932 This came about because there was a unit test for BoxControl failing:
● BoxControl › Reset › should persist cleared value when focus changes
expect(received).toBe(expected) // Object.is equality
Expected: ""
Received: "100"
I didn't actually reproduce it testing manually (only tried in Firefox) but I had my suspicions about this from all the previous work and found this change to fix it.
The remaining two commits are quite self-explanatory.
Comment on lines
227
to
232
| reset( | ||
| initialState.value, | ||
| currentState.current._event as SyntheticEvent | ||
| ); | ||
| dispatch( { | ||
| type: actions.RESET, | ||
| payload: { value: initialState.value }, | ||
| } ); |
There was a problem hiding this comment.
Two reasons for switching to dispatch:
resetis typed to require passing an event for the second argument and I didn't want to change the typing because in most cases it would be important to pass the event.- The
exhaustive-depslint rule knowsdispatchis stable and won't complain when it's not in the dependencies.
My remaining peeve with this is I think it'd be better to not reuse RESET here and maybe reintroduce UPDATE. It's only a theoretical concern only and for now a non-issue. The concern would be that someday a component wants to specialize RESET in a way that should not happen every time the value prop has changed.
There was a problem hiding this comment.
Thanks for the explanation of dispatch over reset, it makes sense. I also agree regarding RESET vs UPDATE and that it is a potential future concern but that can be addressed down the line and separately to this PR.
There was a problem hiding this comment.
This sounds like a good point, if we were to keep the internal reducer!
At the same time, after this fix is merged I'd like to explore ways to simplify InputControl (and consequently its derived components), including the removal of the internal reducer (see this comment for more details)
aaronrobertshaw
approved these changes
May 30, 2022
There was a problem hiding this comment.
Great work @stokesman pushing this forward!
Special thanks for the explanatory comments they are fantastic and make a big difference when it comes to reviewing. 🙇
The changes look good and test well for me.
✅ Could still replicate the original issue on trunk
✅ Applying this PR does fix the problem
✅ Tested InputControl, NumberControl, RangeControl, UnitControl, and BoxControl via the editor and storybook and didn't encounter issues
✅ RangeControl issue with resetting to the initialPosition remains fixed
✅ The ColorPicker hex input field works
✅ BoxControl now calls onChange when blurred after clearing the input
✅ No typing errors
✅ Unit tests are passing for all the control components noted above
✅ e2e failure appears unrelated to me
ciampo
changed the title
ciampo
approved these changes
May 30, 2022
There was a problem hiding this comment.
Just echoing @aaronrobertshaw:
- code changes LGTM (and the detailed explanations definitely helped!)
- had a look at
InputControland related in Storybook, everything seems to work as expected - played around in the editor — the original bug is fixed, and I couldn't spot any regressions
- unit tests and e2e all pass (I re-ran the failing e2e tests and they passed)
🚀
|
Thank you Aaron and Marco for the testing and reviews and Riad for the tidy initial work! |
youknowriad
added a commit
that referenced
this pull request
Jun 3, 2022
* Fix undo when changing padding values * Add changelog entry * Have `InputControl` favor entered value for a render cycle * Run propagation effect when `isDirty` changes * Use event existence to eliminate extraneous `onChange` calls * Keep change handler in a ref to pass `react-hooks/exhaustive-deps` * Cleanup legacy `event.persist` calls Co-authored-by: Mitchell Austin <mr.fye@oneandthesame.net>
|
It appears I missed an edge case during my testing of this PR. When a An issue has been created for this (#42455) but thought I'd note it here as well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Discovered in #40505
What?
This PR solves an issue we have in
InputFieldcomponent where it the component receives a new "value" prop, instead of updating its internal state with the new value, it actually does the opposite, it triggers onChange with the old value, causing a potential infinite loop.Why?
Undo is broken if you change values in the padding control using "drag". So this is a bug fix.
Notes
I've found all the nested chain of components UnitControl -> ValueInput -> NumberControl -> InputField to be too complex. I wonder if all that complexity is worth for a component that is supposed to be simple.
My changes are breaking unit tests. I think it's probably other components (RangeControl and UnitControl) that are misusing InputField but I may be wrong here, I might need some help here @ciampo
Testing Instructions
1- Add a group block
2- Click the "padding" input in the sidebar
3- Click and drag the input to change the value
4- While leaving the focus on the input, click "ctrl + z" to undo the changes
5- You'll notice that it undos but very quickly restores to the modified value in trunk
6- in this branch, it undos the value properly
Screenshots or screencast