From 36eb05fab1af4d2e84bdf21953f7940c5ed34f37 Mon Sep 17 00:00:00 2001 From: InauguralPhysicist Date: Fri, 17 Jul 2026 23:08:04 -0500 Subject: [PATCH] fix(ui): a modal owns the keyboard, not just the pointer (#631) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit push_modal made a dialog modal for the pointer only. Keyboard focus never consulted _ui.modal_stack, so a user could Tab to a widget BEHIND an open dialog and activate it with Enter/Space — an "Are you sure? [Cancel]" dialog could fire the destructive action it exists to guard against. Two halves, one root cause (focus ignored the modal stack): 1. dispatch scoped the mouse to the top modal but returned keys with the real root ABOVE the rebind. 2. the focus list was only ever built from the real root, so a dialog's own children were never focusable and the background's always were. Fix — focus respects the modal stack: - dispatch: move the modal rebind ABOVE the key early-return, so handle_key (and thus _focus_next/_build_focus_list) scopes to the top modal exactly as hit-testing does. Escape still pops the modal (handle_key checks the stack directly, so it survives the rebind). - push_modal: rebuild the focus list from the dialog and clear the focus pointers, so Tab cycles inside the modal, the dialog's own children (the file_dialog text_input/item_list from #575) are immediately focusable, and a stray Enter before any Tab cannot reach a background widget. - pop_modal: rebuild focus from the newly-exposed context — the next modal down, or edit_root when the stack empties (null-guarded for headless use). Pre-existing, not introduced by #575, but #575 made it reachable a second way (dialogs can now host focusable children a user would expect to Tab between). Tests: 7 asserts in test_ui.eigs (gfx-stubbed, keyboard-only so no gfx dep) — Tab stays in the dialog, Enter fires the dialog child not the background button, Tab cycles between the dialog's children, and pop_modal returns focus to the background. 5 of the 7 fail on the pre-fix toolkit (the other two are weaker guards the stronger ones subsume). Suite 3085/3085 release, 3089/3089 ASan+UBSan (detect_leaks=1, tally 0). --- lib/ui.eigs | 41 +++++++++++++++++++++++++-------- tests/test_ui.eigs | 56 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 9 deletions(-) diff --git a/lib/ui.eigs b/lib/ui.eigs index 0f85a97..45de4bc 100644 --- a/lib/ui.eigs +++ b/lib/ui.eigs @@ -477,6 +477,15 @@ define push_modal(dlg, parent_w, parent_h) as: # moment it is pushed, and its children need absolute positions to be # hit-tested (#575). app_loop re-lays it every frame after this. _layout of [dlg, 0, 0] + # Scope keyboard focus to the dialog too (#631): rebuild the focus list + # from the dialog's own children and drop any background focus, so Tab + # cycles inside the modal and a stray Enter cannot fire a widget behind + # it before the user has Tabbed. Mouse modality already scopes via dispatch. + _ui.focus_list is [] + _build_focus_list of dlg + _ui.focus_index is 0 - 1 + _ui.focused_widget is null + _ui.focused_input is null define pop_modal() as: n is len of _ui.modal_stack @@ -488,6 +497,18 @@ define pop_modal() as: for i in range of (n - 1): append of [new_stack, _ui.modal_stack[i]] _ui.modal_stack is new_stack + # Restore focus to whatever is now on top (#631): the next modal down, or + # the app root once the stack is empty. edit_root is null until app_loop + # runs, so a headless push/pop just clears focus rather than erroring. + focus_root is _ui.edit_root + if (len of new_stack) > 0: + focus_root is new_stack[(len of new_stack) - 1] + _ui.focus_list is [] + if focus_root != null: + _build_focus_list of focus_root + _ui.focus_index is 0 - 1 + _ui.focused_widget is null + _ui.focused_input is null return top define show_menu(menu_widget, x, y) as: @@ -565,19 +586,21 @@ define handle_key(root, ev) as: # Event dispatch — registry-driven define dispatch(root, ev) as: + # A modal owns ALL input — pointer AND keyboard (#631). Rebind the root to + # the top of the stack BEFORE routing anything, so hit-testing, the + # registry walk, hover, drag AND keyboard focus navigation/activation all + # run against the dialog's subtree — nothing behind it can be reached. The + # keys previously returned above this rebind (mouse-only modality), which + # let a user Tab to a background widget and Enter-activate it through the + # dialog. This is also what lets a dialog host children (#575): they are + # dispatched to like any other widget. Escape still pops the modal — + # handle_key checks the stack directly, so it survives the rebind. + if (len of _ui.modal_stack) > 0: + root is _ui.modal_stack[(len of _ui.modal_stack) - 1] # Keys route through the same public handler app_loop uses (#563), so # dispatch is the single event door for mouse, wheel and keyboard. if ev.type == "keydown" or ev.type == "keyup": return handle_key of [root, ev] - # A modal owns the pointer: the top of the stack becomes the root for - # this event, so everything below — hit-testing, the registry walk, - # focus, hover, drag — runs against the dialog's subtree and nothing - # behind it can be reached. This is also what lets a dialog host - # children (#575): they are dispatched to like any other widget - # instead of through a special-case click handler that knew only - # about the dialog's own buttons. - if (len of _ui.modal_stack) > 0: - root is _ui.modal_stack[(len of _ui.modal_stack) - 1] mx is 0 my is 0 if ev.type == "mousemove" or ev.type == "mousedown" or ev.type == "mouseup": diff --git a/tests/test_ui.eigs b/tests/test_ui.eigs index 92ff623..c085fd6 100644 --- a/tests/test_ui.eigs +++ b/tests/test_ui.eigs @@ -1205,6 +1205,62 @@ remove_file of "/tmp/eigs_fd_probe/a.wav" remove_file of "/tmp/eigs_fd_probe/sub/inner.txt" _ui.modal_stack is [] +# ============================================================ +# #631: a modal owns the keyboard, not just the pointer +# ============================================================ +# Before the fix, dispatch returned keys with the real root above the modal +# rebind, so Tab escaped to a background widget and Enter activated it — an +# "Are you sure? [Cancel]" dialog could fire the destructive action behind it. +_ui.modal_stack is [] +_ui.focus_list is [] +_ui.focused_widget is null +_ui.focused_input is null +_ui.focus_index is 0 - 1 + +m631_root is panel of ["m631_root", 0, 0, 800, 600] +m631_fired is [0] +define m631_boom(b) as: + m631_fired[0] is m631_fired[0] + 1 +m631_bg is button of ["M631_BEHIND", 0, 0, 100, 30, "danger", m631_boom] +add_child of [m631_root, m631_bg] +_layout of [m631_root, 0, 0] +_ui.edit_root is m631_root +_build_focus_list of m631_root + +# A dialog hosting two focusable children of its own (#575). +m631_dlg is dialog of ["m631_dlg", 400, 300, "Modal", "", ["OK"], null] +m631_hit is [0] +define m631_ok(b) as: + m631_hit[0] is m631_hit[0] + 1 +m631_c1 is button of ["M631_C1", 0, 0, 60, 24, "One", m631_ok] +m631_c2 is button of ["M631_C2", 0, 0, 60, 24, "Two", m631_ok] +add_child of [m631_dlg, m631_c1] +add_child of [m631_dlg, m631_c2] +push_modal of [m631_dlg, 800, 600] + +# Tab scopes to the dialog, never the background. +dispatch of [m631_root, {"type": "keydown", "key": "tab", "shift": 0, "ctrl": 0, "alt": 0}] +assert_true of [_ui.focused_widget != null, "#631 Tab under a modal focuses a dialog child, not nothing"] +assert_eq of [_ui.focused_widget.id, "M631_C1", "#631 Tab lands on the dialog's first child"] + +# Enter fires the DIALOG child, never the button behind the modal. +dispatch of [m631_root, {"type": "keydown", "key": "return", "shift": 0, "ctrl": 0, "alt": 0}] +assert_eq of [m631_fired[0], 0, "#631 Enter does not fire the button behind the modal"] +assert_eq of [m631_hit[0], 1, "#631 Enter fires the focused dialog child"] + +# Tab again cycles within the dialog (C1 -> C2), still not the background. +dispatch of [m631_root, {"type": "keydown", "key": "tab", "shift": 0, "ctrl": 0, "alt": 0}] +assert_eq of [_ui.focused_widget.id, "M631_C2", "#631 Tab cycles between the dialog's own children"] + +# After pop, focus returns to the background list. +pop_modal of null +dispatch of [m631_root, {"type": "keydown", "key": "tab", "shift": 0, "ctrl": 0, "alt": 0}] +assert_eq of [_ui.focused_widget.id, "M631_BEHIND", "#631 after pop_modal, Tab returns to the background widget"] +assert_eq of [m631_fired[0], 0, "#631 the background button never fired via keyboard"] + +_ui.modal_stack is [] +_ui.edit_root is null + # ============================================================ # Summary # ============================================================