From 48e412177c96a9e04f2e83d493246af25b0c060b Mon Sep 17 00:00:00 2001 From: funman300 Date: Wed, 6 May 2026 00:32:19 +0000 Subject: [PATCH] fix(engine): focus arrives on the same frame a modal opens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously when a click-handler in Update spawned a modal, attach_focusable_to_modal_buttons and auto_focus_on_modal_open ran in the same Update — but with no ordering edge to the click handler the deferred Commands wouldn't materialise in time, so attach saw no entities, FocusedButton stayed empty, and the very next Tab/Enter press wasted itself moving focus from None to the primary instead of activating it. Moves attach_focusable_to_modal_buttons + auto_focus_on_modal_open from Update to PostUpdate. The schedule boundary itself supplies the sync point: every modal spawned anywhere in Update is materialised before PostUpdate runs, attach can find the new ModalButtons, and FocusedButton is populated before app.update() returns. handle_focus_keys stays in Update so it observes input on the frame it occurs, reading FocusedButton written by the previous tick's PostUpdate. Two new tests pin the contract: - primary_button_is_focused_on_modal_spawn_same_frame uses a production-shaped spawner system (no chain edge to UiFocusPlugin) and asserts FocusedButton.0 is Some after a single update — fails without the fix, passes with it. - first_tab_after_modal_open_advances_to_secondary guards against a regression where focus arrives but the very first Tab moves from None to primary instead of from primary to secondary. Co-Authored-By: Claude Opus 4.7 (1M context) --- solitaire_engine/src/ui_focus.rs | 162 ++++++++++++++++++++++++++++++- 1 file changed, 161 insertions(+), 1 deletion(-) diff --git a/solitaire_engine/src/ui_focus.rs b/solitaire_engine/src/ui_focus.rs index 00de3f0..1f24154 100644 --- a/solitaire_engine/src/ui_focus.rs +++ b/solitaire_engine/src/ui_focus.rs @@ -121,11 +121,34 @@ impl Plugin for UiFocusPlugin { fn build(&self, app: &mut App) { app.init_resource::() .add_systems(Startup, spawn_focus_overlay) + // Attach + auto-focus run in `PostUpdate` so they see entities + // a click-handler in `Update` queued via `Commands` earlier in + // the same frame. If they ran in `Update` they'd race the + // click handler: there's no ordering edge between an arbitrary + // modal-spawning system and the focus chain, so Bevy's + // `auto_insert_apply_deferred` pass cannot synchronise them. + // Pushing the attach / auto-focus pair into `PostUpdate` puts + // the natural schedule-boundary sync point between every + // modal spawn and focus arrival — `FocusedButton` is always + // populated before the same `app.update()` returns. + // + // The remaining systems stay in `Update` so they keep + // observing input on the frame it occurs. They read + // `FocusedButton` written during the *previous* tick's + // `PostUpdate`, which is exactly what we want: the very next + // user keypress after a modal opens lands on a populated + // resource. .add_systems( - Update, + PostUpdate, ( attach_focusable_to_modal_buttons, auto_focus_on_modal_open, + ) + .chain(), + ) + .add_systems( + Update, + ( sync_focus_on_mouse_click, clear_hud_focus_on_unhover, handle_focus_keys, @@ -827,6 +850,143 @@ mod tests { assert_eq!(focused, Some(a), "Primary button A should auto-focus"); } + /// One-shot trigger resource consumed by the production-shaped test + /// system [`spawn_modal_via_system`]. When set to `true`, the system + /// queues a `spawn_modal` call on the next `Update` and clears the + /// flag. Mirrors the real production flow where a click-handler + /// system queues the modal spawn via `Commands` rather than the + /// test fixture using `world.flush()` ahead of time. + #[derive(Resource, Default)] + struct SpawnModalTrigger(bool); + + /// Production-shaped modal spawner: a regular Bevy `System` that + /// reads a trigger flag and queues a 2-button modal via `Commands`. + /// Crucially this system has **no** ordering relationship with + /// `UiFocusPlugin`'s chain — exactly the situation that surfaces the + /// "focus arrives one frame late" bug in production. + fn spawn_modal_via_system( + mut commands: Commands, + mut trigger: ResMut, + ) { + if !trigger.0 { + return; + } + trigger.0 = false; + spawn_modal(&mut commands, TestModal, 0, |card| { + spawn_modal_actions(card, |actions| { + spawn_modal_button( + actions, + TestButtonB, + "B", + None, + ButtonVariant::Secondary, + None, + ); + spawn_modal_button( + actions, + TestButtonA, + "A", + None, + ButtonVariant::Primary, + None, + ); + }); + }); + } + + /// Same-frame-focus contract: when a modal is spawned by an + /// independent system during the same `Update` as the focus chain, + /// `FocusedButton` must be populated with the primary button by the + /// time `handle_focus_keys` runs in that **same** update — so a Tab + /// pressed in the very next tick advances focus rather than + /// landing on "nothing focused → primary". + #[test] + fn primary_button_is_focused_on_modal_spawn_same_frame() { + let mut app = App::new(); + app.add_plugins(MinimalPlugins) + .add_plugins(UiModalPlugin) + .add_plugins(UiFocusPlugin); + app.init_resource::>(); + app.init_resource::(); + // Register the production-shaped spawn system in `Update` with + // no chain relationship to `UiFocusPlugin`. + app.add_systems(Update, spawn_modal_via_system); + // Initial Startup pass. + app.update(); + + // Trigger the spawn and run exactly ONE update — the same + // `Update` cycle that the focus chain runs in. By the end of + // this update, `FocusedButton` must already point at the + // primary button. + app.world_mut().resource_mut::().0 = true; + app.update(); + + let primary = app + .world_mut() + .query_filtered::>() + .iter(app.world()) + .next() + .expect("Primary button should exist after the spawn update"); + + assert_eq!( + app.world().resource::().0, + Some(primary), + "FocusedButton must be populated with the primary on the same frame the modal spawns" + ); + } + + /// Tab pressed on the very next tick after a modal opens must + /// advance focus from the primary to the secondary — not from + /// "nothing focused" to the primary. The latter would mean focus + /// arrived a frame late and Tab was wasted on first-focus instead + /// of advancing. + #[test] + fn first_tab_after_modal_open_advances_to_secondary() { + let mut app = App::new(); + app.add_plugins(MinimalPlugins) + .add_plugins(UiModalPlugin) + .add_plugins(UiFocusPlugin); + app.init_resource::>(); + app.init_resource::(); + app.add_systems(Update, spawn_modal_via_system); + app.update(); + + // Spawn the modal in update N. + app.world_mut().resource_mut::().0 = true; + app.update(); + + // Press Tab on update N+1. If focus arrived correctly in N, + // Tab advances primary → secondary. If focus arrived late, + // Tab promotes "no focus" to primary (the bug). + let primary = app + .world_mut() + .query_filtered::>() + .iter(app.world()) + .next() + .expect("primary spawned"); + let secondary = app + .world_mut() + .query_filtered::>() + .iter(app.world()) + .next() + .expect("secondary spawned"); + + press_key(&mut app, KeyCode::Tab); + app.update(); + + let focused_after_tab = app.world().resource::().0; + assert_ne!( + focused_after_tab, + Some(primary), + "first Tab after modal open should advance off the primary, not land on it (focus arrived late)" + ); + assert_eq!( + focused_after_tab, + Some(secondary), + "first Tab from primary should land on the secondary" + ); + } + #[test] fn tab_advances_focus_in_spawn_order() { let mut app = headless_app();