Skip to content

Comments

Replace set_clip_path with set_clips_contents method#1627

Open
PoignardAzur wants to merge 7 commits intolinebender:mainfrom
PoignardAzur:set_clips_content
Open

Replace set_clip_path with set_clips_contents method#1627
PoignardAzur wants to merge 7 commits intolinebender:mainfrom
PoignardAzur:set_clips_content

Conversation

@PoignardAzur
Copy link
Contributor

This changes clipping behavior to a boolean flag, which better matches how it was used so far.

A follow-up PR will add set_clip_shape to set arbitrary (even non-Rect) shapes.

// pub(crate) clip_shape: Option<BezPath>,
//
/// Whether the widget and its children's scenes are clipped by the
/// [clip shape](crate::doc::masonry_concepts#clip-shape).
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say "clip shape" yet?

Copy link
Contributor Author

@PoignardAzur PoignardAzur Jan 28, 2026

Choose a reason for hiding this comment

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

Yeah, the documentation link is already valid.

- If the widget is set to clip its contents, pointer events outside the clip shape won't affect the children either.
- If the widget is set to clip its contents, its scene and the children's scenes will be painted inside of the clip shape.

Currently, the clip shape is hardcoded to be the layout rect.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to say "the widgets local rect, transformed for rendering" or something since we just say that a layout rect doesn't make sense above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I think I need to start a discussion about this on Zulip.

PoignardAzur added a commit to PoignardAzur/xilem that referenced this pull request Jan 28, 2026
@PoignardAzur PoignardAzur mentioned this pull request Jan 28, 2026
This changes clipping behavior to a boolean flag, which better matches how it was used so far.

A follow-up PR will add set_clip_shape to set arbitrary (even non-Rect) shapes.
@PoignardAzur
Copy link
Contributor Author

PoignardAzur commented Feb 10, 2026

I've rebased on top of the box model rework.

I'm not super happy with the result of the rebase (you can kinda tell the PR was meant for the previous coordinate system), and we need some kind of discussion about how to integrate clip shapes into the box model. We can do that before or after merging this PR.

Note that one effect of this PR is that the Label and Prose widgets were previously clipped to their border box and they're now clipped to their content box, which matches the behavior of other widgets. I think that's fine in the short term, because these two labels don't have any padding/borders by default, so both behavior are usually identical.

Whether Label and Prose should clip to their border box is another discussion, one that will be tied to how clip shapes fit in the box model.

@PoignardAzur PoignardAzur requested a review from xStrom February 17, 2026 11:08
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

If you want to land this sooner than later, then I suggest making the bool mean border-box (matches the old behavior). I mention in an inline comment how to get that.

That will get the same old results for Prose and Label. It will (temporarily) regress TextInput but that would be ok.


More generally, I think we should provide easy clip support with more options. I think the three choices from CSS's overflow-clip-margin are all very useful.

  • content-box
  • padding-box
  • border-box

Essentially I can imagine having:

enum Clip {
    None,
    ContentBox,
    PaddingBox,
    BorderBox,
    Custom(Shape),
}

Whether you think it should happen already in this PR I'll leave for you to decide.

I am also willing to implement the various pre-defined box clips myself, given I'm very familiar with the box conversions right now.

Comment on lines +567 to +569
let is_inside_clip_shape = ctx.content_box().contains(local_pos);

if ctx.clips_contents() && !is_inside_clip_shape {
Copy link
Member

Choose a reason for hiding this comment

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

A lot of widgets won't have ctx.clips_contents() as true, right? So we could avoid doing the content box fetching and position checking.

Suggested change
let is_inside_clip_shape = ctx.content_box().contains(local_pos);
if ctx.clips_contents() && !is_inside_clip_shape {
if ctx.clips_contents() && !ctx.content_box().contains(local_pos) {

Comment on lines +439 to +444
/// only the painting done in `Widget::paint` by this widget itself will be clipped.
/// The remaining painting done in `Widget::pre_paint` and `Widget::post_paint` will not be clipped.
/// - Pointer events must be inside that shape to reach the widget's children.
///
/// This currently returns the border-box rect if `clips_contents` is true and `None` otherwise, but in the future we may want to support more complex clip shapes, in which case this method would need to be updated.
///
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep these paint methods short and clickable links. Shortness also helps keep the line length under a 100 and links help keep docs fresh.

Similarly, let's break the long sentence into multiple lines so it's below 100.

Suggested change
/// only the painting done in `Widget::paint` by this widget itself will be clipped.
/// The remaining painting done in `Widget::pre_paint` and `Widget::post_paint` will not be clipped.
/// - Pointer events must be inside that shape to reach the widget's children.
///
/// This currently returns the border-box rect if `clips_contents` is true and `None` otherwise, but in the future we may want to support more complex clip shapes, in which case this method would need to be updated.
///
/// only the painting done in [`paint`] by this widget itself will be clipped.
/// The remaining painting done in [`pre_paint`] and [`post_paint`] will not be clipped.
/// - Pointer events must be inside that shape to reach the widget's children.
///
/// This currently returns the border-box rect if `clips_contents` is true and `None` otherwise,
/// but in the future we may want to support more complex clip shapes,
/// in which case this method would need to be updated.
///
/// [`paint`]: crate::core::Widget::paint
/// [`pre_paint`]: crate::core::Widget::pre_paint
/// [`post_paint`]: crate::core::Widget::post_paint

///
/// [clip shape]: crate::doc::masonry_concepts#clip-shape.
pub(crate) fn clip_shape(&self) -> Rect {
self.border_box_size().to_rect() - self.border_box_translation()
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, that translation means that it returns the border box rect in content-box coordinate space.

To return the border box rect in border-box coordinate space, you just do this:

Suggested change
self.border_box_size().to_rect() - self.border_box_translation()
self.border_box_size().to_rect()

If instead you wanted to actually clip to the content-box rect, then it would be:

Suggested change
self.border_box_size().to_rect() - self.border_box_translation()
self.border_box_size().to_rect() - self.border_box_insets

However, then a bunch of comments would need updating as well.

@xStrom xStrom added masonry Issues relating to the Masonry widget layer waiting on author Needs author input before proceeding. labels Feb 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

masonry Issues relating to the Masonry widget layer waiting on author Needs author input before proceeding.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants