Finish making InputControl et al. more controllable
#40568
Conversation
stokesman
added
the
[Feature] UI Components
|
Size Change: +53 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
I just pushed a quick fix to make sure that the CI checks could complete and added a small comment to explain why the |
ciampo
added
[Type] Bug
There was a problem hiding this comment.
Thank you @stokesman for you work on this!
Overall the code changes all make sense to me, and so my idea would be to merge this ASAP into #40568 (cc @youknowriad ) so that we can all give a final look at the fix on that PR.
The good amount of unit tests that we have also reassures me that these changes should be working fine.
I spent some more time checking the components in Storybook, and noticed a regression in the behavior of the "Reset" button in RangeControl:
trunk:
range-control-reset-before.mp4
This PR:
range-control-reset-after.mp4
Basically, with the changes from this PR, the "Reset" button needs to be clicked twice before effectively resetting the value. Once this is ironed out, it would be good to add a unit test to RangeControl to avoid this regression in the future.
|
So the remaining two 3800d28, ff6e10a & e427a0e 38c0158, 8930905 & cb00749 fa2723d Before fa2723dinput-control-stuck-state-from-blur.mp4Afterinput-control-unstuck-state-from-blur.mp4 |
I can confirm that those tests on
This is great to hear! It's quite late for me to have a look today, but I'll definitely do so tomorrow. Thank you once again for your efforts here 🙏 |
Yes, it seems on trunk (and here) that suite is just somehow working and any little change is likely to make some other test fail. Notice that the test that failed was not the one with the change. I think it's because there are actually a few more places in that file with a missing We could fix the others in this PR or if we did it in a separate PR I don't think it would even cause a hiccup here. |
| @@ -215,7 +215,7 @@ describe( 'UnitControl', () => { | |||
| expect( input.value ).toBe( '300px' ); | |||
| expect( state ).toBe( 50 ); | |||
|
|
|||
| user.keyboard( '{Escape}' ); | |||
There was a problem hiding this comment.
This change can be dropped after rebasing on top of trunk since it was included in #40790
| onChangeProp?.( | ||
| `${ validParsedQuantity ?? '' }${ validParsedUnit }`, | ||
| changeProps | ||
| ); |
There was a problem hiding this comment.
Thank for for the hard work and the thoughtful explanations, @stokesman !
Honestly, the complexity that this component has reached makes it very complicated to code review. I did my best, but there's the chance I may have still missed something. At least the core behavior should be verified by the unit tests that we have in place.
Once the last pending comment, feel free to merge this PR into #40518.
At that point, we will be able to:
- rebase on top of
trunkand remove the changes that have already been merged in separate PRs (one and two) - Give a final, through round of review to InputControl: Fix undo when changing padding values #40518 before hopefully being able to merge this fix
(cc @youknowriad )
| 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 ] ); |
There was a problem hiding this comment.
It would be good to add some inline comments here — eg. why are we saving the input's value, and reapplying it to the same input in the next rendering frame?
There was a problem hiding this comment.
I added a comment in 085fd03 and then while double checking things while testing out the change that was to be 2b14b0f, I discovered the hook didn't need to handle NaN values so I made 29634d7. The issue (as non-critical as could be) was the first update after a NaN entry failing to re-sync the input:
range-control-stuck-one-update-after-NaN.mp4
I'd missed this before #40568 (comment) because I dragged the slider instead of clicking it once.
| | ChangeEvent< HTMLInputElement > | ||
| | PointerEvent< HTMLInputElement >, | ||
| } ); | ||
| } | ||
| }, [ state.value ] ); | ||
| refEvent.current = null; | ||
| }, [ state.value, state.isDragging, isFocused ] ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙇
|
Just popping by to say that I won't be around until Wednesday (18th), so feel free to merge this PR into #40518 whenever Riad agrees to. We can then all take that PR for a solid, deep test before merging into |
| @@ -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. | |||
There was a problem hiding this comment.
When does this happen (action without type)?
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
I'd have just used another action but it's not important :)
|
Ideally there might have been less roundabout on the way to these changes. I'd much prefer that the review burden were less but I'm not doubtful of the result. Thanks again to all involved for the help! |
* 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>
* 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>
This reverts commit 769e69d.
* 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>
What?
Builds upon #40518 to add the changes to
RangeControlandnecessitated by the makingUnitControlInputControlcontrollable regardless of its focused state.Why?
RangeControlneeds changes is because it depends on itsNumberControlbeing self-controlled while focused (i.e. ignoring thevalueprop it gets fromRangeControl) so that whenRangeControlactively constrains its state value it does not interfere with the entry of the value in the focused input.I've not yet pinpointed whyUPDATE: It turns out this was an existing issue but our test that would catch it, wasn't doing so on trunk. The related changes from this PR have already been extracted/merged with #40796 so they are to be removed from this branch.UnitControlrequires a change but without it, the issue is that it callsonChangetwice for a single change (from ablurevent). That probably wouldn't have hurt a thing but it's nice that our unit tests caught this.How?
RangeControlto temporarily keep entered values in its number input if they are out-of-range or non-numeric.UnitControlstops callingonChangeexplicitly frommayUpdateUnitbecause apparentlyInputControlnow does so on its ownTesting Instructions
Unit tests pass.
Try finding any differences manually testing
RangeControland.UnitControl