fix(ui): DIAGNOSTICS menu detach, focus loss on F-key, listener cleanup #182

New issue

Open

opened 2026-09-28 13:58:43 +00:00 by gabogg · 0 comments

gabogg commented

2026-09-28 13:58:43 +00:00

Owner

Follow-up from review pass 2 on #173 (deferred P3s, all in app/static/js/src/ui/deck_menu.js).

Findings

Menu detaches from its button when the button scrolls out of the strip. The capture-phase scroll listener (around line 108) repositions from getBoundingClientRect(). When the button is clipped by the deck selector's overflow-x, menuPlacement clamps left to 0 or the right viewport edge (around line 47). The menu stays open at the viewport edge, away from a button that is not visible. Also, the left clamp uses a margin of 0, while the right clamp uses VIEWPORT_MARGIN (8px).
An F-key pressed with the menu open loses keyboard focus. syncActive() calls close() without restoring focus (around line 167). If F4/F6/F7 is pressed while a menu item has focus, the item becomes display:none and focus falls to <body>.
Listeners are never removed. The document mousedown/keydown and window resize/scroll listeners (around lines 103–109) are added in the constructor and there is no destroy(). This is harmless today, since initDiagnosticsMenu creates only one menu, but it matters if DeckMenu is reused (e.g. for #170).

Acceptance criteria

While the menu is open, it closes once its button leaves the deck selector's visible area. The left and right viewport margins are the same.
A deck switch through syncActive() while focus is inside the menu returns focus to the DIAGNOSTICS button.
DeckMenu has a destroy() that removes every listener it added.
Each behaviour is covered in tests/frontend/test_deck_menu.test.js.

Follow-up from review pass 2 on #173 (deferred P3s, all in `app/static/js/src/ui/deck_menu.js`). ## Findings 1. **Menu detaches from its button when the button scrolls out of the strip.** The capture-phase scroll listener (around line 108) repositions from `getBoundingClientRect()`. When the button is clipped by the deck selector's `overflow-x`, `menuPlacement` clamps `left` to 0 or the right viewport edge (around line 47). The menu stays open at the viewport edge, away from a button that is not visible. Also, the left clamp uses a margin of 0, while the right clamp uses `VIEWPORT_MARGIN` (8px). 2. **An F-key pressed with the menu open loses keyboard focus.** `syncActive()` calls `close()` without restoring focus (around line 167). If F4/F6/F7 is pressed while a menu item has focus, the item becomes `display:none` and focus falls to `<body>`. 3. **Listeners are never removed.** The document mousedown/keydown and window resize/scroll listeners (around lines 103–109) are added in the constructor and there is no `destroy()`. This is harmless today, since `initDiagnosticsMenu` creates only one menu, but it matters if `DeckMenu` is reused (e.g. for #170). ## Acceptance criteria - [ ] While the menu is open, it closes once its button leaves the deck selector's visible area. The left and right viewport margins are the same. - [ ] A deck switch through `syncActive()` while focus is inside the menu returns focus to the DIAGNOSTICS button. - [ ] `DeckMenu` has a `destroy()` that removes every listener it added. - [ ] Each behaviour is covered in `tests/frontend/test_deck_menu.test.js`.

gabogg added this to the Historical passenger-flow backfill milestone

2026-09-28 13:58:43 +00:00

gabogg added the

ready-for-agent

label

2026-09-28 13:58:43 +00:00

gabogg referenced this issue

2026-09-28 14:08:49 +00:00

refactor(ui): fold diagnostic decks into one DIAGNOSTICS menu #173

gabogg removed this from the Historical passenger-flow backfill milestone

2026-10-03 12:13:25 +00:00

gabogg referenced this issue