From e5fee03da8a15d332e3ed6201e58c38dacc21818 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Wed, 9 Sep 2026 19:54:53 -0400 Subject: [PATCH] Stop composer layout recursion on spaces --- app-rust/src/ui/composer.rs | 7 +++++ app-rust/tests/phone_screen.rs | 34 ++++++++++++++++++++++ docs/LAYOUT.md | 9 +++++- iris/core/src/ui/render_state.rs | 50 +++++++++++++++++++++++--------- 4 files changed, 86 insertions(+), 14 deletions(-) diff --git a/app-rust/src/ui/composer.rs b/app-rust/src/ui/composer.rs index 2a91ef6..266dd3c 100644 --- a/app-rust/src/ui/composer.rs +++ b/app-rust/src/ui/composer.rs @@ -112,6 +112,13 @@ where // the transcript: measured at 58px of stray text for a 475px message // in a 417px box. let content = field + // A wrapping editor occupies the composer's width even when its + // current text is short. Without this inner constraint the vertical + // ScrollArea reports the text's narrow natural width to Pad; settling + // then offers that width back to the editor, where a trailing space + // wraps and doubles its height. The next parent pass restores the + // wide one-line answer, so the two layouts have no fixed point. + .width(rest(1)) // `scrollable_to_end`: what is being typed is at the end, so a // message longer than the six lines shown holds that end. .scrollable(Axis::Y, Pin::End) diff --git a/app-rust/tests/phone_screen.rs b/app-rust/tests/phone_screen.rs index 7d2b81e..7f1c3df 100644 --- a/app-rust/tests/phone_screen.rs +++ b/app-rust/tests/phone_screen.rs @@ -225,6 +225,40 @@ fn the_composer_sits_above_a_simulated_ime_inset() { ); } +/// A trailing space is narrower than the composer's available width but can +/// wrap when the field is measured again in its own reported width. The +/// settling walk must not bounce forever between those one- and two-line +/// answers. On Android that recursion exhausted the native UI thread's stack +/// and ended in SIGSEGV, before Rust's panic hook could write anything. +#[test] +fn a_space_in_the_composer_finishes_layout() { + let (mut h, screen) = opened(); + screen.composer.set_bottom_inset(&mut h.rsc, 1000.0); + h.frame(PHONE_FRAME_MS * 2); + h.state.set_focus(Some(screen.composer.field)); + screen.composer.field.edit(&mut h.rsc).set_cursor_byte(0); + + for text in ["h", "i", " "] { + screen.composer.field.edit(&mut h.rsc).insert(text); + h.frame(PHONE_FRAME_MS); + } + + assert_eq!(screen.composer.field.edit(&mut h.rsc).text.text(), "hi "); + let region = h + .render + .window_region(&screen.composer.field, &h.rsc) + .expect("the composer field is drawn"); + let size = region.bot_right - region.top_left; + assert!( + size.x > phone_size().x / 2.0, + "the field shrink-wrapped to the short message: {region:?}" + ); + assert!( + size.y < 30.0 * PHONE_SCALE, + "the trailing space wrapped onto a second line: {region:?}" + ); +} + /// A newline typed into the composer must leave the caret inside the /// bar's own padding, not flush against its bottom edge. /// diff --git a/docs/LAYOUT.md b/docs/LAYOUT.md index d37cc1a..3a9b8ee 100644 --- a/docs/LAYOUT.md +++ b/docs/LAYOUT.md @@ -289,7 +289,14 @@ those changed widgets from the outside in, after their parents have assigned the final boxes. Otherwise a newly appended child can retain the provisional (even inverted) region it was measured in until another update happens. The downward work is confined to the branch that changed; unchanged descendants -still take `draw_inner`'s retained fast path. +still take `draw_inner`'s retained fast path. Each downward visit is exactly +one redraw, not another upward propagation: a wrapping child can have no fixed +point when an ancestor shrink-wraps it (a trailing space alternated between one +line in the offered width and two lines in its reported natural width). Feeding +that answer back into the same branch recursively overflowed Android's native +UI-thread stack before Rust could report a panic. A container that intends a +wrapping child to occupy its width declares that constraint explicitly; the +message composer does so on both sides of its vertical `ScrollArea`. ### 4. Wrapped text, and "needs child height before choosing width" diff --git a/iris/core/src/ui/render_state.rs b/iris/core/src/ui/render_state.rs index 0358ee0..d5f5d21 100644 --- a/iris/core/src/ui/render_state.rs +++ b/iris/core/src/ui/render_state.rs @@ -1217,18 +1217,50 @@ impl UiRenderState { /// newly grown subtree can retain the provisional (even inverted) region /// it was measured in until an unrelated later update redraws it. fn redraw_and_settle(&mut self, id: WidgetId, rsc: &mut dyn UiRsc) { + let Some((parent, changed)) = self.redraw_once(id, rsc) else { + return; + }; + if changed { + if let Some(pid) = parent { + self.redraw_and_settle(pid, rsc); + } + // The parent pass above has now placed this widget in its final + // region. Draw it once more there; unchanged descendants still + // take draw_inner's retained fast path. This is deliberately one + // redraw rather than another settling pass: feeding its size + // back into the same upward walk can alternate between the + // provisional and final regions forever (a text edit first did + // that when an Android IME committed a space), overflowing the + // native thread's stack before Rust can report a panic. + let settled_size = self.active.get(&id).map(|active| active.size); + let _ = self.redraw_once(id, rsc); + debug_assert_eq!( + self.active.get(&id).map(|active| active.size), + settled_size, + "a widget changed size after its parent settled its final region" + ); + } + } + + /// Redraw `id` exactly once, returning its parent and whether the size it + /// reports changed. [`Self::redraw_and_settle`] owns any propagation; in + /// particular, its final downward redraw must not start another upward + /// pass through the same branch. + fn redraw_once( + &mut self, + id: WidgetId, + rsc: &mut dyn UiRsc, + ) -> Option<(Option, bool)> { rsc.widgets_mut().needs_redraw.remove(&id); // An ancestor is drawing this widget right now, and that draw is // about to write fresh primitives for it. Drawing it a second time // here would leave one of the two copies on screen with nothing // owning it -- see `draw_started`'s own doc. if self.draw_started.contains(&id) { - return; + return None; } - let Some(active) = self.remove(id, false, true, rsc) else { - return; - }; + let active = self.remove(id, false, true, rsc)?; let old_size = active.size; let parent = active.parent; // `old_move_slot` being `Some` below means the slot is reused in @@ -1259,15 +1291,7 @@ impl UiRenderState { // there is no query left that answers "what size would this be" // without actually drawing (LAYOUT.md section 5). let changed = self.active.get(&id).map(|a| a.size) != Some(old_size); - if changed { - if let Some(pid) = parent { - self.redraw_and_settle(pid, rsc); - } - // The parent pass above has now placed this widget in its final - // region. Draw it once more there; unchanged descendants still - // take draw_inner's retained fast path. - self.redraw_and_settle(id, rsc); - } + Some((parent, changed)) } }