fix(theme): contained buttons honor color=error/success/warning#1674
Conversation
The MuiButton styleOverride painted every .MuiButton-contained with the brand
(keppel) background regardless of the `color` prop and defined no containedError,
so `<Button variant="contained" color="error">` rendered brand green. Every
destructive action across consuming apps was silently miscoloured, and callers
had to fight it with per-site sx specificity hacks.
Add &.MuiButton-containedError / containedSuccess / containedWarning rules using
the matching palette.background.{error,success,warning} tokens, layered after the
brand &.MuiButton-contained rule so they win on source order at equal
specificity; the higher-specificity .Mui-disabled rules still win when disabled.
primary/default and secondary are unchanged, so only the currently-broken
semantic-contained buttons change.
Signed-off-by: Lee Calcote <lee.calcote@layer5.io>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for semantic color properties (error, success, and warning) on contained MuiButton components to prevent them from defaulting to the brand color, and includes corresponding unit tests. The reviewer's feedback highlights an accessibility issue where hardcoding white text on a warning background violates WCAG contrast guidelines, and suggests using theme.palette.getContrastText to dynamically resolve text colors. Additionally, the reviewer recommends adding support and tests for the missing info semantic color.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| '&.MuiButton-containedError': { | ||
| color: constant?.white, | ||
| backgroundColor: error?.default, | ||
| '&:hover': { | ||
| backgroundColor: error?.hover | ||
| } | ||
| }, | ||
| '&.MuiButton-containedSuccess': { | ||
| color: constant?.white, | ||
| backgroundColor: success?.default, | ||
| '&:hover': { | ||
| backgroundColor: success?.hover | ||
| } | ||
| }, | ||
| '&.MuiButton-containedWarning': { | ||
| color: constant?.white, | ||
| backgroundColor: warning?.default, | ||
| '&:hover': { | ||
| backgroundColor: warning?.hover | ||
| } | ||
| }, |
There was a problem hiding this comment.
This block has two main issues:
- Accessibility / Contrast Issue: Hardcoding
color: constant?.white(near-white) for the warning button results in white text on a yellow background (Colors.yellow[30]/Colors.yellow[40]), which severely violates WCAG contrast guidelines and makes the text unreadable. - Missing
infoButton: Theinfosemantic color is also supported by MUI buttons but was omitted, meaning<Button variant="contained" color="info">would still incorrectly render with the brand green background.
To solve both issues robustly, we can use MUI's built-in theme.palette.getContrastText to dynamically calculate the correct WCAG-compliant text color for all semantic buttons (error, success, warning, info), and add the missing &.MuiButton-containedInfo override.
'&.MuiButton-containedError': {
color: error?.default ? theme.palette.getContrastText(error.default) : constant?.white,
backgroundColor: error?.default,
'&:hover': {
backgroundColor: error?.hover
}
},
'&.MuiButton-containedSuccess': {
color: success?.default ? theme.palette.getContrastText(success.default) : constant?.white,
backgroundColor: success?.default,
'&:hover': {
backgroundColor: success?.hover
}
},
'&.MuiButton-containedWarning': {
color: warning?.default ? theme.palette.getContrastText(warning.default) : constant?.white,
backgroundColor: warning?.default,
'&:hover': {
backgroundColor: warning?.hover
}
},
'&.MuiButton-containedInfo': {
color: info?.default ? theme.palette.getContrastText(info.default) : constant?.white,
backgroundColor: info?.default,
'&:hover': {
backgroundColor: info?.hover
}
},There was a problem hiding this comment.
Great catch on both. Adopted theme.palette.getContrastText(...) for the label colour on all four semantic variants (so the yellow warning gets readable dark text instead of white), and added &.MuiButton-containedInfo. Done in d2e8e8c.
| const { | ||
| palette: { | ||
| background: { brand, hover }, | ||
| background: { brand, hover, error, success, warning }, |
There was a problem hiding this comment.
Destructure the info semantic color from background so it can be used to style the info contained button.
| background: { brand, hover, error, success, warning }, | |
| background: { brand, hover, error, success, warning, info }, |
There was a problem hiding this comment.
Done - info is now destructured from background. d2e8e8c.
| it('also honors success and warning contained buttons', () => { | ||
| const { theme, styles } = rootStyles('light'); | ||
| expect(styles['&.MuiButton-containedSuccess'].backgroundColor).toBe( | ||
| theme.palette.background.success.default | ||
| ); | ||
| expect(styles['&.MuiButton-containedWarning'].backgroundColor).toBe( | ||
| theme.palette.background.warning.default | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Add an assertion for &.MuiButton-containedInfo to verify that the info semantic color is also correctly resolved.
it('also honors success, warning, and info contained buttons', () => {
const { theme, styles } = rootStyles('light');
expect(styles['&.MuiButton-containedSuccess'].backgroundColor).toBe(
theme.palette.background.success.default
);
expect(styles['&.MuiButton-containedWarning'].backgroundColor).toBe(
theme.palette.background.warning.default
);
expect(styles['&.MuiButton-containedInfo'].backgroundColor).toBe(
theme.palette.background.info.default
);
});There was a problem hiding this comment.
Done - the test now asserts the info background, plus a new assertion that the label colour equals the getContrastText-derived value (so the WCAG fix is covered). d2e8e8c.
There was a problem hiding this comment.
Pull request overview
Fixes Sistent’s MuiButton theme override so contained buttons with semantic color="error" | "success" | "warning" use the corresponding semantic background tokens instead of always using the brand (keppel) background. This aligns contained button visuals with MUI’s semantic color contract and removes the need for downstream sx specificity workarounds.
Changes:
- Extend
MuiButton.styleOverrides.rootto add&.MuiButton-containedError|Success|Warningbackground/hover rules after the brand contained rule. - Add Jest coverage asserting the resolved contained semantic background tokens (light + dark).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/theme/components/button.modifier.ts | Adds contained semantic color selectors to restore correct error/success/warning backgrounds. |
| src/testing/theme.test.ts | Adds regression tests validating semantic contained backgrounds resolve to semantic tokens (light/dark). |
| '&.MuiButton-containedWarning': { | ||
| color: constant?.white, | ||
| backgroundColor: warning?.default, | ||
| '&:hover': { | ||
| backgroundColor: warning?.hover | ||
| } | ||
| }, |
There was a problem hiding this comment.
Fixed - warning (and the other semantic variants) now derive the label via theme.palette.getContrastText(background), so the yellow warning stays readable in both light and dark modes. d2e8e8c.
| '&.MuiButton-containedError': { | ||
| color: constant?.white, | ||
| backgroundColor: error?.default, | ||
| '&:hover': { | ||
| backgroundColor: error?.hover | ||
| } | ||
| }, |
There was a problem hiding this comment.
Fixed - the error label colour now comes from theme.palette.getContrastText(error.default) too, so it stays readable across palette modes. d2e8e8c.
| const root = MuiButton.styleOverrides!.root as (arg: { theme: typeof theme }) => Record< | ||
| string, | ||
| { backgroundColor?: string; ['&:hover']?: { backgroundColor?: string } } | ||
| >; | ||
| return { theme, styles: root({ theme }) }; |
There was a problem hiding this comment.
The cast is deliberately scoped to the selector shape the test asserts (color / backgroundColor / &:hover); the test only ever indexes selector keys, never fontWeight / lineHeight. Broadening to Record<string, unknown> would add a cast at every access without catching a real bug, so I've kept the narrower shape. Happy to switch if you'd prefer the stricter typing.
Review follow-up: - Derive the label colour with theme.palette.getContrastText per background so a light background (notably the yellow warning) keeps a readable dark label instead of an unreadable hardcoded white one (WCAG contrast). - Add &.MuiButton-containedInfo so color="info" contained buttons also honour the semantic colour instead of the brand green. - Extend the theme test to cover the info background and the contrast-derived label colour. Signed-off-by: Lee Calcote <lee.calcote@layer5.io>
|
Thanks for the thorough review - all addressed in
Full suite: 307 tests pass. |
Replace em-dashes / ellipses with plain ASCII in the comments touched by the semantic-colour change (no non-ASCII punctuation in code). Signed-off-by: Lee Calcote <lee.calcote@layer5.io>
Symptom
<Button variant="contained" color="error">(andsuccess/warning) renders the brand (keppel) green, not the semantic colour - so every destructive action in a consuming app is silently miscoloured, and callers work around it with per-sitesxspecificity hacks.Root cause
MuiButton.styleOverrides.rootsets&.MuiButton-contained { backgroundColor: palette.background.brand.default }for every contained button and defines nocontainedError/Success/Warning. MUI's default semantic-contained styling is therefore overridden andcoloris ignored for the background; a consumer's flatsx.backgroundColor(specificity 0,0,1,0) also loses to the theme selector (0,0,2,0).Fix
Add
&.MuiButton-containedError/containedSuccess/containedWarningrules using the matchingpalette.background.{error,success,warning}tokens, layered after the brand&.MuiButton-containedrule so they win on source order at equal (0,0,2,0) specificity. The higher-specificity.Mui-disabledrules still win when disabled.primary/default andsecondaryare unchanged, so only the currently-broken semantic-contained buttons change.Blast radius / review ask
Low: only
color="error|success|warning"contained buttons change (they were already visually wrong). Please eyeball any surface that usedvariant="contained" color="success|warning"expecting the brand look.outlinedvariants are untouched here.Test
src/__testing__/theme.test.tsasserts the resolvedcontainedError/Success/Warningbackgrounds are the semantic tokens (light + dark) and not the brand background. Full suite: 306 tests pass.Downstream
Unblocks removing a per-site
sxspecificity workaround inlayer5io/meshery-cloud(Edit-Org destructive buttons) once released and consumed.