Full contextual theming#24969
Conversation
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
|
I ended up completely re-writing the previous version ("limited contextual theming") but re-used the same PR instead of creating a new one (partially by accident). In any case, I think this is ready for review. @gagnus I'd be interested in your comments; as this moves somewhat nearer to your "slots" idea. I have not attempted to implement an algorithmic theme generator (I'll leave that to you or someone else), I'm just trying to put in the infrastructure for contextual theming and semantic tokens. I have continued to use alpha colors in a few places - this is primarily for ease of migration on my part. There's no longer an "alpha hack" for sliders and text input backgrounds. Alpha is mainly used for disabled controls, because this is the specification given on the figma doc. However, with some effort, the number of alpha colors could be reduced even further if desired (although there are a few things, like focus rects, which depend on alpha). |
kfc35
left a comment
There was a problem hiding this comment.
I didn’t review the code in dark_theme too deeply because I figured that could just be thoroughly scrutinized by running feathers_gallery. I assume the screenshot was of what’s in the PR so I ran feathers_gallery on main to compare. I didn’t notice anything drastically worse when comparing the two versions.
I appreciate that this will make life a little easier when defining colors for all the different states that controls can be in, so I approve of the spirit of this PR. I can drop an approval after my comments are addressed.
(Caveat: I haven’t really viewed any other theme-ing relevant PR’s or been keeping up with the theme related chatter in ui-dev)
| /// Map of design tokens to colors. | ||
| pub color: HashMap<ThemeToken, Color>, | ||
| /// Map of design tokens to semantic tokens. | ||
| pub color: HashMap<ThemeToken, SemanticToken>, |
There was a problem hiding this comment.
probably should be renamed, something like token_map? theme_to_semantic
There was a problem hiding this comment.
How about token_assignments?
| // Return a bright obnoxious color to make the error obvious. | ||
| palettes::basic::FUCHSIA.into() | ||
| } | ||
| let Some(semantic_token) = self.0.color.get(token) else { |
There was a problem hiding this comment.
Could this function just defer to self.context_color(token, SurfaceLevel::Base) now?
There was a problem hiding this comment.
The only reason I'm even keeping color around is for backwards compatibility.
| /// Fill when pressed | ||
| pub const FILL_ITEM_PRESSED: SemanticToken = SemanticToken::new_static("fill.item.pressed"); | ||
|
|
||
| /// Field fill - text inputs |
There was a problem hiding this comment.
text inputs cant be disabled? or is it a similar note that they are normally transparent
There was a problem hiding this comment.
They can be disabled, but this affects the text color, not the background. The reason is that the background is already so dim that making it dimmer wouldn't do much.
There was a problem hiding this comment.
This reminds me that text input disabling is currently a bit broken from my theming testing (visual change is fine but it still puts the cursor in when you click on it for example)
There was a problem hiding this comment.
I can look into that in my other PR (the TextInput one).
| for (mut bg, theme_bg) in q_background.iter_mut() { | ||
| bg.0 = theme.color(&theme_bg.0); | ||
| for (ent, mut bg, ThemeBackgroundColor(token)) in q_background.iter_mut() { | ||
| let context = q_context |
There was a problem hiding this comment.
any reason why the queries of q_background border and text_color don’t also just query for an Option<&ThemeContext>? Saves you an extra query
There was a problem hiding this comment.
Wouldn't that cause an ECS query conflict? I thought we weren't supposed to have multiple queries accessing the same component.
There was a problem hiding this comment.
You can access the same data immutably across queries :)
| && let Ok(is_disabled) = q_number_input.get(root_id) | ||
| && let Ok((is_disabled, theme_context)) = q_number_input.get(root_id) | ||
| { | ||
| let Some(drag_state) = q_children |
There was a problem hiding this comment.
i’m assuming this is just fixing a side thing unrelated to the theme changes
you could just merge this with the && let’s prior, right? and avoid the deeper nested else return
There was a problem hiding this comment.
It's somewhat related. Slider had a fix which never got ported over to NumberInput: when dragging, you should always display the PRESSED color state even if you aren't hovering, otherwise it looks distracting when the mouse moves off the slider in mid-drag and the color changes.
Why do it now? Because part of the success criteria for this PR is to have all the color assignments look good in every state.
Note that this would not be an issue if we had pointer capture #24396
There was a problem hiding this comment.
that PR needs a second review :P
| /// Map of design tokens to semantic tokens. | ||
| pub color: HashMap<ThemeToken, SemanticToken>, | ||
| /// Map of semantic tokens to colors. | ||
| pub semantic_base: HashMap<SemanticToken, Color>, |
There was a problem hiding this comment.
Is there a reason to separate out the base map from semantic_overrides? Could this be simplified by having
pub semantic_color: HashMap<SurfaceLevel, HashMap<SemanticToken, Color>>,
and just including an entry for SurfaceLevel::Base in this map which would be equivalent to what semantic_base would be?
There was a problem hiding this comment.
One downside is that this increases the indentation level for all the dark_theme color assignments by 8 spaces, which admittedly is a petty reason, but I like it when the lines don't wrap :)
There was a problem hiding this comment.
the logic as is is easily understandable and will work fine with base separated, so this isn’t a blocking comment
| pub const FILL_ACCENT_DISABLED: SemanticToken = | ||
| SemanticToken::new_static("fill.accent.disabled"); | ||
|
|
||
| /// Default fill - buttons |
There was a problem hiding this comment.
Is the use of the word Default here being used as if this is the fill you would see in the “default case”, or is it describing that this is the default for solid fill?
bikeshedding comment: maybe plain instead of solid? But the use of the phrase “plain buttons” might not jive with that. regular?
There was a problem hiding this comment.
It's meant to be the fill color for the widget's normal / natural state (no hover, pressed, or disabled condition). One option would be to just remove the suffix, FILL_SOLID, which I guess would be consistent with the theme tokens. NORMAL is another option I suppose.
Here's some substance to inform the bikeshed. As far as the prominence levels go, we have:
- accent / primary / call-to-action - this is where your eye should go first
- solid / secondary - buttons and widgets that are not as bright, but in no way "subtle" - this is the "cancel" button on the dialog box (as opposed to the "ok" button which is primary).
- item - list rows and menu items; these are basically widgets with a transparent background (except when hovering or selected)
- field - fill colors that are just barely brighter than the background.
We already have "plain" buttons, which are treated like "item" - that is, there's no background fill in the normal state.
There was a problem hiding this comment.
I think I prefer the “primary”/“secondary" terminology because it better delineates an eye priority difference between the two. Maybe even calling field “tertiary” could tie it all together. I think “item” can stay as is since that term conveys a feeling that the ui thingy is minor… although “minor” could also be a good name. (or maybe item should be tertiary and field should be minor)
This is just my personal preference though, and I don’t think my approval is blocked on updating the name to be satisfactory to my taste
There was a problem hiding this comment.
Here's a counter argument: the existing usage of the word "primary" in feathers (and other toolkits) is for a particular variant of button: you have primary and secondary buttons, but not primary and secondary checkboxes or sliders. Yet, the "accent" color is used in checkboxes, sliders, and radio buttons. So I think what we want is a word that describes the visual appearance of the color, not its meaning.
viridia
left a comment
There was a problem hiding this comment.
Very good feedback, thanks!
| ThemeBackgroundColor(tokens::SUBPANE_HEADER_BG) | ||
| ThemeBorderColor(tokens::SUBPANE_HEADER_BORDER) | ||
| InheritableThemeTextColor(tokens::SUBPANE_HEADER_TEXT) | ||
| template(|_| Ok(Propagate(ThemeContext(SurfaceLevel::Highest)))) |
There was a problem hiding this comment.
It makes sense when you look at the figma mocks:
- pane has a dark header
- subpane has a light header
- group has the same shader for both header and footer - I'm not sure group even needs to have a header at all.
| && let Ok(is_disabled) = q_number_input.get(root_id) | ||
| && let Ok((is_disabled, theme_context)) = q_number_input.get(root_id) | ||
| { | ||
| let Some(drag_state) = q_children |
There was a problem hiding this comment.
It's somewhat related. Slider had a fix which never got ported over to NumberInput: when dragging, you should always display the PRESSED color state even if you aren't hovering, otherwise it looks distracting when the mouse moves off the slider in mid-drag and the color changes.
Why do it now? Because part of the success criteria for this PR is to have all the color assignments look good in every state.
Note that this would not be an issue if we had pointer capture #24396
| ]), | ||
| ), | ||
| ]), | ||
| color: HashMap::from([ |
There was a problem hiding this comment.
I think that's a reasonable idea, but maybe we can improve this in a subsequent PR.
I think the easiest solution is to split out the feathers theme props assignments as a separate function.
| if q_indicators.contains(entity) { | ||
| commands.entity(entity).insert(Outline { | ||
| color: theme.color(&tokens::FOCUS_RING), | ||
| color: theme.context_color(&tokens::FOCUS_RING, SurfaceLevel::Base), |
There was a problem hiding this comment.
I haven't decided whether I want to remove color entirely, but I'm making the decision to remove it easier by making sure it isn't called anywhere.
| // Return a bright obnoxious color to make the error obvious. | ||
| palettes::basic::FUCHSIA.into() | ||
| } | ||
| let Some(semantic_token) = self.0.color.get(token) else { |
| /// Map of design tokens to colors. | ||
| pub color: HashMap<ThemeToken, Color>, | ||
| /// Map of design tokens to semantic tokens. | ||
| pub color: HashMap<ThemeToken, SemanticToken>, |
There was a problem hiding this comment.
How about token_assignments?
| /// Map of design tokens to semantic tokens. | ||
| pub color: HashMap<ThemeToken, SemanticToken>, | ||
| /// Map of semantic tokens to colors. | ||
| pub semantic_base: HashMap<SemanticToken, Color>, |
There was a problem hiding this comment.
One downside is that this increases the indentation level for all the dark_theme color assignments by 8 spaces, which admittedly is a petty reason, but I like it when the lines don't wrap :)
| for (mut bg, theme_bg) in q_background.iter_mut() { | ||
| bg.0 = theme.color(&theme_bg.0); | ||
| for (ent, mut bg, ThemeBackgroundColor(token)) in q_background.iter_mut() { | ||
| let context = q_context |
There was a problem hiding this comment.
Wouldn't that cause an ECS query conflict? I thought we weren't supposed to have multiple queries accessing the same component.
| pub const FILL_ACCENT_DISABLED: SemanticToken = | ||
| SemanticToken::new_static("fill.accent.disabled"); | ||
|
|
||
| /// Default fill - buttons |
There was a problem hiding this comment.
It's meant to be the fill color for the widget's normal / natural state (no hover, pressed, or disabled condition). One option would be to just remove the suffix, FILL_SOLID, which I guess would be consistent with the theme tokens. NORMAL is another option I suppose.
Here's some substance to inform the bikeshed. As far as the prominence levels go, we have:
- accent / primary / call-to-action - this is where your eye should go first
- solid / secondary - buttons and widgets that are not as bright, but in no way "subtle" - this is the "cancel" button on the dialog box (as opposed to the "ok" button which is primary).
- item - list rows and menu items; these are basically widgets with a transparent background (except when hovering or selected)
- field - fill colors that are just barely brighter than the background.
We already have "plain" buttons, which are treated like "item" - that is, there's no background fill in the normal state.
| /// Fill when pressed | ||
| pub const FILL_ITEM_PRESSED: SemanticToken = SemanticToken::new_static("fill.item.pressed"); | ||
|
|
||
| /// Field fill - text inputs |
There was a problem hiding this comment.
They can be disabled, but this affects the text color, not the background. The reason is that the background is already so dim that making it dimmer wouldn't do much.
gagnus
left a comment
There was a problem hiding this comment.
Mostly fine with this. Personally I prefer named values (radix) to these semi-semantic tokens. For example here you need to know that certain colors clash with "highest" grouping and nothing about the naming really helps with it, where I think it's more obvious if you know the background is "bg3" (or something) and you're displaying a control whose background is also "bg3".
| tokens::SLIDER_BG | ||
| }); | ||
| let bar_color = theme.context_color( | ||
| &if disabled { |
There was a problem hiding this comment.
We do this dance a lot across controls, can be simplified.
Yeah, I'm not 100% happy with this either, but I struggled to come up with anything better. I'm not even sure if the name That being said, this at least accomplishes my main goals:
|
|
The merging of #24974 with its custom ThemeProps may cause this PR to have an error on merge If you don’t want to deal with cleaning that up, you can just do the migration guide thing I mentioned with regards to just creating |
Actually, I think the simplest thing would be to add an additional constructor for ThemeProps which does this automatically. |
|
Current state of affairs:
|


Third time's the charm, maybe? This now implements full-on contextual theming.
Token lookup now happens in two stages:
A theme token is converted into a semantic token, and then the combination of semantic token + theme context is used to look up a color. The set of semantic tokens is much smaller than the set of theme tokens.
The theme context is an enum which indicates the surface layer of the container holding the widget.
This is largely inspired by the design token architecture used by web toolkits such as Radix, Chakra or MUI. See the release notes for additional explanation.
@alice-i-cecile @gagnus