fix: allow ColorSwatchPicker to have no selection#10331
Conversation
ColorSwatchPicker always defaulted to black (#000000) via useColorPickerState, and disallowEmptySelection meant a black swatch could never be deselected — if the swatch list happened to contain black, it was permanently stuck as selected. ColorSwatchPicker now manages its own controlled/uncontrolled state directly (instead of going through useColorPickerState, which is shared with the more complex ColorPicker and shouldn't be changed to support a nullable color), so value/defaultValue/onChange can be null, and selectedKeys is empty when nothing is selected. The two Spectrum wrapper components (@adobe/react-spectrum and @react-spectrum/s2) just had their prop types widened to match, no behavior change there. Closes adobe#7918 Co-authored-by: Claude <noreply@anthropic.com>
The value/onChange examples used plain useState(parseColor(...)), whose setter no longer matched the widened onChange type. Type the state as Color | null and guard the display text, matching the existing ColorField doc example's pattern.
|
Fixed — the |
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
I guess the other option would be to have a way inside the Color class of representing "no color"
I don't know if that'd truly be no color, or just a fully transparent color, or something like that
|
|
||
| function Example() { | ||
| let [value, setValue] = useState(parseColor('#A00')); | ||
| let [value, setValue] = useState<Color | null>(parseColor('#A00')); |
There was a problem hiding this comment.
Just highlighting for the rest of the team. This now requires additional typescript and checks against null in code as seen in this file.
We went through some similar steps back in Select
https://github.com/adobe/react-spectrum/pull/2676/changes
https://github.com/adobe/react-spectrum/pull/7888/changes#diff-1de776587d3b665172f7a4e80f44244c69d868bd3ac9c8d8b6e3f6235beaa1ac
| ref: ForwardedRef<HTMLDivElement> | ||
| ) { | ||
| [props, ref] = useContextProps(props, ref, ColorSwatchPickerContext); | ||
| let state = useColorPickerState(props); |
There was a problem hiding this comment.
changes should be made in the stately hook, we should still be using the hook here. was there a reason you dropped it?
There was a problem hiding this comment.
I looked at the precedent for Select (#2676) — that PR only changed a type declaration (selectedKey?: Key → Key | null in selection.d.ts), the underlying selection hook already tolerated null at runtime. useColorPickerState is different: its code actively rejects null (if (color != null) { setColor(...) }) and hardcodes #000000 as the default, so widening its type alone wouldn't be enough — its actual logic would need to change, and it's shared with ColorPicker, whose slider/wheel components assume state.color" is always a real Color. That's why I kept the state local to ColorSwatchPicker` instead. Happy to revisit if you think the risk is worth it for consistency.
There was a problem hiding this comment.
That's a good point. For now lets create a new stately hook we can use here. Make it look the same as the original hook so that it's easier to compare against (no need for all the memos)
|
Looks like you need to sign the CLA https://github.com/adobe/react-spectrum/blob/main/CONTRIBUTING.md#contributor-license-agreement |
|
Thanks for the feedback! I did consider that route, but I think it'd carry more risk than benefit here: This also matches the existing precedent in |
| expect(options[1]).toHaveAttribute('aria-selected', 'false'); | ||
| }); | ||
|
|
||
| it('supports deselecting a swatch, including black', async function () { |
There was a problem hiding this comment.
Given how Selects normally work, I don't think we should be allowing clicking on the same swatch to deselect, can probably just set disallowEmptySelection on the ListBox. Doing reset through a form or button should still work with null
| ref: ForwardedRef<HTMLDivElement> | ||
| ) { | ||
| [props, ref] = useContextProps(props, ref, ColorSwatchPickerContext); | ||
| let state = useColorPickerState(props); |
There was a problem hiding this comment.
That's a good point. For now lets create a new stately hook we can use here. Make it look the same as the original hook so that it's easier to compare against (no need for all the memos)
| () => (typeof defaultValueProp === 'string' ? parseColor(defaultValueProp) : defaultValueProp), | ||
| [defaultValueProp] | ||
| ); | ||
| let [color, setColor] = useControlledState<Color | null>(value, defaultValue ?? null, onChange); |
There was a problem hiding this comment.
I just noticed this changes the default color as well, will be easier to see little differences like that when it's in a hook that mirrors the original
Per review: - Move the controlled/uncontrolled color state into a new useColorSwatchPickerState stately hook, mirroring useColorPickerState's structure (no inline memos in the component), so the two are easy to diff against each other. - Restore disallowEmptySelection: once a swatch is selected, clicking it again no longer deselects it, matching how selection works elsewhere. Clearing the selection is only done by passing a controlled null value (e.g. from a form reset).
|
Done — pushed a new commit addressing all three points:
Let me know if the new hook's shape looks right! |
2 participants
Closes #7918
✅ Pull Request Checklist:
ListBox/aria-selectedsemantics, just when a selection exists at all).📝 Test Instructions:
ColorSwatchPickeralways initialized its selection to black (#000000) viauseColorPickerState, anddisallowEmptySelectionmeant a black swatch could never be deselected. If your swatch list happened to include black, it was permanently stuck as "selected" with no way to represent "nothing chosen."ColorSwatchPickernow manages its own controlled/uncontrolled state directly instead of going throughuseColorPickerState— which is shared with the much more complexColorPickerand shouldn't be changed to support a nullable color, sinceColorPicker's slider/wheel components assumestate.coloris always a realColor.value/defaultValue/onChangeonColorSwatchPickercan now benull, andselectedKeysis empty when nothing is selected.The two Spectrum wrapper components (
@adobe/react-spectrumand@react-spectrum/s2) only had their prop types widened (ValueBase<string | Color, Color>→ValueBase<string | Color | null, Color | null>) to stay consistent — no behavior change in those files, they just spread props through.To verify manually: render a
ColorSwatchPickerwith a swatch colored#000and novalue/defaultValue— no swatch should appear selected initially, and clicking the black swatch should select it and clicking it again should deselect it (previously impossible).🧢 Your Project:
N/A — found while investigating this issue.
Parts of this fix (root-cause analysis and implementation) were produced with the help of Claude Code — credited via the
Co-authored-bytrailer on the commit. I reviewed and understood the change before opening this PR.Verification performed: full monorepo type-check (
yarn check-types) passes,oxlint/oxfmtclean on all touched files, and the fullColorSwatchPickertest suites in bothreact-aria-componentsand@adobe/react-spectrumpass (11/11), including two new tests covering no-selection-by-default and deselecting black specifically.