Skip to content

fix(settings): keep the settings modal open when a dropdown is dismissed - #1820

Open
greatcoat wants to merge 1 commit into
mainfrom
fix/settings-modal-dropdown-dismiss
Open

fix(settings): keep the settings modal open when a dropdown is dismissed#1820
greatcoat wants to merge 1 commit into
mainfrom
fix/settings-modal-dropdown-dismiss

Conversation

@greatcoat

Copy link
Copy Markdown
Contributor

Problem — Opening a dropdown in Settings and then clicking anywhere in the panel closes the entire settings modal, not just the dropdown.

Fix — A dropdown makes the settings panel inert, so the dismissing click lands on the modal's backdrop. Settings never had the guard our other dialogs already carry against that; this adds it.


Problem

Reported with a screen recording. Sampling it frame-by-frame, the settings modal closes unexpectedly at 0:09, 0:24.5, 0:31 and 0:47 — every time, a dropdown is open and the next click anywhere inside the panel takes the whole modal down.

The three dropdowns involved are all our shared Radix Select:

Dropdown Section Component
Input Device Preferences → Microphone MicrophoneSettings
Keep Microphone Warm Preferences → Microphone MicrophoneSettings
When triggered by hotkey, open in Hotkeys SettingsPage

The native <select>s in the same panel (Start position, Audio Retention, Transcript Retention) and the LanguageSelector pickers are opened in the same recording and do not reproduce it — which is what pointed at the layer machinery rather than at any one control.

Live probe against the real components in a browser (Playwright, script at the bottom):

modal open at start: true
select open: true
dialog content pointer-events while select open: none      <-- the panel is inert
clicking inside panel at 730 108
element at that point: DIV.fixed inset-0 z-50 bg-black/60   <-- the click hits the overlay
modal open after clicking inside panel: false               <-- BUG

Root cause

The dialog dismisses itself because the click it evaluates is, by then, indistinguishable from a click on its own backdrop:

  1. A Radix Select opening declares disableOutsidePointerEvents, so Radix sets pointer-events: none on the dialog's content — the settings panel goes inert (line 1 of the probe).
  2. The click therefore never reaches the panel. It falls through to the Overlay, a full-viewport fixed inset-0 scrim that Radix registers as the dialog's own dismiss affordance (line 2).
  3. DialogContentImpl passes deferPointerDownOutside: true (@radix-ui/react-dialog@1.1.18), so the dialog's outside-click decision is deferred to a one-time document click listener rather than settled at pointerdown.
  4. By the time that listener runs, the Select has closed and handed pointer events back. The gate if (!isPointerEventsEnabled …) return in @radix-ui/react-dismissable-layer no longer sees a layer above, so it lets the dismissal through and closes the modal.

The native <select>s and LanguageSelector are unaffected because neither is a Radix dismissable layer — the panel is never made inert, so their clicks land on real elements.

src/components/ui/dialog.tsx already carried a guard for this class (snapshot "was something above me" at pointerdown capture time). SidebarModal — the component Settings actually renders through — never got it.

Fix

Extracted that guard into src/components/ui/dismissGuard.ts as useDismissGuard, and applied it where it was missing.

The shared predicate keeps the two signals dialog.tsx already used (a mounted popper wrapper; a dialog stacked later in DOM order) and adds a third: the content's own pointer-events: none. That is the exact condition that misroutes the click, rather than a proxy for it, and it also covers layers that render without a popper wrapper. It can only ever suppress a dismissal that was already wrong — a modal dialog with nothing above it carries pointer-events: auto, never none.

Applied to:

  • SidebarModal — the settings modal, the reported bug.
  • dialog.tsx — behaviour unchanged, now sharing one implementation instead of an inline copy.
  • CommandSearch — the same latent defect: a raw Dialog.Content hosting a DropdownMenu. Not in the recording; flagged rather than left silently broken. Happy to split it out if you would rather keep this PR to Settings.

Deliberately untouched

  • The native <select>s (Start position, Audio/Transcript Retention) — not dismissable layers, verified unaffected in the recording.
  • LanguageSelector — portals into the closest [role="dialog"], so its clicks already count as inside. Unaffected.
  • ReferralModal — the third raw Dialog.Content, but it hosts no popper, so there is no bug to fix there.
  • Radix's deferred-dismissal behaviour itself — the guard sits at our call sites; no patching around the library.

Tests

test/components/dismissGuard.test.js — 6 cases over the extracted policy, written first and watched fail with the module absent (its state on main):

✖ nothing above the dialog is not a layer
✖ an inert content node means a layer above disabled its pointer events
✖ a mounted radix popper is a layer above
✖ a dialog opened later stacks above this one
✖ the topmost open dialog has nothing above it
✖ an unmounted content node reports no layer above
ℹ pass 0   ℹ fail 6

then green after the fix (pass 6, fail 0).

Symptom-level RED → GREEN on this branch, toggling only the SidebarModal wiring:

### RED — guard removed from SidebarModal
modal open after clicking inside panel: false
### GREEN — guard in place
modal open after clicking inside panel: true

Behaviour matrix, all green, confirming nothing else moved:

Scenario Result
Dropdown open → click empty space in panel modal stays open
Dropdown open → click a sidebar nav item (the 0:47 case) modal stays open, dropdown closes
Dropdown open → Escape closes dropdown only; second Escape closes modal
Dropdown open → click the real backdrop closes dropdown; next click closes modal
No dropdown open → click backdrop modal closes (unchanged)
Pick a dropdown item modal stays open (unchanged)

npm test: 2947 tests, 2758 pass, 16 fail, 172 skipped. The 16 failures are the pre-existing VoicePill suite failing on an uninstalled border-beam dependency — the identical 16 fail on a clean origin/main checkout, and tsc reports the same module as missing there too. npm run lint clean; npm run typecheck clean apart from those same pre-existing missing-dependency errors.

Browser probe used above (needs a Playwright install; not committed — the repo has no browser-test harness)

Add src/repro.html + src/repro.tsx mounting SidebarModal with one Select inside, run cd src && npx vite, then:

const page = await browser.newPage({ viewport: { width: 1280, height: 900 } });
await page.goto("http://127.0.0.1:5173/repro.html");
await page.click("#trigger");                                  // open the dropdown
await page.waitForSelector("[data-radix-popper-content-wrapper]");
const r = await page.$eval('[role="dialog"]', (el) => el.getBoundingClientRect().toJSON());
await page.mouse.click(r.x + r.width * 0.6, r.y + 40);         // click inside the panel
console.log(await page.$eval("#state", (el) => el.dataset.open)); // "false" before, "true" after

Provenance

Screen recording from Josh, 2026-08-25: https://cap.so/s/h79j2y5m2k4yvv9

Opening a Select inside the settings modal makes Radix set
`pointer-events: none` on the dialog content, so the click that dismisses
the dropdown falls through to the dialog's own overlay. Radix defers the
dialog's outside-click decision to a `click` listener that runs after the
Select has closed and handed pointer events back, so it no longer sees a
layer above and dismisses the whole modal.

SidebarModal never had the guard that ui/dialog.tsx already carried for
this. Extract that guard into a shared `useDismissGuard` hook, have it
also read the content's own inert state (the exact condition, rather than
a proxy for it), and apply it to SidebarModal and CommandSearch — the two
remaining raw Dialog.Content surfaces that host a popper.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants