Potential issue when child expands container using margin

[abvadabra]

While using layout library in my project I've come across somewhat unexpected layouting behaviour.

Minimal reproducible example follows:

1. Create root item of arbitrary size, without any additional configuration it acts as layout container
2. In that root insert row container with unfixed size
3. In that row container insert item with fixed size, and margin from the bottom

Equivalent code:

    lay_id root = lay_item(ctx);
    lay_set_size_xy(ctx, root, 1, 100);

    lay_id row = lay_item(ctx);
    lay_set_contain(ctx, row, LAY_ROW);
    lay_insert(ctx, root, row);

    lay_id child = lay_item(ctx);
    lay_set_size_xy(ctx, child, 1, 50);
    lay_set_margins_ltrb(ctx, child, 0, 0, 0, 10);
    lay_insert(ctx, row, child);

In a case like that I would expect the following

1. row's height becomes 60, since item's height is 50 + item's bottom margin is 10
2. item is placed at the begginng of the row (row pos = [0, 20], item pos = [0,20])

It can be validated by a test:

    lay_run_context(ctx);
    lay_vec4 root_rect = lay_get_rect(ctx, root);
    lay_vec4 row_rect = lay_get_rect(ctx, row);
    lay_vec4 child_rect = lay_get_rect(ctx, child);

    LTEST_VEC4EQ(root_rect, 0, 0, 1, 100);
    LTEST_VEC4EQ(row_rect, 0, 20, 1, 60);
    LTEST_VEC4EQ(child_rect, 0, 20, 1, 50);

It is consistent with like other layout engines implement margins (like yoga layout or flexboxes in basically all modern browsers).

However, in current version of the library:

1. row's height indeed calculated as 60
2. but item is placed above the row, at position [0,15]

From my understanding, the culprit is the lay_arrange_overlay_squeezed_range function, specifically line 1104:

// in LAY_HCENTER case
rect[dim] += (space - rect[2 + dim]) / 2 - margins[wdim];

The following change seems to be fixing the problem, and doesn't break any of the tests

--- a/layout.h
+++ b/layout.h
@@ -1101,7 +1101,7 @@ void lay_arrange_overlay_squeezed_range(
         switch (b_flags & LAY_HFILL) {
             case LAY_HCENTER:
                 rect[2 + dim] = lay_scalar_min(rect[2 + dim], min_size);
-                rect[dim] += (space - rect[2 + dim]) / 2 - margins[wdim];
+                rect[dim] += (space - rect[2 + dim] - margins[wdim]) / 2;
                 break;
             case LAY_RIGHT:
                 rect[2 + dim] = lay_scalar_min(rect[2 + dim], min_size);

Before submitting an actual pull request I would like to ask:

• Whether or not there is any actual reason this function implemented the way it is, or nobody just ever tried such use case
• If you can spot any other side effects of the suggested chang which are not covered by tests, but may turn out to be a breaking change

[randrew]

Hmm. I'm not sure why it's implemented the way it is. It's been a long time since I've looked at it. You might be right that this is a bug or mistake. I wonder if it would mess up anyone else's projects by making this change. Maybe it's fine to change it. Hmm.

[codecat]

This change doesn't appear to break anything in my projects, and I think the suggested behavior makes more sense than the current behavior.

If this change is made, I think you'd probably also want to make the same change in lay_arrange_overlay.