Make text/number inputs of `ColorPicker` keep draft values while focused #41264

stokesman · 2022-05-24T02:41:43Z

What?

In support of the changes to InputControl in #40518 this ensures the text and number inputs of ColorPicker will keep a "drafted" value while the input is focused (like they do in trunk but without relying on the current InputControl behavior).

Why?

To avoid interference with typing a value in the input and preserve instant updates during entry.

How?

Adds logic (in a hook) to prefer a draft value over the state while focused yet restore control to props after if a change comes in that wasn't initiated from the input (e.g. global undo).

Testing Instructions

Open a Post or Page.
Insert a Heading Block.
Open the Text color settings for the block.
Open the Custom Color Picker.
Show the detailed inputs.
Clear the input.
Enter a color value.
Verify that the value entered is kept as is and the visible color changes as different values are entered.
Enter a shorthand value with a letter e.g. c03.
Click outside the input.
Verify that the value becomes the longhand form and its letters are changed to uppercase.
Focus the input.
Press the keyboard shortcut for undo.
Verify that the actual selected color in the parent dialog reverts to what it was before any value changes*

*The result of the last step can be confusing because the color picker itself will revert to its last local state and instead of the editor’s revision as can be seen in its parent dialog . This is an existing issue with the component and an edge case besides.

Screenshots or screencast

Issue with the hex input of `ColorPicker` from #40518

color-picker-fail-on-40518.mp4

The hex input on this branch

color-picker-draftable-on-40518.mp4

While I recorded a different set of actions they both include typing into the hex input so this demonstrates the fix. It also demonstrates the undo works (aside from the caveat mentioned in testing instructions) even while the input is focused.

Issue with `InputWithSlider` of `ColorPicker` from #40518

color-picker-iput-with-slider-interference.mp4

This is the same issue with the hex input but isn't likely to be as commonly encountered. What is also seen in the screen recording is a very related but separate issue with these controls #36703.

`InputWithSlider` on this branch

color-picker-iput-with-slider-draftable.mp4

* 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Lena Morita <[email protected]>

stokesman · 2022-05-25T05:12:35Z

I wanted to highlight that 96b0a2e and 301c3d4 are extraneous to this PR. I'd be glad to break them into a separate PR if the changes are deemed worthwhile. The motivation was to avoid having to write undefined checks or use as string to silence typescript when there's actually no possibility of undefined. 301c3d4 could use a little more scrutiny though and perhaps a typing change to UnitControlOnChangeCallback would be a better alternative to it.

ciampo · 2022-05-25T13:28:27Z

Hey @stokesman , thank you for working on this.

I'd like to get some clarity on the status of the various PRs that involve InputControl & derived components.

This PR is currently required in order to unblock InputControl: Fix undo when changing padding values #40518, an important bug fix.
Then, we have a bunch of other open PRs related to InputControl & co.:

The reason I'm bringing this up is because in the past months we've been working on a lot of potential changes to these components, but we haven't been able to merge most of these changes to trunk, as it seems that every "fix" causes a new related breakage, while making the code more and more difficult to understand.

If this situation persists, I wonder if we should just consider rewriting this chain of components in a simpler way.

What's your point of view, and how far from being ready is this PR in your opinion?

I wanted to highlight that 96b0a2e and 301c3d4 are extraneous to this PR. I'd be glad to break them into a separate PR if the changes are deemed worthwhile.

Would this new PR be merged into trunk, or into one of the existing PRs?

stokesman · 2022-05-25T16:41:25Z

I wonder if we should just consider rewriting this chain of components in a simpler way.

Some time ago I expected they'd be swapped out with near equivalents from the g2 components library. Which as I understood it had them written in simpler ways. I liked that idea. I'd also have no qualms with otherwise rewriting them but I suppose either option may precipitate other difficulties.

I do feel there's opportunity to simplify and improve the comprehensibility and flexibility of their implementations without full rewrites. Something I've experimented with was improving the extensibility of InputControl by breaking it into a hook/component combo. That allows simplifying it a fair bit because it doesn't have to carry state or implement stubbed actions for features it doesn't actually use. E.g. Drag behavior/state moves into NumberControl.

I also think your idea of possibly removing isPressEnterToChange is interesting. If we could get to that and make the hook/component separation it might be possible to drop the reducer and use simpler state.

how far from being ready is this PR in your opinion?

Aside from the question of the last two commits I'd say this is ready. Though now I'm curious if the approach in this PR for useDraft might be applicable directly to InputControl in trunk. If so, it could be a simpler alternative to #40518 (and thus also obviate this PR). So I think I'll have a look at that before I mark this as ready (@mirka, 👋 heads up so you don't bother with a review just yet).

Would this new PR be merged into trunk, or into one of the existing PRs?

I'd like to make it on trunk but after a rethink I'll say it'd be better to spare shaving that yak for now. If I do mark this PR as ready it will be after updating to make it work without the last two commits.

ciampo · 2022-05-26T21:28:33Z

