Repository navigation
feat(theme): add semantic container color prop to Surface - #5121
hristototov wants to merge 4 commits into
Conversation
…into hristototov/surface-semantic-container-color
|
Review by Claude iOSBanner text is unreadable with the custom theme in the Banner example. (via Claude, verify if it makes sense)
What happens: the text turns near white ( What should happen: the banner shows the dark tertiary background with light text, like on Surface example: role labels break mid-word and two tiles are invisible. (via Claude, verify if it makes sense)
What happens: What should happen: each label fits on its own line and each tile is visible against the screen background, for example with a border or AndroidBanner text is unreadable with the custom theme in the Banner example. (via Claude, verify if it makes sense) Same steps and cause as on iOS. With the dynamic theme, Banner and Menu change color compared to
What happens: the background now uses the wallpaper based What should happen: the migration guide says that the color changes when you use the dynamic themes on Android. WebBanner text is unreadable with the custom theme in the Banner example. (via Claude, verify if it makes sense) Same steps and cause as on iOS. (via Claude, verify if it makes sense) |
| {...rest} | ||
| style={[surfaceStyle, style]} | ||
| theme={theme} | ||
| container="surfaceContainerLow" |
There was a problem hiding this comment.
This breaks the "Set custom theme" action in the Banner example on iOS, Android and web. example/src/Examples/BannerExample.tsx sets the custom background through colors.elevation.level1 and the text through onSurface: Palette.tertiary100. Banner no longer reads elevation.level1, so the near white text now sits on the light surfaceContainerLow background.
Update the example to override the role Banner now reads:
const customTheme = {
...defaultTheme,
colors: {
...defaultTheme.colors,
onSurface: Palette.tertiary100,
surfaceContainerLow: Palette.tertiary50,
primary: Palette.tertiary10,
},
};Any app that customized elevation.level1 to style Banner (or elevation.level2 for Menu) hits the same problem, so the migration guide should also say which role to override. (via Claude, verify if it makes sense)
| * variant-driven fills outside the surface family should keep resolving | ||
| * their own tokens and pass the result via `backgroundColor`. | ||
| */ | ||
| container?: SurfaceRole; |
There was a problem hiding this comment.
The existing backgroundColor prop already does this. backgroundColor={theme.colors.surfaceContainer} resolves from the theme, follows theme changes, and makes elevation control only the shadow, because it also wins over the elevation color. Searchbar (backgroundColor={theme.colors.surfaceContainerHigh}) and Dialog (surfaceContainerHigh) already use that pattern. With this PR, the library has two ways to set a surface role, and the new JSDoc tells contributors to prefer container, which those two components do not follow.
The name is also unclear. container="surface" or container="inverseSurface" does not name a container, and "container" in this library usually means a wrapper element (contentContainerStyle, styles.container).
Suggested fix: drop container and SurfaceRole, and pass backgroundColor={theme.colors.surfaceContainer} in Menu and backgroundColor={theme.colors.surfaceContainerLow} in Banner. The behavior stays the same and the public API does not grow. If you keep the prop, migrate Searchbar and Dialog to it so there is one pattern. (via Claude, verify if it makes sense)
There was a problem hiding this comment.
Kept container so components can name an MD3 role instead of resolving a raw color, and moved Searchbar to it. I left Dialog as is, because it sets its color through Modal's public contentBackgroundColor and moving it would mean adding a new Modal prop.
| * | ||
| * Prefer `container` when the fill is an MD3 surface-family role (as | ||
| * component specs define it, e.g. menus use `surfaceContainer`). Use | ||
| * `backgroundColor` only for raw, non-semantic colors. Components with | ||
| * variant-driven fills outside the surface family should keep resolving | ||
| * their own tokens and pass the result via `backgroundColor`. |
There was a problem hiding this comment.
If the prop stays, this paragraph is guidance for library contributors, not for users of Surface. It shows up in the public API docs. Consider removing it.
| * | |
| * Prefer `container` when the fill is an MD3 surface-family role (as | |
| * component specs define it, e.g. menus use `surfaceContainer`). Use | |
| * `backgroundColor` only for raw, non-semantic colors. Components with | |
| * variant-driven fills outside the surface family should keep resolving | |
| * their own tokens and pass the result via `backgroundColor`. |
(via Claude, verify if it makes sense)
| const backgroundColor = | ||
| customBackgroundColor ?? colors.elevation?.[`level${elevation}`]; | ||
| customBackgroundColor ?? | ||
| (container != null ? colors[container] : undefined) ?? | ||
| colors.elevation?.[`level${elevation}`]; |
There was a problem hiding this comment.
container is either a role or undefined, and colors[container] is always a color, so the : undefined) ?? step is not needed. A plain ternary reads easier:
| const backgroundColor = | |
| customBackgroundColor ?? colors.elevation?.[`level${elevation}`]; | |
| customBackgroundColor ?? | |
| (container != null ? colors[container] : undefined) ?? | |
| colors.elevation?.[`level${elevation}`]; | |
| const backgroundColor = | |
| customBackgroundColor ?? | |
| (container ? colors[container] : colors.elevation?.[`level${elevation}`]); |
(via Claude, verify if it makes sense)
|
|
||
| ### Banner | ||
|
|
||
| The banner background is now always `theme.colors.surfaceContainerLow` and no longer depends on the `elevation` prop, which only controls the shadow. At the default `elevation` of `1` the rendered color is unchanged. |
There was a problem hiding this comment.
"The rendered color is unchanged" is true for LightTheme and DarkTheme only. DynamicLightTheme and DynamicDarkTheme on Android 12 and newer replace surfaceContainer* with PlatformColor values but keep the static elevation colors. So Banner and Menu change from the static tint to the wallpaper based color there. The example app uses the dynamic theme by default on Android:
| main | this PR |
|---|---|
![]() |
![]() |
The guide should also tell users which role to override now, since a custom elevation.level1 no longer applies:
| The banner background is now always `theme.colors.surfaceContainerLow` and no longer depends on the `elevation` prop, which only controls the shadow. At the default `elevation` of `1` the rendered color is unchanged. | |
| The banner background is now always `theme.colors.surfaceContainerLow` and no longer depends on the `elevation` prop, which only controls the shadow. If you customized `theme.colors.elevation.level1` to change the banner background, customize `theme.colors.surfaceContainerLow` instead. With `LightTheme` and `DarkTheme`, the rendered color at the default `elevation` of `1` is unchanged. With `DynamicLightTheme` and `DynamicDarkTheme` on Android 12 and newer, the banner now uses the system surface color. |
(via Claude, verify if it makes sense)
|
|
||
| ### Menu | ||
|
|
||
| The menu background is now always `theme.colors.surfaceContainer`, following the Material Design 3 spec, and no longer depends on the `elevation` prop, which only controls the shadow. At the default `elevation` of `2` the rendered color is unchanged. |
There was a problem hiding this comment.
Same as for Banner above:
| The menu background is now always `theme.colors.surfaceContainer`, following the Material Design 3 spec, and no longer depends on the `elevation` prop, which only controls the shadow. At the default `elevation` of `2` the rendered color is unchanged. | |
| The menu background is now always `theme.colors.surfaceContainer`, following the Material Design 3 spec, and no longer depends on the `elevation` prop, which only controls the shadow. If you customized `theme.colors.elevation.level2` to change the menu background, customize `theme.colors.surfaceContainer` instead. With `LightTheme` and `DarkTheme`, the rendered color at the default `elevation` of `2` is unchanged. With `DynamicLightTheme` and `DynamicDarkTheme` on Android 12 and newer, the menu now uses the system surface color. |
(via Claude, verify if it makes sense)
| - The `pointerEvents` prop is no longer supported as it's deprecated in React Native Web. You can specify `pointerEvents` in the `style` prop instead. | ||
| - The `overflow: 'hidden'` style is no longer supported in `style` as it can clip shadows. You can nest a `View` inside the `Surface` and apply `overflow: 'hidden'` to that instead. | ||
| - The default `testID` for `Surface` was removed. You can specify a `testID` explicitly if you need it. | ||
| - A new `container` prop sets the background to a semantic surface-family color role from the theme (e.g. `container="surfaceContainerLow"`). When `container` is set, `elevation` only controls the shadow. Precedence: `backgroundColor` > `container` > the color derived from `elevation`. |
There was a problem hiding this comment.
This bullet sits between the style bullet and the "e.g.:" diff that shows how to migrate style, so the example now reads as if it belongs to container. It also describes a new feature, not a step users need to take. The Surface API docs already cover it, so I would remove the bullet:
| - A new `container` prop sets the background to a semantic surface-family color role from the theme (e.g. `container="surfaceContainerLow"`). When `container` is set, `elevation` only controls the shadow. Precedence: `backgroundColor` > `container` > the color derived from `elevation`. |
(via Claude, verify if it makes sense)
| <Surface | ||
| key={role} | ||
| style={styles.surface} | ||
| borderRadius={8} | ||
| mode="flat" | ||
| container={role} | ||
| > |
There was a problem hiding this comment.
Two problems with this row on iOS and Android:
- The
surfaceandsurfaceBrighttiles have the same color as the screen, so they look like plain text with no tile. - In the 120 pt tiles,
surfaceContainerLowestandsurfaceContainerLowwrap mid-word ("surfaceContainerLo / west").
An elevated tile makes every role visible, and it also shows that elevation only sets the shadow here. For the wrapping, use wider tiles for this row or a smaller text variant.
| <Surface | |
| key={role} | |
| style={styles.surface} | |
| borderRadius={8} | |
| mode="flat" | |
| container={role} | |
| > | |
| <Surface | |
| key={role} | |
| style={styles.surface} | |
| borderRadius={8} | |
| elevation={1} | |
| container={role} | |
| > |
(via Claude, verify if it makes sense)
There was a problem hiding this comment.
The elevated tile does not fix this in dark theme, because surface and surfaceDim are both neutral6 there and the shadow does not show on the dark screen (screenshots). A hairline outlineVariant border shows every tile in light and dark theme:
| <Surface | |
| key={role} | |
| style={styles.surface} | |
| borderRadius={8} | |
| mode="flat" | |
| container={role} | |
| > | |
| <Surface | |
| key={role} | |
| style={[ | |
| styles.surface, | |
| { | |
| borderWidth: StyleSheet.hairlineWidth, | |
| borderColor: theme.colors.outlineVariant, | |
| }, | |
| ]} | |
| borderRadius={8} | |
| mode="flat" | |
| container={role} | |
| > |
(via Claude, verify if it makes sense)
|
Review by Claude iOSSurface example, dark theme: the
What happens: in What should happen: every tile is visible in light and dark theme. A hairline AndroidSurface example, dark theme: the Same steps and cause as on iOS. WebSurface example, dark theme: the Same steps and cause as on iOS. (via Claude, verify if it makes sense) |
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
docs/6.x/docs/guides/theming.mdx(line 109) still names menus as an example of surfaces tinted bytheme.colors.elevation. With this PR, Menu and Banner take their background fromsurfaceContainerandsurfaceContainerLow, so a reader who changeselevation.level2to restyle menus sees no effect. Drop "or menus" there, or add that Menu and Banner use thesurfaceContainer*roles. (via Claude, verify if it makes sense)
| * Semantic color role of the Surface background, resolved from the | ||
| * theme, e.g. `container="surfaceContainerLow"` renders with | ||
| * `theme.colors.surfaceContainerLow`. |
There was a problem hiding this comment.
The generated docs page shows the type of this prop as SurfaceRole only, so readers cannot see which values it accepts. The values are only listed in src/theme/types/color.ts, which the docs do not show. List them here:
| * Semantic color role of the Surface background, resolved from the | |
| * theme, e.g. `container="surfaceContainerLow"` renders with | |
| * `theme.colors.surfaceContainerLow`. | |
| * Semantic color role of the Surface background, resolved from the | |
| * theme, e.g. `container="surfaceContainerLow"` renders with | |
| * `theme.colors.surfaceContainerLow`. Accepted roles: `surface`, | |
| * `surfaceDim`, `surfaceBright`, `surfaceVariant`, | |
| * `surfaceContainerLowest`, `surfaceContainerLow`, `surfaceContainer`, | |
| * `surfaceContainerHigh`, `surfaceContainerHighest` and `inverseSurface`. |
(via Claude, verify if it makes sense)
…into hristototov/surface-semantic-container-color
Updated |

















Motivation
Surfacetakes its background fromelevation, so a component can't use an MD3 surface role without passing a raw color. The newcontainerprop takes a surface role and resolves it from the theme.elevationthen only sets the shadow.Menu, Banner and Searchbar now use it. With
LightThemeandDarkThemethey look the same. With the dynamic themes on Android 12+, Menu and Banner use the system surface colors.Related issue
Fixes #5119
Screenshots / Videos
light-mode.mov
dark-mode.mov
Test plan