# 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`. ## Sixth sweep: the shader boundary and the position widgets (2026-09-20) Over what the five earlier rounds did not name -- the WGSL prelude and how it is assembled, the position widgets, `orientation/`, and the sensor walk. **Scoped against `upstream/main` at `ca2b4b2`, which is PR #19's real base**; the first half of this sweep used the local `main` and had to be redone, for which see "The branch layout" in `docs/HANDOFF.md`. Two findings. The cold dump is byte-identical to `1096c31` and all three seed scans pass (400 at depth 5 in 69.07s, 1,000 at depth 6 in 169.29s, 2,000 at depth 4 in 300.75s). **A number both sides count in, written twice.** `module_source` already builds each shader's preamble from `iris_core`'s own constants, with a comment saying why: "a grid the two disagree about puts every coordinate somewhere else". The move-chain work then added, to the very file it prepends, a second copy of two of its own numbers -- `const MOVE_NONE` and `const CHAIN_LIMIT`, under "Keep in step with `iris_core::CHAIN_LIMIT`" -- asking a reader by hand for what the mechanism beside it exists to do, and duplicating the reasoning `CHAIN_LIMIT`'s Rust declaration already carries. Both are injected now and the shader declares neither. `every_shader_validates` composes the real preamble so it covers the change; what it could never have caught is the two numbers drifting apart, which is now unrepresentable. `MASK_NONE` went in beside them, replacing a bare `4294967295u` in `masked` -- where the CPU deliberately keeps `MaskIdx` and `MoveIdx` as separate types so the two cannot be swapped. **That literal pre-dates this PR**; it is a one-line drive-by in a block the PR was already rewriting, taken because leaving it means a named sentinel for one index and a magic number for its sibling four lines apart. Drop it if the scope matters more. **A scroll positioning content it does not position.** Two halves, one mistake, both new in this PR -- the base's `Scroll` has none of this machinery. The author believed `Scroll` places its own content. `content_len` is `answer_px.max(container_len)`, so it is never less than the box. Two lines on, `slack` was `(container_len - content_len).max(ZERO)` and `anchor` was `slack * align.rel()` -- provably always zero, whatever the alignment, with `moved` then testing `anchor != ZERO` for nothing. Instrumented at `1096c31`, a centred scroll over 50 px of content in a 200 px box prints `slack=0 align=AxisAlign(0.5) anchor=0` and centres the content anyway: the framework does it, by placing the inner's answer in the whole box, which comes out as `rel 0.5 - px 25` and resolves at any length. The comment credited the arithmetic for behaviour it could not produce. The same belief cost a redraw. The contract for content that fits was guarded by `align == AxisAlign::NEG`, on the reasoning that at any other alignment "it moves with every length the box takes and the drawing holds for that length alone". It does not move -- the framework's placement is a fraction of the box -- and the default alignment is the middle, so the common case was the guarded one. `Painter::px_len` holds a drawing to the length it read unless the widget says otherwise, so with no `holds` stated the scroll redrew on every box change. Measured with `distinct_widgets`: 1 at `CENTER`, 0 at `TOP_LEFT`. Dropping the alignment test gives 0 at all three, which is what `a_fitting_scroll_holds_for_every_box_its_content_fits_in` asserts; it fails at `1096c31` on the `CENTER` case. `align` had no other reader, so the widget no longer asks its own alignment at all -- which is the tell that both halves were one mistake. **`UiSpan::translated` and `UiRegion::translated`** arrived on this branch and are reachable only from each other, which is why a plain "is this name used anywhere else" scan does not see them: neither looks unused on its own. The region one carried a performance argument for a function nobody calls. Both deleted. ### Kept although they pre-date this PR `GlyphAtlas::glyph_count`, `TextBuffer::new_empty` and the let-chain in `TextEdit::apply_event` that #10 had expanded into a nested `if`. All three were found under the wrong base and are outside #19's diff; Bryan said to keep them anyway (2026-09-20), since the two are dead either way and the third is a straight restoration. ### Tripped a rule and left as it stands - `diag::untrace_widget` is new here with no caller, which is the test the `translated` pair was deleted by. Left: it is the "removed" half of what `trace_widget`'s own doc promises ("until explicitly removed or cleared"), and `clear_traced_widgets` is the "cleared" half and does have one. A rig drives this API from outside. - `UiSpan::flip` swaps `start.rel` with `end.rel` and `start.px` with `end.px`, where `Len` has exactly those two fields and one swap of the structs would do. Pre-dates this PR. - `UiVec2` translates under three names -- `shift`, `offset`, and `UiRegion::offset` -- beside `Len::offset`, which adds pixels to one end and is a different operation. Pre-existing, and one vocabulary is Bryan's call rather than a sweep's. - The nine `#[allow(unused_variables)]` are all on trait methods with empty default bodies, where the parameter names are the signature's documentation and underscoring them would hide it from implementors. - `GpuPages::update` grows before it drains and both go through the one queue, so the copy is ordered before the writes. Correct as it stands. ## Fifth sweep: the retained path, the renderer and the diagnostics (2026-09-20) Over the parts the four earlier rounds did not read -- the renderer, the text store, the input default, the harness -- and once more over `redraw`. Six findings, `d8d5122` through `1096c31`, the last two from Bryan's reading of the first four. The cold dump is byte-identical to `781199a` and all three seed scans pass (400 at depth 5 in 69.45s, 1,000 at depth 6 in 214.69s, 2,000 at depth 4 in 419.50s). **A contract was kept where it no longer held** (`d8d5122`). The sibling of `713e3e7`, in the same function. `redraw` keeps the narrower of the old and the fresh contract so widening and narrowing back do not churn the parent. The drawing's half asks `was_holds.contains(window, rel_base, region)` first; the answer's half did not. A widget whose answer contract widened in a frame that also resized the window therefore kept a range the new window is outside, and the parent's next ask refused it and redrew the whole subtree -- throwing away the drawing that widget had just made. Cost, not geometry: the size kept is the size just reported. It needs both a resize the root does not absorb and a mark on a deeper widget in the same frame, which is what `a_contract_this_window_is_outside_is_not_kept` builds; it draws the leaf twice at `781199a` and once with the guard in. **Configuring a surface under its own texture** (`02048ea`). wgpu 30 says at both `Surface::configure` and `Surface::get_current_texture` that configuring while a texture the surface handed out is still alive panics. The `Suboptimal` arm of `UiRenderer::draw` configured with the texture it was about to draw with in hand, so the first suboptimal frame -- a resize or a display change on some drivers -- takes the app down rather than rebuilding the swapchain. The texture is good for that frame, so it is drawn with and presented and the rebuild happens after `present` consumes it. Not reproducible on demand here; the claim rests on wgpu's own documented panic. This one is the reason for the sweep recorded under "The code written before the review gate" below: nothing about the arm was hard, and it was written that way anyway. **A counter named the wrong contract** (`9b4cc32`). `AxisHolds` is four contracts and `diag::outside` counted three: a refusal because this window is outside the range the drawing was made for bumped "reuse outside: a rel base". A window range is pixels and a rel base pin is a window-unit length an unchanged window can still change, so the rig answered "why did that redraw?" with the wrong one for every resize. Same class as `8088a1f`. **Things nothing reads** (`7502176`, `1096c31`). `Axis::pair`, `RegionAlign::NEAR` and `Painter::text_data` arrived on this branch with no caller and never got one. The comment beside a span's cross-axis accumulator said a scalable child "makes Children scalable too" -- `Children` names nothing in this repository, and what it makes scalable is the span. `text_data` was held back a round on the grounds that it is the only way a widget inside `draw` can reach `TextData`, and the app's integration might want it. That reasoning is wrong: nothing in iris is kept for the app's sake, because the app is to be largely rewritten against this API rather than ported call by call (Bryan, 2026-09-20). **A question asked through a value** (`445287c`). `7502176` moved `Holds::contains` from `&self` to `self` to match its five siblings, which was the wrong way to reconcile them. A method taking `self` can only be called on a value, so a caller holding a reference has to dereference to ask -- `Copy` or not (Bryan, 2026-09-20). Every method that answers a question about a value now takes `&self`: `Holds`, `AxisHolds` and `LayoutHolds` throughout, `LayoutLen::{is_px, is_only_leftover, declared, fills}` and `Size::within_box`. Builders that return a changed copy still take `self`. ### Tripped a rule and left as it stands - `TextData`'s spare store clones the whole string into `Placed` on every re-break. Bounded at 128 entries, but the clone is per re-break and proportional to the text; a transcript-sized text would pay it on every width change. Left because a cheaper key changes what "two texts of the same words share an answer" means, which is a design question. - `TextEdit`'s undo history pushes a whole copy of the text per changed keystroke and is never bounded, and `apply_event` clones the text on every event including the arrow keys. **`apply_event` is not in this PR's diff at all**: it was last touched by #10 and #16, both already on `upstream/main`. It keeps resurfacing in sweeps because they diffed against the local `main` -- see "The branch layout" in `docs/HANDOFF.md`. Bryan wants the unbounded push and the clone dealt with as a change of their own (2026-09-20). - `ActivationState::update` writes four arms where the `Start`/`On` and `End`/`Off` pairs are identical, and `is_off` is `!is_on`. Also verbatim from `main`. - `TextView::draw`'s empty-with-hint branch matches on `self.hint` again after `is_some()` guarded it, so its `None` arm is unreachable. The guard cannot become an `if let` because `self.render(painter)` needs `&mut self` between the two. Left rather than cloning the handle to satisfy the shape. - `CurrentSurfaceTexture::Lost` is answered by reconfiguring, where wgpu says to recreate the surface. It will not panic, and recreating needs the window; worth doing with the next renderer change rather than this one. ## Quality sweep of the whole branch (2026-09-20) A fourth sweep, over the layout core, the arithmetic, the atlas, the sensor walk and the fuzz rig rather than over naming. Four findings, all on `layout/one-ask` past `1ebd4d3`; the cold dump is byte-identical to it and all three seed scans pass (400 at depth 5 in 65.09s, 1,000 at depth 6 in 161.27s, 2,000 at depth 4 in 301.90s). **A kept contract was judged against the wrong box** (`713e3e7`). `redraw` keeps the narrower guarantee a parent holds when the fresh drawing covers it, so widening and narrowing back do not churn the parent. It asked `was_holds.contains(.., active.placement)` -- where the answer put the drawing -- when `holds` is about `active.region`, the box the drawing was made in. The two differ on every axis a widget reported less than it was offered, so such a widget marked its parent every time its contract widened. Cost, not geometry: accepting is always safe, since `region` is always inside the old range, so refusing only escalates. `resize` and `try_reuse` both already ask about `region`. `widening_what_a_drawing_holds_for_does_not_relay_out_the_parent` fails at `1ebd4d3` and passes with the line changed; the existing `widening_and_restoring_a_contract_does_not_invalidate_its_reader` cannot see it, because its leaf reports `LEFTOVER`, which fills its box. **Two things nothing read** (`aea0387`). `ActiveData::size_deps` was written on every draw and cleared on every undraw, and read nowhere -- a `Vec` per active widget. The `Painter`'s own copy is the live one, used in `draw_at` to record whoever asked about a child it did not draw. `SizeRule::apply` had no caller and would have been wrong with one: it answers the rule's own length where `draw_at` resolves a fraction against the rel base first. **Three reuse rejections said nothing** (`8088a1f`). Of the eight rejections in `try_reuse`, a changed inherited mask counted and traced nothing, an undrawn record traced without counting, and a changed region-node choice counted without tracing. The mask one is what this branch's repair was about, so the rig could not answer "why did that redraw?" for it. Adding a counter meant editing a variant list and a name list at the same index; they are one declaration now. **A fuzz case ran only in the long scan** (`69ba915`). `Case::SizeResize` was in `ALL` and in none of `generated.rs`'s `case!` invocations, so the size-then-resize order -- which the enum's own comment argues is not the same test as the other order -- was never checked by `cargo test`. The tests and the list of which cases have one come from one macro invocation, and a case missing from it now fails a test. ### Tripped a rule and left as it stands - `Span` reads every child's cross length through `place_at(..).len(!axis)` even where `has_exact_size(!axis)` makes it moot. The read looks like an unwanted dependency, but `depend_on` only matters for a child that is not in `children`, which is how `undraw` keeps a measured-then-dropped child reachable. For a placed child it does nothing. - `PixelRegion::contains` is inclusive at both ends, so two adjacent widgets both claim the boundary step. Senses on one layer never block each other, so both receiving it is what the design says. - `CursorData::sense` is meaningless until `should_run` fills it, which the code says in place and proposes a prepare stage for. A real unrepresentable-state finding, but it is the event API's shape rather than this branch's. - `Wrapper` with no child answers `Size::default()`, which is `LEFTOVER`. It reads as "nothing" but matches `impl Widget for ()`, whose comment says a gap takes the default length so a span gives it a share. - `ALL` in `tests/scenario/mod.rs` is still a hand-kept list of every `Case`; `Case::name`'s match is the compiler-checked one. A variant left out of `ALL` is invisible to the shrinker's `--case` selection too. ## Naming and logic sweep (2026-09-19) Settled with Bryan across one session, on the branch past `58ce74d`. Nothing here changed what layout computes: the cold dump is byte-identical to `58ce74d` at every commit. **How a description is said.** A `PlaceDescAxis` is built by chaining off the value that says it -- `UiSpan::within_desc`/`shifted_desc`, `Len::as_desc` -- never by a constructor naming the type, because a constructor sends the reader back to the start of the line. The `_desc` suffix is what says which type comes out. `PlaceDescAxis::on_axis(axis)` lifts one axis into a pair with the whole box across it; `on` alone was rejected as contentless and reserved for events. `from_axes` is the constructor taking a function, beside the `from_axis` taking one axis and two values. **Arithmetic that needed a comment became a name.** `UiSpan::place` was the aligned-placement rule written out three times; `LayoutLen::without_leftover` was the sibling `apply_leftover` never had, at six sites; `is_px`, `is_only_leftover` and `declared` name field comparisons the surrounding comments had to translate; `Holds::covers` was interval containment by hand. Seven module-level functions became methods on the value each took first. **Every pair is a struct of two per-axis values, read with `[axis]`.** `LayoutHolds` was four two-element arrays, so none of its own operations could be written once; it is `AxisHolds` on `x` and `y`, and `and`, `covers` and `contains` lost their loops. `impl_axis_index!` gives every pair `Index`/`IndexMut`, replacing eighteen `axis`/`axis_mut` methods -- `const_index` keeps them usable in const context. The bare `[Option; 2]` became `Declared` of `Option`, which makes "a share is never a declaration" structural rather than two filters and a comment. **Two findings in the logic, both one mistake.** A value computed from other state was being stored as if it were state, and in both cases the visible symptom was something that looked like an off-by-one: - A `Span` carried `start` as a third accumulator beside `fixed` and `taken`, assigned at three points, when every assignment was `reached(fixed, taken)`. Both ends of a slot are now read where they are used; the variable and two of the three calls per child go, and the gap added after the last child derives nothing rather than needing to be subtracted. - The measuring loop's `cursor` added `px` and `rel` by hand where the placing loop below said `fixed += len.without_leftover()` -- the same sum, one of them named. **One property that held but nothing guarded.** A `Scroll`'s draw writes `amt` and `snap_end`, so a second draw at another viewport reads what the first wrote. Warm matches cold only because re-clamping is idempotent and monotone. The seed scans build `Scroll`s and never scroll one, so this was untested; `a_scrolled_view_resized_lands_where_a_cold_layout_puts_it` scrolls four distances, one past the end, then widens. It passes. ## 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 fills. 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 Everything about naming is done: the unswept `extent`, the two same-typed boxes on `ActiveData` and `Painter`'s four holds accumulators in `5642f20`, `Part::All` in `aeb60e5`, and `Place`/`Part` themselves in `58ce74d`. The settled vocabulary and the ask API are in `docs/LAYOUT.md`. The clarity sweep of `3da1c71` and `7e2b4cd` closed the first three items that stood here. Both are cold-dump identical to `6c84b6f`, so none of it moved a box, and all three seed scans passed on `7e2b4cd` (400 at depth 5 in 66.25s, 1,000 at depth 6 in 188.34s, 2,000 at depth 4 in 300.80s) because `reposition` and `redepth` were restructured on the retained path: - `Answer` {size, holds} and `Drawn` {answer, drawing_holds} replace `(Size, LayoutHolds)` and the three-tuple with two `LayoutHolds` in it. `try_reuse` answers `bool` rather than `Option<()>`. - `Span::along` is `Span::slot`; `far` is `row`, `shares` is `has_room` beside a named `any_leftover`, and `reached` guards on the weight it divides by rather than on the numerator. - `DrawInfo::px` was the rel base in pixels, and all three readers printed it as the box the widget drew in. Removed; each reads `region.to_px(window)`. `Placing::window` existed only to feed it. - `diag::outside` holds the 23 counter lines that were inside `try_reuse`. - `ActiveData::is_region_node` replaces four copies of `move_idx != parent_move`; `Axis::BOTH` replaces `AXES` in three modules; `Len::rel_min`, `rel_max` and the unused `select_len` are gone. `1ebd4d3` then closed the `PlaceSpan` item. `PlaceSpan` and `RelBase` are `pub` and `ui/mod.rs` re-exports `place` by name rather than by glob, the way it already did for `painter`, so the six `pub(crate)` accessors (`stated_rel_base`, `narrows_rel_base`, `within_span`, `is_sized`, `does_fill`, `with_rel_base`) are gone and `in_parent` matches `(at.span, declared)`. `!at.is_sized()` was dead: `PlaceSpan::Sized` is built only by `Len::as_desc`, which sets `RelBase::Len(self)` in the same literal, and deleting `with_rel_base` removes the only writer that could have separated them. Visibility here is plain `pub` plus a named re-export wherever the path can be hidden (Bryan, 2026-09-19); `pub(super)` is for inherent methods on types the crate exports, where it cannot. What is left, none of it urgent: - `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. - `DrawInfo` and `ActiveData` both carry `placed` and `asked`, two `PlaceDesc` fields distinguished only by position in every literal. They are genuinely different and documented, but the names are past participles with no operand; a rename is Bryan's vocabulary call.