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

[stokesman]

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

1. Open a Post or Page.
2. Insert a Heading Block.
3. Open the Text color settings for the block.
4. Open the Custom Color Picker.
5. Show the detailed inputs.
6. Clear the input.
7. Enter a color value.
8. Verify that the value entered is kept as is and the visible color changes as different values are entered.
9. Enter a shorthand value with a letter e.g. c03.
10. Click outside the input.
11. Verify that the value becomes the longhand form and its letters are changed to uppercase.
12. Focus the input.
13. Press the keyboard shortcut for undo.
14. 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

[stokesman]

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]

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.:
  • Remove forced focus of InputControl on mousedown #41118
  • UnitControl: Revamp support for changing unit by typing #39303
  • Add paste handling to #39303 #40168

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]

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]

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]

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.