ai-app/docs/LAYOUT_LOG.md

# Layout findings log

What the sessions reviewing Iris's retained layout found, kept so that
nothing here is rediscovered. Each entry says who found it and when.
**Delete this file when #19 lands and its fixes are in**; what must outlive
it (settled design, the measurement method) belongs in `docs/LAYOUT.md`, and
the current plan is in `docs/HANDOFF.md`.

## Follow-up implementation review (2026-09-19)

The ask/place split, window-unit frames, exact validity preimages, and
bottom-up dirty settling implement the settled design. Keep this approach.
It does not guarantee one body call per widget: an unhinted descendant that
reports leftover weight still needs a room ask and a slot ask. The explicit
measurement redesign remains deferred until an app screen justifies it.

The fixes below are on `layout/one-ask` in `/home/bob/repos/iris`:

- A collapsed share advances both span cursors. The regression covers one
  and two collapsed children in all four directions.
- A masking widget owns a mask reference and reclaims its existing slot on
  redraw. Primitives retain their own references. Removing the mask,
  undrawing its owner, freeing the widget, and replacing the root release
  ownership; a changed inherited mask rejects drawing reuse. Tests check
  actual primitive mask indices, movement with and without a region node,
  child draw counts, clip removal/addition, and empty-mask slot reuse.
- **A further handover defect:** `draw_inner` saved the old parent only
  after a redraw replaced `ActiveData`. The old parent therefore kept the
  child in its list and could undraw the subtree after its new parent drew
  it. Capture the old parent before replacing the record. A branch-switch
  regression reproduces disappearing content when its new parent owns a
  region node, and also checks the ordinary reuse path.

The mask and handover tests fail on the reviewed code and pass with the
fixes. The mask fix preserves child reuse rather than redrawing descendants
on every mask repaint. No naming sweep or rounding-policy change is included.

Validation: workspace tests with and without diagnostics, the release fast
oracle, and all three prescribed seed scans pass. Comparing 34,488 cold
boxes against `cadfba0` finds 650 changes; withholding just the collapsed-slot
fix reproduces the baseline exactly. This is an expected geometry correction,
not a cost-only change whose dump should remain identical. The tabs example
and an exact 400 px collapsed-share fixture were rendered and inspected.

Two test-harness savings leave the random stream and coverage unchanged:
`generated` constructs one plan per seed for its sixteen scenarios, and the
warm/cold comparison constructs its diagnostic ancestry lookup only after
finding a mismatch. No overall speedup is claimed; no deep profile was run.

**Integration is larger than an API rename.** The app's pinned `32f6ad8`
has 45 commits not reachable from this review branch. In particular, the
app's nested/shape masks and shared `Ui` ownership are absent here: #19's
mask is still a single rectangle and `set_mask` rejects nested masks. Keep
the app pin until those existing capabilities have been integrated. The
"Masks" and "UI ownership" sections of `LAYOUT.md` describe the app-side
implementation, not everything already present on the upstream review branch.

## Original review of #19 at `cadfba0` (2026-09-19)

The original review read `cadfba0`, then the tip of `layout/one-ask`.
`cargo fmt --all --check`, clippy `-D warnings` and the 123-test suite were
clean there. These are the original failures, fixed by the follow-up above.

### A span misplaces the slot after a collapsed `leftover` child

`src/widget/position/span.rs:101` keeps two cursors while it places: `fixed`,
everything taken so far, and `start`, where the next slot begins. The branch
that drops a share child with no room to divide advances `fixed` by the gap
and not `start`:

```rust
if len.leftover > Weight::ZERO && len.px == Px::ZERO && len.rel == Rel::ZERO && !shares {
    painter.undraw(child);
    fixed.px += self.gap;
    continue;              // `start` still excludes this gap
}
let from = start;
```

