Skip to content

fix: throw on invalid rgb() and rgba() color strings - #10580

Open
giaBaoJS wants to merge 1 commit into
adobe:mainfrom
giaBaoJS:fix/rsp-0906b
Open

fix: throw on invalid rgb() and rgba() color strings#10580
giaBaoJS wants to merge 1 commit into
adobe:mainfrom
giaBaoJS:fix/rsp-0906b

Conversation

@giaBaoJS

@giaBaoJS giaBaoJS commented Sep 6, 2026

Copy link
Copy Markdown

Closes

parseColor is documented as "Throws an error if the string could not be parsed", and the hsl(), hsb() and #hex branches all do. The rgb()/rgba() branch does not: it hands back a Color whose channels are NaN.

Intent: make the four parse branches agree on what an unparseable string is, so callers can keep relying on the documented throw. I picked the smallest change that does that rather than rewriting the rgb branch to a strict regex like the hsl/hsb ones, because the rgb branch also feeds the hex path and shares the colors array with it.

Reproduction:

import {parseColor} from '@react-stately/color';

parseColor('rgb(a, b, c)');      // -> Color, toString('rgba') === 'rgba(NaN, NaN, NaN, 1)'
parseColor('rgba(0, 0, 0, abc)') // -> Color with alpha NaN
parseColor('rgb(, , )')          // -> rgba(0, 0, 0, 1)

parseColor('hsl(a, b, c)');      // -> throws 'Invalid color value: hsl(a, b, c)'
parseColor('hsb(a, b, c)');      // -> throws
parseColor('#ggg');              // -> throws

Root cause: RGBColor.parse matches with /^rgba?\((.*)\)$/, so anything between the parens is accepted and passed to Number(). Number('a') is NaN, ?? does not catch NaN, and clamp(NaN, 0, 255) is NaN, so the final guard (colors[0] === undefined || ...) never fires. HSL_REGEX and HSB_REGEX validate the numeric payload inside the regex instead, which is why those two behave correctly.

This matters beyond the direct call: useColor wraps parseColor in a try/catch and returns undefined for invalid input. Without the throw, a NaN Color flows into useColorAreaState / useColorWheelState / useColorSliderState and surfaces later as NaN thumb positions or an error thrown from a different stack.

The fix rejects components that are empty or do not parse to a finite number, so RGBColor.parse returns undefined and parseColor falls through to its existing Invalid color value error. The empty check is separate because Number('') is 0, not NaN. Valid input is unaffected, including the existing out-of-range clamping test (rgba(300, -10, 0, 4)).

What the tests assert: three new cases in packages/react-stately/test/color/Color.test.tsx under the existing rgb describe, each asserting parseColor throws Invalid color value: <input> for non-numeric channels, a non-numeric alpha, and empty channels. They mirror the should throw on invalid hex value test already in the hex describe. Reverting only Color.ts turns all three red with "Received function did not throw"; the other 26 cases in the file stay green either way.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices
  • I understand every change in this PR and can explain why it's there.
  • If AI-assisted, I followed our AI contribution guidance and pointed my assistant at CLAUDE.md.

No issue exists for this; I found it while reading the color parsers. Happy to open one first if you would prefer that.

Docs are unchanged: the parseColor docstring already promises the throw, so this makes the code match the existing documentation rather than changing it.

📝 Test Instructions:

yarn jest packages/react-stately/test/color/Color.test.tsx

29 passing, up from 26. To see the failure, revert packages/react-stately/src/color/Color.ts and rerun: the three new cases fail with "Received function did not throw".

I also ran the wider color surface (packages/react-stately/test/color, packages/react-aria/test/color, packages/@adobe/react-spectrum/test/color, packages/react-aria-components/test/Color*): 176 passing before, 179 after, no change in failures.

This is parser logic with no rendered output, so there was nothing to check for mouse/touch/keyboard/screen reader, LTR/RTL, light/dark, or sizing. AI-assisted, reviewed and verified by me.

🧢 Your Project:

Personal contribution.

parseColor('rgb(a, b, c)') returned a Color with NaN channels instead
of throwing. The rgb branch matched with /^rgba?\((.*)\)$/ and passed
whatever was inside the parens through Number(), so NaN reached the
RGBColor constructor. The hsl and hsb branches validate their numeric
payload inside the regex, so they throw as documented.

Reject components that are empty or do not parse to a finite number,
which lets parseColor fall through to its "Invalid color value" error.

@snowystinger snowystinger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants