Skip to content

gpui: Make refining a Style properly refine the TextStyle#42852

Merged
as-cii merged 8 commits intozed-industries:mainfrom
Serophots:text-style-refine
Dec 15, 2025
Merged

gpui: Make refining a Style properly refine the TextStyle#42852
as-cii merged 8 commits intozed-industries:mainfrom
Serophots:text-style-refine

Conversation

@Serophots
Copy link
Contributor

Motivating problem

The gpui API currently has this counter intuitive behaviour

 div()
            .id("hallo")
            .cursor_pointer()
            .text_color(white())
            .font_weight(FontWeight::SEMIBOLD)
            .text_size(px(20.0))
            .child("hallo")
            .active(|this| this.text_color(red()))

By changing the text_color when the div is active, the current behaviour is to overwrite all of the text styling rather than do a proper refinement of the existing text styling leading to this odd result:
The button being active inadvertently changes the font size.

Screen.Recording.2025-11-16.at.10.51.32.pm.mov

Solution

Previously refining a Style would not recursively refine the TextStyle inside of it, leading to this behaviour:

let mut style = Style::default();
style.refine(&StyleRefinement::default().text_size(px(20.0)));
style.refine(&StyleRefinement::default().font_weight(FontWeight::SEMIBOLD));

assert!(style.text_style().unwrap().font_size.is_none());
//assertion passes

(As best as I can tell) Style deliberately has pub text: TextStyleRefinement storing the TextStyleRefinement rather than the absolute TextStyle so that these refinements can be elsewhere used in cascading text styles down to element's children. But a consequence of that is that the refine macro was not properly recursively refining the text field as it ought to.

I've modified the refine macro so that the #[refineable] attribute works with TextStyleRefinement as well as the usual TextStyle. (Perhaps a little bit haphazardly by simply checking whether the name ends in Refinement - there may be a better solution there).

This PR resolves the motivating problem and triggers the assertion in the above code as you'd expect. I've compiled zed under these changes and all seems to be in order there.

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Nov 17, 2025
@mikayla-maki
Copy link
Member

Please add a small GPUI test showing that this fixes the problem, perhaps in styled.rs? Or style.rs?

@maxdeviant maxdeviant changed the title GPUI: Refining a Style proprely refines the TextStyle gpui: Make refining a Style properly refine the TextStyle Nov 17, 2025
@franciskafyi franciskafyi assigned as-cii and unassigned mikayla-maki Dec 14, 2025
The push_text_style call was made unconditional by this PR, but the
corresponding pop_text_style remained conditional on text.is_some().
This caused the text style stack to become unbalanced when code_block.text
had all None fields.
@as-cii as-cii enabled auto-merge (squash) December 15, 2025 10:37
@as-cii
Copy link
Member

as-cii commented Dec 15, 2025

Hey @Serophots, thanks for this pull request! I pushed a small fix and queued this up for merging! :shipit:

The font_features method was added after the PR branch diverged and
used the old Option-based API. Updated to match the new direct
TextStyleRefinement return type.
@as-cii as-cii merged commit a3ac595 into zed-industries:main Dec 15, 2025
23 checks passed
@github-project-automation github-project-automation bot moved this from Community PRs to Done in Quality Week – December 2025 Dec 15, 2025
CherryWorm pushed a commit to CherryWorm/zed that referenced this pull request Dec 16, 2025
…dustries#42852)

## Motivating problem
The gpui API currently has this counter intuitive behaviour

```rust
 div()
            .id("hallo")
            .cursor_pointer()
            .text_color(white())
            .font_weight(FontWeight::SEMIBOLD)
            .text_size(px(20.0))
            .child("hallo")
            .active(|this| this.text_color(red()))
```
By changing the text_color when the div is active, the current behaviour
is to overwrite all of the text styling rather than do a proper
refinement of the existing text styling leading to this odd result:
The button being active inadvertently changes the font size.


https://github.com/user-attachments/assets/1ff51169-0d76-4ee5-bbb0-004eb9ffdf2c



## Solution
Previously refining a Style would not recursively refine the TextStyle
inside of it, leading to this behaviour:
```rust
let mut style = Style::default();
style.refine(&StyleRefinement::default().text_size(px(20.0)));
style.refine(&StyleRefinement::default().font_weight(FontWeight::SEMIBOLD));

assert!(style.text_style().unwrap().font_size.is_none());
//assertion passes
```

(As best as I can tell) Style deliberately has `pub text:
TextStyleRefinement` storing the `TextStyleRefinement` rather than the
absolute `TextStyle` so that these refinements can be elsewhere used in
cascading text styles down to element's children. But a consequence of
that is that the refine macro was not properly recursively refining the
`text` field as it ought to.

I've modified the refine macro so that the `#[refineable]` attribute
works with `TextStyleRefinement` as well as the usual `TextStyle`.
(Perhaps a little bit haphazardly by simply checking whether the name
ends in Refinement - there may be a better solution there).

This PR resolves the motivating problem and triggers the assertion in
the above code as you'd expect. I've compiled zed under these changes
and all seems to be in order there.

Release Notes:

- N/A

---------

Co-authored-by: Antonio Scandurra <me@as-cii.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The user has signed the Contributor License Agreement

Projects

Development

Successfully merging this pull request may close these issues.

3 participants