Skip to content
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

ScrollAreas can overrun their container's margins #3385

Open
valadaptive opened this issue Sep 25, 2023 · 6 comments
Open

ScrollAreas can overrun their container's margins #3385

valadaptive opened this issue Sep 25, 2023 · 6 comments
Labels
bug Something is broken

Comments

@valadaptive
Copy link
Contributor

Describe the bug

When displaying a ScrollArea inside a container, and it only scrolls along one axis, it will be a few pixels too big and will overflow into that container's margins along that axis.

This is easiest to see when we set the container's margins to 0. Here's an example with a ScrollArea containing the grey rectangle in the central panel, surrounded by 4 panels:

Peek 2023-09-25 01-09

I haven't delved into the code much, but it seems like the scroll area overruns the container margin by clip_rect_margin pixels. Setting clip_rect_margin to 0.0 before rendering the scroll area fixes the issue.

To Reproduce
Steps to reproduce the behavior:

See the following demo code, which renders the UI shown above:

Demo code ```rust use eframe::egui;

fn main() -> Result<(), eframe::Error> {
let options = eframe::NativeOptions {
initial_window_size: Some(egui::vec2(480.0, 360.0)),
..Default::default()
};
eframe::run_native(
"My egui App",
options,
Box::new(|_cc| Box::::default()),
)
}

#[derive(Default)]
struct MyApp {}

impl eframe::App for MyApp {
fn update(&mut self, ctx: &egui::Context, frame: &mut eframe::Frame) {
let no_margins = egui::Frame::side_top_panel(&ctx.style()).inner_margin(0.0);
egui::TopBottomPanel::top("top").show(ctx, |
| {});
egui::TopBottomPanel::bottom("bottom").show(ctx, || {});
egui::SidePanel::left("left").show(ctx, |
| {});
egui::SidePanel::right("right").show(ctx, |_| {});
egui::CentralPanel::default()
.frame(no_margins)
.show(ctx, |ui| {
egui::ScrollArea::new([true; 2]).show(ui, |ui| {
ui.add(
egui::Image::from_texture((
egui::TextureId::default(),
egui::vec2(360.0, 240.0),
))
.uv(egui::Rect::from_min_max(
egui::pos2(0.0, 0.0),
egui::pos2(0.0, 0.0),
))
.tint(egui::Color32::GRAY),
);
});
});
}
}

</details>

**Expected behavior**
The ScrollArea should properly resize to fit within its container.

**Screenshots**
See GIF above.
@valadaptive valadaptive added the bug Something is broken label Sep 25, 2023
@valadaptive
Copy link
Contributor Author

I came up with a tentative fix that just doesn't expand the inner clip rect by clip_rect_margin but it has some side effects. For instance, the current codebase just so happens to almost cancel out the inner frame's margin (in the default style) with clip_rect_margin. This means that scroll areas' contents are mostly visible even around the margins. My fix would change that behavior to just clip the scroll area:

Current Proposed fix
image image

This matches how e.g. the CSS padding property works.

It seems like clip_rect_margin has snaked its way around the codebase, and its default value of 3.0 is close enough to many default-style spacing values to conceal many visual hiccups. I would try to remove it, but I have no idea what the intended visuals are.

@emilk
Copy link
Owner

emilk commented Sep 25, 2023

clip_rect_margin is an ugly hack, and I'd like to get rid of it.

The reason I added it is because a lot of widgets draw their border outline at their margin, and so if the border is e.g. 2 points wide, half of the outline is outside the widget rect. This means that if you put such a widget first thing inside e.g. a ScrollArea, you would see a clipped button, unless there was some extra margin somewhere.

I think there are a few ways to fix this mess:

  • Adjust the border logic of widgets and Frame to always paint within the lines instead
  • Add an margin to the contents of a ScrollArea, so that the first widget does not start at the top-left (0,0) coordinate, but e.g. at (3,3)

@emilk emilk added this to the 0.24.0 milestone Sep 25, 2023
@valadaptive
Copy link
Contributor Author

Painting the borders inside the lines seems like the best solution to me as well.

It seems like egui doesn't currently count borders as taking up any space. This would make them more similar to the CSS outline property than a CSS border. This is good because changing the borders to paint inside the lines won't have a bunch of subtle layout implications like I originally thought.

It seems like most of the current use cases for clip_rect_margin outside of ScrollArea are for elements like Area and Resize which clip their contents then immediately create a Frame as their only child element. I haven't looked into whether those two can have a size which isn't known until they render its contents, but:

  • If they have a size that isn't fully determined before adding their contents, then it seems like clipping doesn't really do much? You can clip the top and left sides, but the bottom and right could be significantly further out than expected. In this case, could we just stop clipping entirely?
  • If they have a size that is fully determined, then we may be able to separate the styling of Frame from the sizing. The elements that previously used Frame could just paint the Rect themselves since they know the size. This would allow them to precisely control clipping.

@valadaptive
Copy link
Contributor Author

Hm, it seems like a lot of widgets draw outside their rectangle. Resize, for instance, expands its bounds by a hardcoded 2 units to give the content some "breathing room". And basically every widget draws a little bit outside a Frame's inner_margin.

For example, the Window element uses one big frame for both its title bar and contents (which does not clip), then places a Resize inside that frame for its contents only (which does clip).

I tried removing clip_rect_margin from Resize, but this leads to window contents being clipped (see the slider being hovered):
image

On the other hand, if I just stop Resize from clipping entirely, then windows' contents can overflow (see the central panel on the left window):
image

It seems like the right place for clipping to happen is Frame, but as the commented-out Frame clipping code from 3 years ago says, it doesn't know how big its contents will be until after they're drawn.

Maybe instead of having one hardcoded clip_rect_margin, a Frame could propagate its inner margin downwards to its children, which could then expand their clip rectangles by that margin. Alternatively, as you suggested, container elements like ScrollArea and Resize which clip their contents could have their own inner margin. This would require rewriting a lot of Window's layout code, however.

@emilk
Copy link
Owner

emilk commented Sep 27, 2023

I may tackle this when I start working on #3284

@valadaptive
Copy link
Contributor Author

I'm thinking about this once again, and I think I've convinced myself that the real issue has been ScrollArea the entire time.

In the HTML/CSS world, you have one primitive: the box. It has a border, margin, and padding, and you can make it scrollable.

In egui, if you want a scrollable container, you have to put a ScrollArea inside an existing container (like a Panel). There is no way for that Panel to "tell" the ScrollArea what inner margin it has (and transfer it over)--what if that ScrollArea isn't the only child of the Panel? So instead, there's clip_rect_margin, which approximates that functionality if you don't look too closely.

There's also resizability--as far as I can tell, the Resize container has two disparate functionalities. It can be a widget that you can resize by dragging it with a mouse, or it can be a primitive for building your own resizable layouts by setting resizable to false and controlling it manually, as Window does. In fact, it seems like Window overrides a lot of Resize's behavior--what purpose does Resize serve there?

I think that it's better to conceptualize "scrollable" and "resizable" as attributes of a box/layout, rather than their own distinct widgets. Window already does this--it has the resizable and scroll options.

From there, you would need to rework the container widgets' APIs with this in mind. I think the best way would be to create a struct that contains both the Frame and scroll/resize state (something like Box). Containers like Panel can then expose the functionality that makes sense to support.

What are your thoughts on this API? I may try to implement it--I will probably fail, but I think it's worth exploring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken
Projects
None yet
Development

No branches or pull requests

2 participants