Components: Preserve ColorPicker hue when saturation picker reaches black/white#75157
Components: Preserve ColorPicker hue when saturation picker reaches black/white#75157JosVelasco wants to merge 5 commits intoWordPress:trunkfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @yllip. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I've found some issues with other blocks, like the heading block. I'll convert this to draft so I can keep working. |
JosVelasco
force-pushed
the
fix/colorpicker-hue-resets-at-black
branch
2 times, most recently
from
df82bb1 to
e91ab3f
Compare
JosVelasco
force-pushed
the
fix/colorpicker-hue-resets-at-black
branch
from
e91ab3f to
34b260e
Compare
|
Updated the approach: instead of caching oldHue, I switched from RgbStringColorPicker to HslColorPicker so hue is passed as a discrete channel and never lost during RGB serialization. A useRef handles the remaining edge case for achromatic colors from hex round-tripping. This is the same pattern used by Figma, Photoshop, and React-Colorful's own examples. Single file change, no API changes. Ready for review! @t-hamano |
|
Thanks, @JosVelasco! Do you mind adding the changelog entry as requested by CI? I also wonder if it's related to #75243. |
There was a problem hiding this comment.
Thank you for working on this. Apart from the feedback left in the comments, after a first look at the Storybook examples (where the component is tested in isolation), I noticed that the thumb shows the selected color:
- generally jitters while dragging it on the picker
- occasionally jumps to the bottom left corner (0, 0, 0)
Kapture.2026-02-09.at.13.28.34.mp4
I assume this is caused by the RGB => HSL conversion, since I cannot reproduce on trunk.
…lack/white Switch the internal color picker from RgbStringColorPicker to HslColorPicker so that hue is passed as a discrete channel rather than being discarded during RGB string serialization. A ref preserves the last meaningful hue for achromatic edge cases (black, white, gray) where hex round-tripping reports hue as 0. This is the standard approach used by professional color pickers (Figma, Photoshop, react-colorful) where HSLA serves as the internal model and each UI control only mutates its own channel.
JosVelasco
force-pushed
the
fix/colorpicker-hue-resets-at-black
branch
from
34b260e to
673fe8b
Compare
|
Thank you for the PR @JosVelasco. I’d love to see this issue fixed. As noted by Aki #47504 (comment), there are some conditions that avoid the issue and that has me curious if there’s some other way to resolve this. I tinkered around a bit and found one that seems to work – use the non-string picker components from |
|
Thanks for looking into this and for the pointer to #35670! The latest revision actually landed in the same place — I switched from RgbStringColorPicker to HsvColorPicker/HsvaColorPicker. Using HSVA (react-colorful's native internal model) seems to eliminate both the hue reset and the thumb jitter that an HSL-based approach introduced. Since the saturation panel works directly in HSVA, there's no intermediate conversion that could lose precision or reset the hue. |
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
I can confirm that the new version of the code doesn't cause the jitter anymore. Also, unit test are overall good, and I tested that they would fail with the code from trunk.
I left some additional feedback, but we're getting there
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
…ing, cache isChromatic
stokesman
left a comment
stokesman
left a comment
There was a problem hiding this comment.
In my testing this works well and the approach makes sense.
With regards to the unit tests, I want to note that they don’t cover the particular interaction that is first highlighted in #47504. Granted, that seems impossible without e2e tests. Still, the added testing of the hex input makes sense. Though I’d rather reduce it down to a single test:
Combine hue preservation tests into one
diff --git a/packages/components/src/color-picker/test/index.tsx b/packages/components/src/color-picker/test/index.tsx
index fbcd75b8b7..3e5e8bf587 100644
--- a/packages/components/src/color-picker/test/index.tsx
+++ b/packages/components/src/color-picker/test/index.tsx
@@ -421,7 +421,7 @@ describe( 'ColorPicker', () => {
} );
describe( 'Hue preservation', () => {
- it( 'should preserve the picker hue when the color becomes black', async () => {
+ it( 'should preserve the picker hue when the color becomes achromatic', async () => {
const user = userEvent.setup();
render(
@@ -439,73 +439,17 @@ describe( 'ColorPicker', () => {
// Blue (#3366ff) has a hue of 225°.
expect( pickerHueSlider ).toHaveValue( 225 );
- // Change color to black via hex input.
const hexInput = screen.getByRole( 'textbox' );
- await user.clear( hexInput );
- await user.type( hexInput, '000000' );
-
- // The picker hue should still be 225° (blue), not 0° (red).
- await waitFor( () => {
- expect( pickerHueSlider ).toHaveValue( 225 );
- } );
- } );
-
- it( 'should preserve the picker hue when the color becomes white', async () => {
- const user = userEvent.setup();
-
- render(
- <ControlledColorPicker
- onChange={ jest.fn() }
- enableAlpha={ false }
- initialColor="#38cc38"
- />
- );
-
- const pickerHueSlider = screen.getByRole( 'slider', {
- name: 'Hue',
- } );
-
- // Green (#38cc38) has a hue of 120°.
- expect( pickerHueSlider ).toHaveValue( 120 );
-
- // Change color to white via hex input.
- const hexInput = screen.getByRole( 'textbox' );
- await user.clear( hexInput );
- await user.type( hexInput, 'ffffff' );
-
- // The picker hue should still be 120° (green), not 0° (red).
- await waitFor( () => {
- expect( pickerHueSlider ).toHaveValue( 120 );
- } );
- } );
-
- it( 'should preserve the picker hue when the color becomes gray', async () => {
- const user = userEvent.setup();
-
- render(
- <ControlledColorPicker
- onChange={ jest.fn() }
- enableAlpha={ false }
- initialColor="#0000ff"
- />
- );
-
- const pickerHueSlider = screen.getByRole( 'slider', {
- name: 'Hue',
- } );
-
- // Pure blue (#0000ff) has a hue of 240°.
- expect( pickerHueSlider ).toHaveValue( 240 );
-
- // Change color to gray via hex input.
- const hexInput = screen.getByRole( 'textbox' );
- await user.clear( hexInput );
- await user.type( hexInput, '999' );
-
- // The picker hue should still be 240° (blue), not 0° (red).
- await waitFor( () => {
- expect( pickerHueSlider ).toHaveValue( 240 );
- } );
+ for await ( const achromaticColor of [ '000', 'fff', '999' ] ) {
+ // Change color via hex input.
+ await user.clear( hexInput );
+ await user.type( hexInput, achromaticColor );
+
+ // The picker hue should still be 225° (blue), not 0° (red).
+ await waitFor( () => {
+ expect( pickerHueSlider ).toHaveValue( 225 );
+ } );
+ }
} );
} );
} );
I’d defer to Marco or others opinion on that though.
Finally I’ll mention that it’d be nice if the remaining similar issue with the HSL inputs could be fixed by integrating with the newly added logic in some way. Yet I suppose that’d be fine to tackle separately. Here’s a quick screen recording of what I’m referring to:
hsl-hue-loss.mp4
There was a problem hiding this comment.
LGTM 🚀
Let's unify the newly added unit tests into one test, as suggested by @stokesman, and then we can merge this PR.
@JosVelasco , would you also be up for looking into a follow-up PR to fix the related issue that @stokesman pointed out?
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @yllip. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Consolidated the tests into a single test, as suggested — thanks, @stokesman! |
|
To fix the HSL inputs issue, I think we'd need a full refactor, so this PR could wait. This is what I have so far, but I haven't started/tested yet: component.tsx: Keep internal HSLA state as the single source of truth (authoritative H/S). |
|
Here is the new PR draft: #75493 |
|
Closing in favour of #75493, which takes a more comprehensive approach by lifting HSLA state to the parent component and preserving both hue and saturation (not just hue) through achromatic colors. |
4 participants
What?
Fixes the hue slider jumping to red when dragging the saturation picker to black, white, or gray areas.
Why?
When dragging to color-space extremes like black (
v=0), white (s=0, v=100), or gray (s=0), the hue becomes mathematically undefined. The previous implementation usedRgbStringColorPicker, which serialized the color as an RGB string (e.g."rgb(0,0,0)"). Since RGB carries zero hue information for achromatic colors, every re-render fedh: 0back into react-colorful, causing the hue slider to snap to red.This is a well-known problem in color science and has been solved by every major color picker — Figma, Photoshop, react-colorful's own examples, MyPaint (mypaint/mypaint#838), and react-color's
oldHuepattern (casesandberg/react-color#29).How?
Switches from
RgbStringColorPickertoHsvColorPicker/HsvaColorPicker— HSVA is react-colorful's native internal model. Because the saturation panel works directly in HSVA, there is no intermediate conversion that could lose precision or reset the hue. This also eliminates the thumb jitter that an HSL-based approach would introduce (HSL→hex→HSL round-trips lose floating-point precision, causing the thumb to shift on re-render).A
useRefpreserves the last meaningful hue for the one remaining edge case: when the parent re-renders with a hex-derived achromatic color (e.g.#000000→hsv(0, 0%, 0%)), the ref substitutes the last known chromatic hue instead of0.A second pair of refs (
lastHsvRef/lastHexRef) caches the most recent HSV↔hex pair. When the parent re-renders with the same hex we last emitted, the cached HSV is returned directly — bypassing the lossy hex→HSV conversion and preventing precision drift during dragging.This is the standard approach — HSVA as the internal model, where each UI control only mutates its own channel. The hue is structurally isolated from saturation/value changes. The alternative (caching
oldHue) is what react-color uses but is more fragile and harder to maintain.Changes: ~50 lines in a single file (
picker.tsx), no API changes. Added unit tests for hue preservation and a changelog entry.Testing Instructions
#3366FF(blue) in the hex inputAlso test with:
#38CC38(green) to confirm hue preservation works for other hues#000000,#FFFFFF,#999999) then dragging the saturation pickerBefore / After
https://github.com/user-attachments/assets/b9c89a9d-bb9b-40a0-bdc8-7ba1b2ffdd9a
https://github.com/user-attachments/assets/137c5f7e-1f60-470a-bef5-e6be5c5f5ddb
Fixes #47504
AI assistance: Yes
Tool(s): Claude (Anthropic)
Used for: Researching how other editors solve this problem (Figma, Photoshop, react-color, react-colorful, MyPaint), identifying HSVA as the standard solution, debugging the HSL jitter issue, and implementation. Final code was reviewed, tested, and validated by me.