Some time ago I expected they'd be swapped out with near equivalents from the g2 components library.

I believe that was the plan, yes — that was more or less at the time I had started working on components. I believe ultimately the task was put on pause given the complexity of reconciling the way input components are structured currently vs the way g2 components are.

I do feel there's opportunity to simplify and improve the comprehensibility and flexibility of their implementations without full rewrites. Something I've experimented with was improving the extensibility of InputControl by breaking it into a hook/component combo. That allows simplifying it a fair bit because it doesn't have to carry state or implement stubbed actions for features it doesn't actually use. E.g. Drag behavior/state moves into NumberControl.

I also think your idea of possibly removing isPressEnterToChange is interesting. If we could get to that and make the hook/component separation it might be possible to drop the reducer and use simpler state.

That's definitely something I'd like us to explore. I think that the major sources of complexity are:

handling controlled vs uncontrolled component's state
handling the isPressEnterToChange
handling validation (especially with respect to the component being focused or blurred)
handling drag gestures

Removing and/or extracting some of these functionalities may simplify InputControl (and its chain of components) by quite a bit.

While we move to a hook + component setup, I would also like to explore removing the internal reducer, which I believe just adds a further layer of abstraction which makes it quite hard to follow the "flow" of the logic in the component.

So I think I'll have a look at that before I mark this as ready
...
If I do mark this PR as ready it will be after updating to make it work without the last two commits.

Makes sense. Let us know once this PR is ready for review, and which overall strategy you're opting for (removing the last 2 commits and merging the PR back into #40518, or generalising the useDraft approach to InputControl as an alternative to #40518).

stokesman · 2022-05-29T20:52:47Z

I would also like to explore removing the internal reducer, which I believe just adds a further layer of abstraction which makes it quite hard to follow the "flow" of the logic in the component.

🎯 👍

Let us know once this PR is ready for review, and which overall strategy you're opting for (removing the last 2 commits and merging the PR back into #40518, or generalising the useDraft approach to InputControl as an alternative to #40518).

I was able to get the useDraft approach working on InputControl and it's actually not a complete alternative to #40518, because it still needs the change from Riad’s first commit which removes the use of focus in determining syncronization direction. It's still much simpler overall and I've merged it into that PR. Going to close this one.

youknowriad and others added 5 commits May 17, 2022 11:25

Fix undo when changing padding values

4046235

Fix RangeControl reset from values out-of-range by multiple digits

b91a418

Add changelog entry

5150ade

Make hex input of ColorPicker keep draft values while focused

b67b3b4

stokesman added the [Package] Components /packages/components label May 24, 2022

ciampo self-requested a review May 24, 2022 14:15

stokesman added 4 commits May 24, 2022 10:12

Support draft values in InputWithSlider by adding useDraft hook

699ce90

Switch to hook for draft value support in HexInput

9d7dbc9

Update type of nextValue in InputChangeCallback

96b0a2e

Have UnitControl avoid sending undefined through onUnitChange

301c3d4

stokesman changed the title ~~Make hex input of ColorPicker keep draft values while focused~~ Make text/number inputs of ColorPicker keep draft values while focused May 24, 2022

ciampo assigned stokesman May 25, 2022

ciampo requested a review from mirka May 25, 2022 13:29

stokesman force-pushed the fix/input-field-reset-behavior branch from 5150ade to 3ac6020 Compare May 29, 2022 20:37

stokesman closed this May 29, 2022

ciampo mentioned this pull request May 30, 2022

InputControl: Fix undo when changing padding values #40518

Merged

stokesman mentioned this pull request Jul 27, 2022

InputControl: Fix acceptance of falsy values in controlled updates #42484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make text/number inputs of `ColorPicker` keep draft values while focused #41264

Make text/number inputs of `ColorPicker` keep draft values while focused #41264

stokesman commented May 24, 2022 •

edited

Loading

stokesman commented May 25, 2022

ciampo commented May 25, 2022

stokesman commented May 25, 2022

ciampo commented May 26, 2022 •

edited

Loading

stokesman commented May 29, 2022

Make text/number inputs of ColorPicker keep draft values while focused #41264

Make text/number inputs of ColorPicker keep draft values while focused #41264

Conversation

stokesman commented May 24, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

Issue with the hex input of ColorPicker from #40518

The hex input on this branch

Issue with InputWithSlider of ColorPicker from #40518

InputWithSlider on this branch

stokesman commented May 25, 2022

ciampo commented May 25, 2022

stokesman commented May 25, 2022

ciampo commented May 26, 2022 • edited Loading

stokesman commented May 29, 2022

Make text/number inputs of `ColorPicker` keep draft values while focused #41264

Make text/number inputs of `ColorPicker` keep draft values while focused #41264

stokesman commented May 24, 2022 •

edited

Loading

Issue with the hex input of `ColorPicker` from #40518

Issue with `InputWithSlider` of `ColorPicker` from #40518

`InputWithSlider` on this branch

ciampo commented May 26, 2022 •

edited

Loading