diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index e2c91f2..1930c0d 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -5,20 +5,25 @@ they can be judged and reversed later. Detail lives in RUST.md (and IRIS.md for iris API changes); this file is only the summary. Newest first. Items marked **DEFERRED** are ones the agent chose not to decide alone. -## 2026-09-08 (later: a scroll area that grew redraws once) +## 2026-09-08 (later: a scroll area measures and places in one frame) From Iris's phone report about the composer's padding while typing -newlines. IRIS.md's entry has the account. +newlines, and the rule she stated when she read the first fix: layout is +a pure function of the state, nothing self-heals, and two draws to place +something happen in the same frame. IRIS.md's entry has the account. -- **A `Scroll` whose measured content length differs from the length it - offered its child asks for one more draw** (`Painter::draw_again`), - rather than the stale box being the last one drawn. Pays one extra - draw of the scroll's subtree when the content's length changes -- - including its first frame, where the offered length is a placeholder -- - and nothing on an ordinary scroll tick. -- **A frame that leaves any widget dirty now requests another frame** on - both backends. Previously only an animation did, so any use of - `draw_again` depended on some later input to deliver its correction. +- **`Scroll::draw` draws its child twice** -- once at last frame's length + to measure it, once at the measured length to place it -- instead of + placing against the stale length and leaving a wrong frame on screen. + The second draw is free unless the content's length changed. +- **An end-anchored `Scroll` is at its end on its first drawn frame**, a + consequence of the above. Two layout tests now build their area with + `at_end: false`, which is what they meant: they scroll down from the + top. +- **`List::clamp_to_content`'s next-frame correction is left in place** + and written down in docs/IRIS_TODO.md instead of fixed here, because + `List::place` is a larger piece of machinery and deserves its own + before/after on the phone. ## 2026-09-08 (every crate to its latest version, wgpu 28 -> 30) diff --git a/docs/IRIS.md b/docs/IRIS.md index 869343e..a54e1a8 100644 --- a/docs/IRIS.md +++ b/docs/IRIS.md @@ -12,48 +12,65 @@ things still stay out. An entry gives the date, what changed, why, and a short before/after where it helps judge the change without the session that made it. Newest first. -## 2026-09-08 (later): a `Scroll` whose content grew asks to be drawn again +## 2026-09-08 (later): a `Scroll` measures and places its content in one frame Iris's phone: "when typing with the keyboard up and entering enough newlines ... the text drops down close to the bottom and seems to ignore the padding. If I close (and optionally reopen) the keyboard it seems to fix itself." -`Scroll::draw` offers its child **last** frame's content length rather -than measuring afresh, deliberately: an ordinary scroll tick then hands -the child the same box it already has, which is what makes a tick an O(1) -move instead of a redraw (LAYOUT.md sections 2 and 4). The comment there -said the lag "self-corrects the next frame". It does not, because nothing -asks for that frame: a keystroke dirties the *field*, the frame it -requests draws the field in a box one line short of its new text, and the -tree is clean afterwards -- so the stale placement is simply the last one -drawn. The composer's text is centred in its box, so a box one line short -put half a line past each end, and the caret's line box landed a full -12dp below the bar's inside edge, flush against its bottom. Closing the -keyboard rewrote the bar's inset, which dirtied it, which is why it -"fixed itself". +`Scroll::draw` used to place its child against **last** frame's content +length. Every newline therefore drew the field in a box one line short of +its text, and since that text is centred in its box it hung half a line +past each end -- putting the caret's line box a full 12dp below the bar's +inside edge, flush with its bottom, with the padding eaten. The comment +there said the lag "self-corrects the next frame". There was no next +frame: a keystroke dirties the field, not the scroll area, and after that +frame the tree is clean, so the stale placement was simply the last one +drawn -- until the keyboard closed, whose inset rewrite dirtied the bar +and forced the redraw. That is the "it fixes itself" half of the report. -Two halves to the fix, and the second is the general one: +The rule Iris stated when she saw the first fix, and which the code now +follows: **layout is a pure function of the state, never of how many +frames have been drawn.** Nothing should heal itself, because nothing +should be drawn wrong in the first place; where two draws are genuinely +needed to place something, both happen in the same frame. -- **`Scroll::draw` calls `Painter::draw_again` when what it just measured - differs from what it offered** (by more than half a pixel). Costs one - extra draw when the content's length actually changes, and nothing on - an ordinary scroll tick, so the O(1)-move path is untouched. It - converges: the next draw offers the measured length and measures the - same thing again. -- **A frame that leaves anything dirty now asks for another frame**, on - both backends (`android::view`'s `post_frame_callback`, - `default`'s `request_redraw`). `draw_again` sets its mark *during* the - update, after the input path's own "does anything need redrawing?" - check has run, so before this nothing asked -- which quietly applied to - `List::clamp_to_content`'s use of the same mechanism too. +So `Scroll::draw` now draws its child twice: once at last frame's length +purely to measure it, then once at the length it just measured, with the +end-pin and the clamp applied only to that second placement. The same +measure-then-place idiom `Span::draw` and `List::place` already use. +**The second draw is free unless the content's length actually changed** +-- an ordinary scroll tick offers the same size at a new offset, so the +first call is `draw_inner`'s O(1) `mov` and the second, with an identical +region, returns at its first line. Growing a bottom-anchored area is +still O(1) in the sense that mattered; what it is not is free to place +its child against a length already known to be wrong. Last frame's length +survives only as a *hint* that keeps the common case cheap; nothing drawn +depends on it. + +Two consequences worth knowing: + +- **An end-anchored `Scroll` now sits at its end on its first drawn + frame**, not its second. It could not before: the end-pin needs the + content's length, which was a frame behind, so a fresh area showed its + start and jumped. Two layout tests that scrolled *down* from what they + assumed was the top now build their area with `at_end: false`, which is + what they always meant. +- **`List::clamp_to_content` is now the only place left that corrects on + the next frame** -- it finds a fling has run past the content's end and + marks itself for a redraw. Same defect, larger machinery; recorded in + docs/IRIS_TODO.md rather than folded into this change. Covered by `a_newline_leaves_the_caret_inside_the_composers_padding` -(layer 1, `transcript-fixture/tests/phone_screen.rs`), which fails on the -old code with the caret exactly on the bar's edge. `phone.rs` grew a -`--typed TEXT` argument beside `--message`: the two are different cases, -since one lays the composer out from scratch and the other grows one -already drawn, and only the second reproduces this. +(layer 1, `transcript-fixture/tests/phone_screen.rs`), which draws no +settling frame on purpose and fails on the old code with the caret +exactly on the bar's edge. On the emulator the caret's bottom moved from +1535 -- the bar's own bottom edge -- to 1509, 26px inside a 31px padding; +the remainder is parley's line box standing ~6px taller than its line +height. `phone.rs` grew a `--typed TEXT` argument beside `--message`, +since laying the composer out from scratch and growing one already drawn +are different cases and only the second reproduces this. ## 2026-09-08: what a cancel means, what a row's box is, and one fling for every scroll area diff --git a/docs/IRIS_TODO.md b/docs/IRIS_TODO.md index 81934b0..b4722ae 100644 --- a/docs/IRIS_TODO.md +++ b/docs/IRIS_TODO.md @@ -7,6 +7,24 @@ order and what "done" looks like. Tick and date them in place. ## Fix +- [ ] **`List::clamp_to_content` still corrects on the next frame + (2026-09-08).** Iris's rule, stated while the composer's caret was + being fixed: "nothing in the framework should ever self heal because + it should not be drawn incorrectly in the first place. If you need 2 + draws to get something into the correct position then that should + happen within the same frame. Layout should never be frame dependent, + it should be a pure function of the state." `Scroll::draw` was brought + to that rule the same day (it measures its content and places it + again in the one frame, IRIS.md's entry). `List::clamp_to_content` is + the one place left that has not been: it discovers a fling has run + past the content's end, calls `scroll(gap)` and `Painter::draw_again`, + and asks its own `RequestRedraw` handle for a frame -- so one frame is + drawn with the content past its end and the next one snaps it back. + The fix is the same shape as `Scroll`'s: re-place inside the draw that + found the gap. Not done in the same change because `List::place` is a + larger piece of machinery than `Scroll::draw` and this deserves its + own before/after on the phone. + - [x] **`request_device` asked for compute-shader limits it never uses (2026-09-05).** `Limits::default()` (both `iris/src/android/render.rs` and `iris/src/default/render.rs`) requests desktop-tier compute limits diff --git a/iris/src/android/view.rs b/iris/src/android/view.rs index 8752d56..39ce3d7 100644 --- a/iris/src/android/view.rs +++ b/iris/src/android/view.rs @@ -518,19 +518,7 @@ impl IrisViewPeer { // A frame callback is one-shot, so an animation that wants // another frame has to say so every frame -- unlike `after_input`, // which only has to ask when input dirtied something. - // `animating` is not the only thing a frame can leave - // unfinished: a widget that can only discover a correction to - // itself by laying out once asks for another draw with - // `Painter::draw_again` (`Scroll`, learning its content's real - // length; `List::clamp_to_content`). That mark is set *during* - // `update` above, after `after_input`'s own check has run, so - // without this nothing asks for the frame that applies it and - // the stale placement is the last one drawn. - let unfinished = { - let ui_state = self.state.android_state(); - self.render.needs_redraw(&ui_state.root, self.rsc.widgets()) - }; - if animating || unfinished { + if animating { ctx.view.post_frame_callback(&mut ctx.env); } if crate::diagnostics::trace_enabled() { diff --git a/iris/src/default/mod.rs b/iris/src/default/mod.rs index 45b89c2..b707ada 100644 --- a/iris/src/default/mod.rs +++ b/iris/src/default/mod.rs @@ -347,17 +347,7 @@ impl AppState for DefaultApp { let draw_start = std::time::Instant::now(); ui_state.renderer.draw(); crate::diagnostics::log_frame(render, frame_start, draw_start.elapsed(), animating); - // `animating` is not the only thing a frame can leave - // unfinished: a widget that can only discover a - // correction to itself by laying out once asks for - // another draw with `Painter::draw_again` - // (`Scroll`, learning its content's real length; - // `List::clamp_to_content`). That mark is set *during* - // this update, so the input path's own check - // (`window_event` below) has already run and nothing - // else would ask -- leaving the stale placement as the - // last one drawn. - if animating || render.needs_redraw(&ui_state.root, rsc.widgets()) { + if animating { ui_state.window.request_redraw(); } // I4 (RUST.md): only produces a `TreeUpdate` when the named diff --git a/iris/src/layout_tests.rs b/iris/src/layout_tests.rs index 73541bf..4ef7ad6 100644 --- a/iris/src/layout_tests.rs +++ b/iris/src/layout_tests.rs @@ -53,7 +53,15 @@ fn scrolled_rects( let scroll = rsc .ui .widgets - .add_strong(Scroll::new(span.any(), Axis::Y, true)); + // Anchored at the *start*: every test below scrolls down from + // the top and states its sign convention against that. An + // end-anchored area now sits at its end from its first drawn + // frame (`Scroll::draw` measures and places in the same frame), + // so `at_end: true` here would mean scrolling down from a + // position that is already the bottom -- a clamped no-op, which + // reads as "the move path is broken" rather than as the test + // starting somewhere it did not mean to. + .add_strong(Scroll::new(span.any(), Axis::Y, false)); let weak = scroll.weak(); (weak, scroll.any(), rects) } @@ -337,7 +345,9 @@ fn a_scroll_measures_the_box_it_was_offered_not_the_window() { let scroll = rsc .ui .widgets - .add_strong(Scroll::new(tall.any(), Axis::Y, true)); + // Start-anchored, so the `scroll(-37.0)` below has somewhere to + // go -- see `scrolled_rects`' note on the same choice. + .add_strong(Scroll::new(tall.any(), Axis::Y, false)); let scroll_w = scroll.weak(); let scroll_id = scroll.id(); let capped = rsc.ui.widgets.add_strong(MaxSize { @@ -403,7 +413,9 @@ fn a_panned_widgets_own_hit_box_moves_exactly_once() { let scroll = rsc .ui .widgets - .add_strong(Scroll::new(tall.any(), Axis::Y, true)); + // Start-anchored, so the `scroll(-37.0)` below has somewhere to + // go -- see `scrolled_rects`' note on the same choice. + .add_strong(Scroll::new(tall.any(), Axis::Y, false)); let scroll_w = scroll.weak(); let root = scroll.any(); diff --git a/iris/src/widget/list.rs b/iris/src/widget/list.rs index 8b7c5af..286c4ff 100644 --- a/iris/src/widget/list.rs +++ b/iris/src/widget/list.rs @@ -831,8 +831,14 @@ impl List { /// widget rejecting its own default (`repair_anchor`). /// /// Applied to the anchor, so it lands on the *next* frame rather than - /// re-running this one: the same one-frame-lag `Scroll` accepts for - /// its content length, and one frame is 8ms on the phone. + /// re-running this one -- which means one frame is drawn with the + /// content past its own end, and **that is a deviation from the rule + /// that layout is a pure function of the state rather than of how + /// many frames have been drawn** (Iris, 2026-09-08; `Scroll::draw` + /// used to lag the same way and no longer does, so this is now the + /// only place left). docs/IRIS_TODO.md carries it; the fix is the + /// same shape as `Scroll`'s -- re-place within this frame instead of + /// marking the next one. fn clamp_to_content(&mut self, painter: &mut Painter, top: f32, bottom: f32) { if self.at_start == self.at_end { return; diff --git a/iris/src/widget/position/scroll.rs b/iris/src/widget/position/scroll.rs index 0457088..155b47f 100644 --- a/iris/src/widget/position/scroll.rs +++ b/iris/src/widget/position/scroll.rs @@ -94,60 +94,66 @@ impl Widget for Scroll { // density, and `draw` is where this widget meets the only thing // that knows it. self.density = painter.density(); + let density = self.density; - if self.snap_end - && let Some(content_len) = self.content_len - { - self.amt = content_len - self.container_len; - } - self.update_amt(); - - let mut region = UiRegion::FULL; - // The container's own length until the content has been measured: - // a zero-length region on the first frame would place the child's - // primitives against a box of no size. - let offered = self.content_len.unwrap_or(container_len); - region.axis_mut(axis).end = region.axis(axis).start.offset(offered); - let region = region.offset(Vec2::from_axis(axis, -self.amt, 0.0)); - - let used = painter.widget_within(&self.inner, region); + // **The child's box along this axis is its own length**, not the + // container's -- that is what a scroll viewport means, and it is + // why the offset is the only thing that moves. A length is not + // knowable without drawing (LAYOUT.md section 5), so this draws + // the child once to measure it and once to place it, the same + // measure-then-place idiom `Span::draw` and `List::place` + // already use. Written out rather than through + // `Painter::draw_twice`, which cannot take a closure needing + // `&mut self` while `self.inner` is borrowed. + // + // **Both draws are in this frame, and only the second one + // decides anything.** The first is handed last frame's length as + // a *hint* -- nothing about where the child ends up depends on + // it, and it exists only so that the usual case, where the + // content's length did not change, offers the same region twice: + // `draw_inner` then makes the first call an O(1) `mov` and + // returns at the first line of the second. A frame on which the + // content did grow or shrink pays one real extra draw, and that + // is a frame on which the content was being redrawn anyway. + // + // The alternative -- place against the hint and let the next + // frame fix it -- is what Iris found on her phone (2026-09-08): + // every newline typed into the composer drew the field in a box + // one line short of its text, and since that text is centred in + // its box it hung half a line past each end, putting the caret a + // whole 12dp below the bar's inside edge and flush with its + // bottom. There was no next frame: nothing dirtied that subtree + // again, so the stale placement was the last one drawn, until + // the keyboard closed and its inset rewrite forced a redraw ("it + // fixes itself"). **Layout is a pure function of the state, not + // of how many frames have been drawn** (Iris, 2026-09-08) -- a + // correction that needs a second frame is a frame drawn wrong. + // + // The container's own length stands in as the hint until + // anything has been measured: a zero-length region on the first + // frame would place the child's primitives against a box of no + // size. + let hint = self.content_len.unwrap_or(container_len); + let used = painter.widget_within(&self.inner, self.child_region(hint)); // A child reporting `rel` means "this fraction of what I was // offered", and what it was offered is this scroll area -- so the // container, again, is what that resolves against. - let measured = used - .axis(axis) - .apply_rest(painter.density()) - .to_abs(container_len); + let measured = used.axis(axis).apply_rest(density).to_abs(container_len); self.content_len = Some(measured); - // The content grew or shrank *this* frame, so the box it was just - // drawn in (`offered`, last frame's length) is the wrong one -- - // and the correction the comment above promises only happens if - // something draws this again. Nothing else will: an ordinary - // keystroke dirties the field, not this widget, and after that - // frame the tree is clean, so the stale placement is simply the - // last one drawn. - // - // What that looked like: every newline typed into the composer - // left the field drawn in a box one line short of its text, and - // since the text is centred in its box, it hung half a line past - // each end -- the caret on the new last line landing 31px below - // the box, flush against the bar's bottom edge with the 12dp - // padding eaten (Iris's phone, 2026-09-08; it "fixed itself" - // when the keyboard closed because the composer's inset rewrite - // dirtied the bar and forced exactly the redraw that is missing - // here). Costs one extra draw when the content's length actually - // changes, and nothing on an ordinary scroll tick, which is what - // keeps `draw`'s O(1)-move path above intact. Converges: the - // next draw offers `measured` and measures the same thing again. - // Half a pixel rather than exact inequality: a length that only - // differs in float noise is not a content change, and asking for - // a redraw on it would be the "redraws forever" case - // `Painter::draw_again` warns about. - if (measured - offered).abs() > 0.5 { - painter.draw_again(); + // Everything that decides the placement, against the length just + // measured: the end-pin, then the clamp `update_amt` shares with + // `scroll` and `drag`. Deliberately not also run before the + // measuring draw above -- clamping against the hint would let a + // stale length reduce `amt` in a way this pass cannot undo, and + // then where the content sits would depend on the previous frame + // after all. + if self.snap_end { + self.amt = measured - container_len; } + self.update_amt(); + let used = painter.widget_within(&self.inner, self.child_region(measured)); // The **content's** size, not the container's. A parent that can // grow (the composer's bar) should hug the text until its own cap @@ -263,6 +269,17 @@ impl Scroll { } } + /// Where the child sits for a given content length: a box that long + /// along the scroll axis, pulled back by `amt`. Taken as a parameter + /// rather than read from `content_len`, because `draw` places twice + /// -- once against last frame's length and once against the one it + /// has just measured -- and the two must be the same arithmetic. + fn child_region(&self, content_len: f32) -> UiRegion { + let mut region = UiRegion::FULL; + region.axis_mut(self.axis).end = region.axis(self.axis).start.offset(content_len); + region.offset(Vec2::from_axis(self.axis, -self.amt, 0.0)) + } + /// Clamp `amt` into the range the content allows, and re-read whether /// this area is sitting at its end. /// diff --git a/iris/transcript-fixture/tests/phone_screen.rs b/iris/transcript-fixture/tests/phone_screen.rs index c3e58b5..494ec23 100644 --- a/iris/transcript-fixture/tests/phone_screen.rs +++ b/iris/transcript-fixture/tests/phone_screen.rs @@ -227,14 +227,16 @@ fn the_composer_sits_above_a_simulated_ime_inset() { /// entering enough newlines ... the text drops down close to the bottom /// and seems to ignore the padding. If I close (and optionally reopen) /// the keyboard it seems to fix itself." The cause was `Scroll::draw` -/// offering its child *last* frame's content length (deliberate, so an -/// ordinary scroll tick is an O(1) move): each newline drew the field in -/// a box one line short of its text, and since the text is centred in -/// its box it hung half a line past each end, putting the caret's line -/// box a full padding below the bar's inside edge. Nothing dirtied that -/// subtree again, so the stale placement was simply the last one drawn -/// -- until the keyboard closed and the inset rewrite forced a redraw, -/// which is the "fixes itself" half of the report. +/// placing its child against *last* frame's content length and stopping +/// there: each newline drew the field in a box one line short of its +/// text, and since the text is centred in its box it hung half a line +/// past each end, putting the caret's line box a full padding below the +/// bar's inside edge. Nothing dirtied that subtree again, so the stale +/// placement was simply the last one drawn -- until the keyboard closed +/// and the inset rewrite forced a redraw, which is the "fixes itself" +/// half of the report. No settling frame here on purpose: the placement +/// is corrected within the frame that typed, so the first frame drawn +/// after a keystroke is already right. #[test] fn a_newline_leaves_the_caret_inside_the_composers_padding() { let (mut h, screen) = opened(); @@ -251,11 +253,6 @@ fn a_newline_leaves_the_caret_inside_the_composers_padding() { screen.composer.field.edit(&mut h.rsc).insert("a\n"); h.frame(PHONE_FRAME_MS); } - // The frame `Scroll`'s `draw_again` asks for. On a device this is - // the `post_frame_callback`/`request_redraw` the backends make when - // an update leaves anything dirty; here the harness drives it. - h.frame(PHONE_FRAME_MS); - // The caret is the last primitive `TextEdit::draw` emits. let caret = { let slot = *h