Fix box/block clipping when no stroke set

[kjagiello]

The clipping had no effect when no stroke was set, which I found a bit unexpected. This PR attempts to fix that by falling back to a default for the purpose of clipping.

For reference, the original fix for clipping was implemented in #2338.

[laurmaedje]

I wonder whether the fallback stroke should have default width or rather 0pt width.

cc: @Dherse

[kjagiello]

Just an observation, but when I was experimenting with it yesterday, the width of the stroke did not affect the clipping result. Though I guess it would make sense to default it to 0pt, just in case the width is ever taken into account when clipping.

[laurmaedje]

The stroke width is taken into account when computing the border radius, so I think it might have an effect here. Not entirely sure about this though.

[PgBiel]

If I analyzed the code path correctly, it does seem that stroke width will default to 0, which is considered in calculations:

typst/crates/typst/src/geom/rect.rs

Lines 161 to 170 in 8fd5467

	let stroke_widths = strokes
	.as_ref()
	.map(|s| s.as_ref().map_or(Abs::zero(), |s| s.thickness / 2.0));
	let max_radius = (size.x.min(size.y)) / 2.0
	+ stroke_widths.iter().cloned().min().unwrap_or(Abs::zero());
	let radius = radius.map(|side| side.relative_to(max_radius * 2.0).min(max_radius));
	let corners = corners_control_points(size, radius, &strokes, stroke_widths);

However, that doesn't explain why setting stroke to 0pt explicitly makes it work...

[PgBiel]

Okay, I ran a debugger, and the difference is that the else branch below does get entered when using an explicit stroke (even if 0pt), but not when stroke: none (the default), as the latter would cause the top stroke to return false for .is_some():

typst/crates/typst/src/geom/rect.rs

Lines 146 to 149 in 8fd5467

	} else if strokes.top.is_some() {
	// single segment
	path_segment(Corner::TopLeft, Corner::TopLeft, &corners, &mut path);
	}

Note that just changing the top stroke to none isn't enough to reproduce the problem, as, in that case, the if branch (instead of the else) would be entered (it seems to be triggered when at least two of the strokes differ). The problem here is a very specific case where neither the if or the else are entered when all strokes are identical (so, doesn't enter the if) and equal to none (so, doesn't enter the else), and thus the path returned is precisely the value returned by Path::new(). This is evidenced by the fact that setting the stroke to (top: none, rest: 0pt) (or other combinations between none and 0pt) works.

I believe the PR as is, thus, will likely produce regressions when not all strokes are none (because then they would all default to FixedStroke::default, which seems to have a thickness of 1pt etc.). Rather, the solution is likely to add an exception for the specific case of all strokes identical and equal to none, to avoid having neither of the if / else branches run.

[PgBiel]

Therefore, in principle, it could work to apply the following fix instead (which, locally, seems to solve the problem at hand):

diff --git a/crates/typst/src/geom/rect.rs b/crates/typst/src/geom/rect.rs
index 37b94527..319270f6 100644
--- a/crates/typst/src/geom/rect.rs
+++ b/crates/typst/src/geom/rect.rs
@@ -143,7 +143,7 @@ fn segmented_path_rect(
             last = current;
             path_segment(start, end, &corners, &mut path);
         }
-    } else if strokes.top.is_some() {
+    } else if strokes.top.is_some() || strokes.iter().all(Option::is_none) {
         // single segment
         path_segment(Corner::TopLeft, Corner::TopLeft, &corners, &mut path);
     }

However:

1. Testing is needed (in particular, please add tests using the green rectangle so it's more obvious if it failed or not);
2. Approval from @Dherse and (perhaps) @antonWetzel , who wrote the original code, is highly recommended.

In particular, the change above can cause regressions in other locations which use that function (all tests currently pass, but caution is advised). It might be better, thus, to use a safer alternative from within box and block instead, if desirable (the following diff, which seems to fix the problem as well from local tests, only contains the change for block, so it would be necessary to copy paste it on box code as well):

diff --git a/crates/typst-library/src/layout/container.rs b/crates/typst-library/src/layout/container.rs
index 36b62864..252f23e6 100644
--- a/crates/typst-library/src/layout/container.rs
+++ b/crates/typst-library/src/layout/container.rs
@@ -421,6 +421,13 @@ impl Layout for BlockElem {
                 let outset = self.outset(styles).relative_to(frame.size());
                 let size = frame.size() + outset.sum_by_axis();
                 let radius = self.radius(styles);
+                let stroke = if stroke.iter().all(Option::is_none) {
+                    stroke.clone().map(|_| {
+                        Some(FixedStroke { thickness: Abs::zero(), ..Default::default() })
+                    })
+                } else {
+                    stroke.clone()
+                };
                 frame.clip(path_rect(size, radius, &stroke));
             }
         }

I'm not sure which one is better as I didn't write the original code, so I'll let others give their opinions!

[Dherse]

@PgBiel I don't think you need my approval to fix a bug 😂 , this is indeed an issue, did it exist before my clip fix PR?

I also noticed another issue, in SVG, it seems that boxes always clip.

[antonWetzel]

With limited knowledge about clipping.

It looks like path_rect is only used for clipping, which always requires a path. The original code was written for the visual, so a rect without stroke can be skipped. To bugfix the stroke can be set to zero width or the change the code to always draw a rect.

At the moment the middle of the stroke is used for clipping, which ignores the radius.
I see 2 possibilities to fix this. One is calculating the inner path of the rect (see antonWetzel@8eaa1b4) or drawing the rect above the content if clip is active (see antonWetzel@e4779c0)

[PgBiel]

@PgBiel I don't think you need my approval to fix a bug 😂

Sorry if I didn't make myself clear; I meant approval as in make sure I'm not spitting out a bunch of lies about your code since I know next to nothing about it 😅

[laurmaedje]

What's the status of this?

[PgBiel]

What's the status of this?

I believe the PR shouldn't be merged as is, as I believe it could cause regressions. I proposed two alternative patches (one of which is very unlikely to cause regressions) as a stopgap, but that's likely a worse alternative than finding a proper solution in the clipping logic. @antonWetzel proposed two more lengthy alternative solutions instead, which are likely more appropriate than mine. Perhaps their solutions should be proposed separately and tested.

[laurmaedje]

Superseded by #2626, which resulted from the discussion here. Thanks everybody!