Skip to content

UI leaf node styling and scrollbars #19344

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

andrewzhurov
Copy link

@andrewzhurov andrewzhurov commented May 23, 2025

Objective

Enable layout and rendering of padding and border on UI leaf nodes.
As presently they're being ignored.

Fixes #6879
Fixes #11557
Fixes #17300 (duplicate of the above)

Scale-to-fit leaf node images.
As padding and border on these nodes "eat" available for image space.

Adds layout and render of scrollbars.

Solution

  1. Provide Taffy with border and scrollbar sizes. (previously, border is set to Zero for leaf nodes, scrollbar_size always 0)
  2. Measure text and images with regard to available_width. (needs an eye on it)
    measure_fns make use of Taffy's available_size, which comes with borders, scrollbars and padding stripped out, so available_size for pure content.
    known_sizes are disregarded as they include those (so it's a size of border box).
    Previously they've been fine to use, as leaf nodes meant to have border_box = pure content size.
    Also, images follow CSS's object-fit: contain; and that's not a default behavior in CSS. Default is to stretch. So perhaps that's something to change?
  3. Adjust clip calculation for node's content to account for scrollbar sizes.
    Cache it as ComputedContentClip (as content's clip, and not inherited from parent clip is now necessary for rendering UI leaf nodes)
  4. Add scrollbar rendering (of track and thumb). (eyes wanted on whether that's efficient)
    Scrollbar sizes are consts. (as CSS does not allow for Px val customization of scrollbar sizes, only coarse "auto" "thin" via scrollbar-width, so Px vals are not exposed to be configurable e.g., via a component)
    ScrollbarColor component, required from Node can be used to specify track and thumb colors for node's scrollbars.
  5. Switch from Node requiring ScrollPosition to manually enabling it for nodes with overflow scroll on any axis.
  6. Preliminary work on enabling scroll for a reverse-ordered flexbox, where scroll position is meant to be in negative values - clamp scroll with this in mind. Yet present version of Taffy reports content_size that = layout_size for reverse-ordered flexboxes, so that does not fixes ScrollPositions does not work on Node with flex_direction set to RowReverse #17129 atm.

For rendering of a scale-to-fit image I've explored two approaches:

  1. Modify it's Affine transform with scale, at extraction step. Worked. Seems simple.
    But then I discovered the caveat, when clip cuts some part of an image (e.g., only top part of image is seen when scrolling), GLSL's UVs are not modified, so whole image texture is rendered on top-half rect, resulting in a "squashed" look.
  2. Modify image's UV's to account for clipping. This way we have e.g., vertexes and UVs for top-half of the image.
    That's the way it's done in PR. Images get clipped correctly (incl. flipped ones, as can be seen in borders example)
    Eyes wanted on performance of the approach.

Testing

I performed visual testing on borders scroll and game_menu examples.
Ubuntu 25.04, X11


Showcase

Before / After

Screencast.From.2025-05-23.13-06-47.mp4
Screencast.From.2025-05-23.13-13-02.mp4

Can be run with cargo run --example borders on commits:
24cc495 (Extend borders example with text and images)
e976b54 (UI: add support for padding...)

Content now is rather lengthy, so it's put in a sclobbale container.

typo
As presently the meant-to-be-scrollable list displays in full on a
not-that-large screen.

tweak scroll
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@andrewzhurov andrewzhurov force-pushed the ui-leaf-node-styling-and-scrollbars branch from a6a3dd4 to e976b54 Compare May 23, 2025 10:22
Copy link
Contributor

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-19344

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

Copy link
Contributor

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-19344

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

Tuffy provides us content size that is stripped from border
scroll padding sizes - available_width available_height.
We need to figure out whether it's a hard constraint or not.
If size styles are Auto - image need not scale to fit if it's less than
available_size. So rendered size is min of image size and available size.
If size is explicitly set via styles, then we scale image accordingly.
This is similar to CSS's object-fit: contain;
Which may not be expected as default, as I think of it.
Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

I haven't had a look at this properly yet and it might be better than I think.

IMO I'm probably against this though, I think it's much simpler to just disallow styling on leaf nodes containing content completely. If users want text or an image with borders and padding they can just wrap them in an intermediate parent Node.

Maybe if we could have some sort of common interface for content that handles recalculating the coordinates that's invisable to the update, clipping, interaction and extraction logics for each content type it might be okay, but that's still a lot of effort and extra complexity that seems unnecessary.

Comment on lines -69 to +94
if let Some(mut calculated_clip) = maybe_calculated_clip {
if let Some(inherited_clip) = maybe_inherited_clip {
// Replace the previous calculated clip with the inherited clipping rect
match (maybe_inherited_clip, maybe_calculated_clip) {
(Some(inherited_clip), None) => {
commands.entity(entity).try_insert(CalculatedClip {
clip: inherited_clip,
});
}
(Some(inherited_clip), Some(mut calculated_clip)) => {
if calculated_clip.clip != inherited_clip {
*calculated_clip = CalculatedClip {
clip: inherited_clip,
};
}
} else {
// No inherited clipping rect, remove the component
}
(None, Some(_)) => {
commands.entity(entity).remove::<CalculatedClip>();
}
} else if let Some(inherited_clip) = maybe_inherited_clip {
// No previous calculated clip, add a new CalculatedClip component with the inherited clipping rect
commands.entity(entity).try_insert(CalculatedClip {
clip: inherited_clip,
});
}
_ => {}
};
Copy link
Author

Choose a reason for hiding this comment

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

Just switches from nested ifs to a match

@andrewzhurov
Copy link
Author

andrewzhurov commented May 24, 2025

IMO I'm probably against this though, I think it's much simpler to just disallow styling on leaf nodes containing content completely. If users want text or an image with borders and padding they can just wrap them in an intermediate parent Node.

Yeah, that approach seems appealing.
May be confusing for those coming from HTML CSS world to not be able to slam padding border on images.
But is simpler. Both internally (no need to account for styles on every kind of content node on render) and externally, I kinda like the decoupling of box and content from the data point of view. May be a bit cumbersome to type though..

(no need to account for styles on every kind of content node on render)

Although that boils down to "add content_inset to content's tf".
And pass to prepare ComputedContentClip instead of ComputedClip (which is of parent).
In hindsight, I thought to +1 on having no styles on leaf nodes to save on complexity, but as I gave another look on the cost^.. have mixed feelings now. :) Curious about your opinion.

I modified the borders example to see how it'll look with children![content] (branch)
(without any style tweaks of this PR)

Screencast.From.2025-05-24.14-13-30.mp4

What seems broken:

  • scroll overscrolls
  • images seem to be measured and rendered improperly

But that would be relatively easy to fix, I guess.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets M-Needs-Release-Note Work that should be called out in the blog due to impact X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 26, 2025
@alice-i-cecile alice-i-cecile moved this to Widget-ready in Alice's Work Planning May 26, 2025
Copy link
Contributor

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.

@JMS55
Copy link
Contributor

JMS55 commented May 26, 2025

I don't have the knowledge to review this PR, but I'm excited that we can finally add layout styling to text nodes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
Status: Widget-ready
4 participants