In a 400 px row with `gap(10)` over children of 200 px, `leftover(1)` and
180 px -- exactly full, so nothing is left over and the share collapses --
the tail is placed at `(215, 0)..(395, 100)`. Its slot was 210..400, a gap
too long and a gap too early, and the declared 180 was then centred in it.
The row reports a total that ends at 400. A child drawn with `rel` lengths
is stretched into the extra gap instead of being centred in it, because the
slot is a `Place::Fill`.

Recomputing the cursor in that branch fixes it, and the tail lands at
`(220, 0)..(400, 100)`:

```rust
start = shared(fixed, taken, total.leftover, room);
```

The 123-test suite passes with the line in. Nothing in it or in the
generated oracle catches the defect: warm and cold layouts are wrong
identically, so an oracle comparing the two cannot see it. This is the
sharp form of the handoff's older "a vanished child leaves a double gap"
item, which described the accounting and not the misplacement.

### A reused child keeps the mask its parent replaced

`ActiveData::parent_mask` is recorded and documented as the inherited mask
"the one a redraw of it must not be handed back", but `try_reuse`
(`core/src/ui/render_state.rs:565`) never compares it with `info.mask`: it
gates on dirtiness, layer, move parent and region-node status only.
`Painter::set_mask` pushes a fresh `MaskIdx` on every draw, so a masking
widget that redraws while its child is reused leaves that child's
primitives naming a mask nobody updates again:

```
frame 1: masked mask=Id(0)  inner mask=Id(0)
frame 2 (the masked widget alone marked): masked mask=Id(1)  inner mask=Id(0)
frame 3 (the subtree moves up to y=10):
   mask 0: y starts at 50    <- what the inner's primitives are clipped by
   mask 1: y starts at 10    <- the live one, clipping nothing
```

The child's drawing is then clipped 40 px too high and its top is cut off.
`main` guarded this with `active.mask == mask` in its reuse gate and
re-marked the owners of a rebuilt mask in `remask_shape_users`; the rewrite
dropped both.

Returning `None` from `try_reuse` where `active.parent_mask != info.mask`
fixes the repro and keeps the suite green, but it redraws the whole subtree
whenever a masking ancestor redraws. The better fix is to give `set_mask` a
per-widget mask slot kept across redraws, the way `UiRenderState::move_slot`
already keeps a move entry, so the index is stable and `reposition` goes on
updating the one the descendants name.

### Regression coverage

Both paths now have regressions in `tests/cases/layout.rs` and
`tests/cases/retained.rs`; the follow-up above records the additional cases.
The descriptions above preserve the original failure at `cadfba0`.

### Clarity, in the order worth doing

The first three -- the unswept `extent`, the two same-typed boxes on
`ActiveData`, and `Painter`'s four holds accumulators -- were done in
`5642f20`. What is left:

- `draw_inner` returns `(Size, LayoutHolds, LayoutHolds)`, two same-typed
  values ordered by convention. A named pair makes the swap unwriteable.
- `DrawInfo::px` is computed at all four construction sites and read only by
  `diag::draw_request` and two `debug_assert!` messages. Compute it in the
  assert.
- The `layout-diagnostics` block inside `try_reuse` is 23 lines of counters
  in the middle of the decision; one `diag::outside(...)` call would keep
  the branch readable.
- `widget_at` does three linear scans per child (`children.contains`,
  `under.iter_mut().find`, `depend_on`), so a span of *n* children is
  O(n^2) per draw. Not a problem at today's sizes; it is worth knowing
  before a long transcript list lands on it.
- `Part::All` is exactly `Part::Of(UiSpan::FULL)`: `Of` composes through
  `within`, and `through(Len::FULL)` maps a range back to itself, which
  `the_whole_of_a_box_maps_back_to_itself` already asserts. It buys a
  separate arm in `in_parent` and in `Part::of`. Keeping it as sugar is
  defensible; the case analysis being wider than the geometry is the thing
  to decide about rather than to leave unstated.