Repository: typst/typst
File: crates/typst-layout/src/shapes.rs (Rust)

Code before:
        }

        // Layout the child.
        frame = crate::layout_frame(engine, child, locator.relayout(), styles, pod)?;

        // If the child is a square or circle, relayout with full expansion into
        // square region to make sure the result is really quadratic.
        if kind.is_quadratic() {
            let length = frame.size().max_by_side().min(pod.size.min_by_side());
            let quad_pod = Region::new(Size::splat(length), Axes::splat(true));
            frame = crate::layout_frame(engine, child, locator, styles, quad_pod)?;
        }

        // Apply the inset.
        if has_inset {
            crate::pad::grow(&mut frame, &inset);
        }
    } else {
        // The default size that a shape takes on if it has no child and
        // enough space.
        let default = Size::new(Abs::pt(45.0), Abs::pt(30.0));
        let mut size = region.expand.select(region.size, default.min(region.size));
        if kind.is_quadratic() {
            if region.expand.any(|ex| *ex) {
                size = region.size;
            }
            size = Size::splat(size.min_by_side());
        }
        frame = Frame::soft(size);
    }

    // Prepare stroke.
    let stroke = match stroke {
        Smart::Auto if fill.is_none() => Sides::splat(Some(FixedStroke::default())),
        Smart::Auto => Sides::splat(None),
        Smart::Custom(strokes) => {
            strokes.unwrap_or_default().map(|s| s.map(Stroke::unwrap_or_default))
        }
    };

    // Add fill and/or stroke.
    if fill.is_some() || stroke.iter().any(Option::is_some) {
        if kind.is_round() {
            let outset = outset.unwrap_or_default().relative_to(frame.size());
            let size = frame.size() + outset.sum_by_axis();
            let pos = Point::new(-outset.left, -outset.top);
            let shape = Shape {
                geometry: Geometry::Path(Path::ellipse(size)),
                fill,
                stroke: stroke.left,
                fill_rule: FillRule::default(),

Review comment (bug):
I must admit I'm not too familiar with the surrounding code, but if I understood correctly, the problem is that `expand` will be false for the missing dimension (e.g. `#square(height: 5cm)` is missing width, which will be `auto`, so `expand` will be false on the horizontal axis), in which case, from the `select` above, the height will be kept in `size`, but the width will be set to `default.min(region.size)`, which will be smaller than the height (unless it is smaller than the default width), and thus `size.min_by_side()` will assign the default width as the height as well, leading to the bug. The buggy behavior here (at least conceptually) is precisely that the specified height should actually take priority over the unspecified width, thus it should not be determining the minimum size from both the specified region and the default, only from the specified region. (Unless no dimensions were specified, in which case the default should be used.) Does this check out?

To be honest, it feels a bit unclear to me why `expand` and `min` are being used to detect auto dimensions (where `min` will always return the default when region.size is infinity / auto); in previous code, there was an explicit `unwrap` for `auto` dimensions. I feel like this could be hiding some other bug.

Either way, I think there should be some more comments in the code to make this more explicit, in particular:

1. Indicating that `default.min(region.size)` will return the default dimensions when the region size is infinite (`auto`).
2. Indicating that `size.min_by_side()` should only consider the specified region size when at least one region dimension is fixed (in which case `expand` will be true), ignoring the other dimension set to the default value, if it received that value, to give priority to the specified dimension. This is, in theory, only necessary to check for quadratic shapes as they must be resized to ensure they have equal dimensions. Only the resizing is a problem.
(Though I do wonder if there could be some edge case generating problems with non-quadratic shapes as well, but I could not find one.)

Code after:
        }

        // If the shape is quadratic, we first measure it to determine its size
        // and then layout with full expansion to force the aspect ratio and
        // make sure it's really quadratic.
        if kind.is_quadratic() {
            let length = match quadratic_size(pod) {
                Some(length) => length,
                None => {
                    // Take as much as the child wants, but without overflowing.
                    crate::layout_frame(engine, child, locator.relayout(), styles, pod)?
                        .size()
                        .max_by_side()
                        .min(pod.size.min_by_side())
                }
            };

            pod = Region::new(Size::splat(length), Axes::splat(true));
        }

        // Layout the child.
        frame = crate::layout_frame(engine, child, locator, styles, pod)?;

        // Apply the inset.
        if has_inset {
            crate::pad::grow(&mut frame, &inset);
        }
    } else {
        // The default size that a shape takes on if it has no child and no
        // forced sizes.
        let default = Size::new(Abs::pt(45.0), Abs::pt(30.0)).min(region.size);

        let size = if kind.is_quadratic() {
            Size::splat(match quadratic_size(region) {
                Some(length) => length,
                None => default.min_by_side(),
            })
        } else {
            // For each dimension, pick the region size if forced, otherwise
            // use the default size (or the region size if the default
            // is too large for the region).
            region.expand.select(region.size, default)
        };

        frame = Frame::soft(size);
    }

    // Prepare stroke.
    let stroke = match stroke {
        Smart::Auto if fill.is_none() => Sides::splat(Some(FixedStroke::default())),
        Smart::Auto => Sides::splat(None),