[P2] autoFocus does nothing in always-mounted modal dialogs, so the confirmations open with the destructive Accept button focused and Save as opens with Cancel focused instead of the name field #113

New issue

Open

opened 2026-10-31 19:47:41 +00:00 by pedro · 0 comments

pedro commented

2026-10-31 19:47:41 +00:00

Owner

Priority: P2
Type: bug
Source: full codebase review requested 2026-10-31 (independently reproduced against 2cac7cf)

Location: src/App.tsx:213-232, src/App.tsx:3012-3018, src/App.tsx:3036-3045, src/App.tsx:3063-3073, src/planner/SavePlanDialog.tsx:50-56, src/planner/SavePlanDialog.tsx:109-112

Problem and evidence

The Reset to Defaults, model-change and gas-replacement <dialog>s are always mounted and start closed. Each is opened with showModal() from an effect. Each one puts autoFocus on its Reject button. On the client, React does not write an autofocus attribute. It only calls focus() once, when the element mounts, and at that point the dialog is closed (display:none), so the call does nothing. When showModal() later runs the dialog focusing steps, it finds no [autofocus] element and focuses the first focusable descendant. In all three dialogs that is the Accept button, because it comes before Reject in the DOM. One Space or Enter press then confirms the destructive action. Space is the app's documented play/pause key, which makes an accidental press likely. SavePlanDialog has the same problem: PlanLibrary always mounts it, so its autoFocus file-name input is never focused, and focus lands on the Cancel button in the heading.

I ran Playwright Chromium against the HEAD production build in /tmp/divesim-review-dist-SqbiNx on a temporary port. The server has since been stopped.

document.querySelectorAll('[autofocus]').length returned 0.
Reset to Defaults: I dismissed the welcome dialog with Escape, set Units to metric and speed to 128x, picked a depth target, started the dive, waited 2.5 s and paused. Dive time was 5:44 and units were metric. I clicked 'Reset to Defaults'. document.activeElement was BUTTON "Accept · reset to defaults". I pressed Space. The dialog closed, dive time was 0:00 and units were back to imperial, so the reset ran.
Model change: I opened Live decompression model and chose 'Select Bühlmann ZHL-16B'. The 'Reset current dive?' dialog opened with BUTTON "Accept · reset dive and switch model" focused.
Save as: I opened Planner Mode, then 'Save, import & share', then Export JSON. Focus was on BUTTON "Cancel". Typing 'abc' left the Export file name as 'My dive.json'.
Gas replacement: I did not exercise this one in the browser. It uses the same pattern: an effect calls showModal(), and the Accept button comes before the autoFocus Reject button in the DOM (App.tsx:3063-3073), so the same result follows.

Suggested correction

In each open effect, call showModal() and then focus the intended element through a ref. For example, if (open && !dialog.open) { dialog.showModal(); rejectRef.current?.focus(); } in the reset, model and gas effects. In SavePlanDialog, call inputRef.current?.focus() (optionally .select()) after showModal().

Other options:

Set the real attribute with a ref callback, e.g. ref={el => el?.setAttribute('autofocus','')}, so the native dialog focusing steps pick it up.
Render the dialog contents only while open.

Optionally also apply the ref fix to ModelSelector's Close button for consistency.

Acceptance criteria

After showModal(), document.activeElement should be:

Reject in the Reset to Defaults, model-change and gas-replacement confirmations.
The 'Export file name' input in Save as, so typing changes the file name.

Pressing Space or Enter right after opening a confirmation must leave the dive and settings unchanged: dive time and units are preserved and the dialog closes as rejected.

Regression test: a component or Playwright test that opens each dialog and asserts the focused element. For example, for Reset to Defaults: start a dive, click Reset to Defaults, press Space, then assert that dive time is not 0:00 and units are unchanged. pnpm verify passes.

Scope: maintenance of the educational simulator. Preserve historical R1 acceptance and operational-validation limits.

Priority: P2 Type: bug Source: full codebase review requested 2026-10-31 (independently reproduced against `2cac7cf`) Location: `src/App.tsx:213-232`, `src/App.tsx:3012-3018`, `src/App.tsx:3036-3045`, `src/App.tsx:3063-3073`, `src/planner/SavePlanDialog.tsx:50-56`, `src/planner/SavePlanDialog.tsx:109-112` Problem and evidence The Reset to Defaults, model-change and gas-replacement `<dialog>`s are always mounted and start closed. Each is opened with `showModal()` from an effect. Each one puts `autoFocus` on its Reject button. On the client, React does not write an `autofocus` attribute. It only calls `focus()` once, when the element mounts, and at that point the dialog is closed (display:none), so the call does nothing. When `showModal()` later runs the dialog focusing steps, it finds no `[autofocus]` element and focuses the first focusable descendant. In all three dialogs that is the Accept button, because it comes before Reject in the DOM. One Space or Enter press then confirms the destructive action. Space is the app's documented play/pause key, which makes an accidental press likely. SavePlanDialog has the same problem: PlanLibrary always mounts it, so its `autoFocus` file-name input is never focused, and focus lands on the Cancel button in the heading. I ran Playwright Chromium against the HEAD production build in /tmp/divesim-review-dist-SqbiNx on a temporary port. The server has since been stopped. - `document.querySelectorAll('[autofocus]').length` returned 0. - **Reset to Defaults:** I dismissed the welcome dialog with Escape, set Units to metric and speed to 128x, picked a depth target, started the dive, waited 2.5 s and paused. Dive time was 5:44 and units were metric. I clicked 'Reset to Defaults'. `document.activeElement` was `BUTTON "Accept · reset to defaults"`. I pressed Space. The dialog closed, dive time was 0:00 and units were back to imperial, so the reset ran. - **Model change:** I opened Live decompression model and chose 'Select Bühlmann ZHL-16B'. The 'Reset current dive?' dialog opened with `BUTTON "Accept · reset dive and switch model"` focused. - **Save as:** I opened Planner Mode, then 'Save, import & share', then Export JSON. Focus was on `BUTTON "Cancel"`. Typing 'abc' left the Export file name as 'My dive.json'. - **Gas replacement:** I did not exercise this one in the browser. It uses the same pattern: an effect calls `showModal()`, and the Accept button comes before the autoFocus Reject button in the DOM (App.tsx:3063-3073), so the same result follows. Suggested correction In each open effect, call `showModal()` and then focus the intended element through a ref. For example, `if (open && !dialog.open) { dialog.showModal(); rejectRef.current?.focus(); }` in the reset, model and gas effects. In SavePlanDialog, call `inputRef.current?.focus()` (optionally `.select()`) after `showModal()`. Other options: - Set the real attribute with a ref callback, e.g. `ref={el => el?.setAttribute('autofocus','')}`, so the native dialog focusing steps pick it up. - Render the dialog contents only while open. Optionally also apply the ref fix to ModelSelector's Close button for consistency. Acceptance criteria After `showModal()`, `document.activeElement` should be: - Reject in the Reset to Defaults, model-change and gas-replacement confirmations. - The 'Export file name' input in Save as, so typing changes the file name. Pressing Space or Enter right after opening a confirmation must leave the dive and settings unchanged: dive time and units are preserved and the dialog closes as rejected. Regression test: a component or Playwright test that opens each dialog and asserts the focused element. For example, for Reset to Defaults: start a dive, click Reset to Defaults, press Space, then assert that dive time is not 0:00 and units are unchanged. pnpm verify passes. Scope: maintenance of the educational simulator. Preserve historical R1 acceptance and operational-validation limits.

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: pedro/DiveSim#113

No description provided.

Rows
Columns