Skip to content

fix: don't close dialog when drag-selecting text out of modal - #247

Merged
diegoimbert merged 1 commit into
mainfrom
fix/dialog-close-on-drag-select
May 20, 2026
Merged

fix: don't close dialog when drag-selecting text out of modal#247
diegoimbert merged 1 commit into
mainfrom
fix/dialog-close-on-drag-select

Conversation

@diegoimbert

Copy link
Copy Markdown
Contributor

Summary

The "New worktree" modal (and any other BaseDialog consumer) closes when the user mousedowns inside a textarea/input, drags the pointer outside the dialog, and releases — a common gesture when selecting prompt text with the mouse. The browser fires a synthetic click on the <dialog> element (the common ancestor of mousedown/mouseup), which the existing handler interprets as a backdrop click.

Changes

  • BaseDialog.svelte: track an onmousedown flag indicating whether the press originated on the backdrop. The onclick handler now only closes the dialog when both the click target and the original mousedown target are the dialog element itself.

Test plan

  • Open "New worktree", click+drag inside the prompt textarea, release outside the modal — modal stays open and text remains selected.
  • Click on the backdrop (mousedown and mouseup both on the dimmed area outside the modal) — modal closes as before.
  • Click the Cancel button — modal closes.
  • Press Escape — modal closes.

Generated with Claude Code

Track where the press started — only close on backdrop click if the
mousedown also originated on the backdrop. Previously, selecting text
inside a textarea and releasing the mouse outside the dialog would fire
a click on the dialog element and dismiss the modal.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented May 20, 2026

Copy link
Copy Markdown

Claude finished @diegoimbert's task in 55s —— View job


Code Review — PR #247

  • Read PR diff and BaseDialog.svelte
  • Analyze fix correctness and edge cases
  • Post review

Verdict: Looks good. The fix correctly addresses the bug and is appropriately minimal. BaseDialog is consumed by 10+ dialogs, so this is a worthwhile fix.

Why the fix is correct

The browser fires a synthetic click on the nearest common ancestor of mousedown/mouseup. When drag-selecting out of a textarea, that ancestor is <dialog>, so the old e.target === dialogEl check produced a false positive. Gating on the origin of the press (pressStartedOnBackdrop) is the right discriminator. I walked the relevant gestures and they all behave correctly:

Gesture Result
Drag-select from textarea → release outside stays open ✅ (the bug being fixed)
mousedown + mouseup on backdrop closes ✅
mousedown on backdrop → drag into content → release stays open ✅ (target check fails)
Cancel button / Escape unaffected ✅

The flag can never go stale across gestures: every click event is preceded by its paired mousedown, which always refreshes pressStartedOnBackdrop before onclick reads it. Touch devices are also covered — browsers synthesize mousedown/mouseup/click after taps, so tap-to-close on the backdrop still works.

Minor notes (non-blocking)

  1. pressStartedOnBackdrop = false in onclick (line 34) is dead code. Since every click is always preceded by a mousedown that sets the flag, the reset has no observable effect. It's harmless and arguably defensive, so feel free to keep it — but it could be dropped for clarity.

  2. Plain let vs $statepressStartedOnBackdrop is a non-reactive local read only inside event handlers, so a plain let is correct here (and consistent with dialogEl). No change needed; just noting it's an intentional non-rune.

  3. No regression test. frontend/src/lib/ already has WorktreeLabelDialog.test.ts, DiffDialog.test.ts, SettingsDialog.test.ts. A small test that dispatches mousedown on content + click on the dialog and asserts it stays open would lock in this fix. Optional given the size of the change.

Style / TS

Event handlers are explicitly typed as MouseEvent, matching frontend/CLAUDE.md. No any, no casts. 👍

Nice, well-scoped fix.

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
webmux 92a611a Commit Preview URL

Branch Preview URL
May 20 2026, 09:41 AM

@diegoimbert
diegoimbert marked this pull request as ready for review May 20, 2026 10:04
@claude

claude Bot commented May 20, 2026

Copy link
Copy Markdown

Claude finished @diegoimbert's task in 1m 16s —— View job


Code Review — PR #247

  • Read PR diff and BaseDialog.svelte
  • Analyze fix correctness and edge cases
  • Post review

Verdict: Looks good — correct, minimal, and safe to merge. BaseDialog backs 10 dialogs (CreateWorktreeDialog, SettingsDialog, DiffDialog, etc.), so fixing this one place is the right call.

Why the fix is correct

A click event is dispatched at the nearest common ancestor of the mousedown and mouseup targets. Drag-selecting out of a textarea makes that ancestor the <dialog> element, so the old e.target === dialogEl check false-positived as a backdrop click. Gating on where the press originated (pressStartedOnBackdrop) is the correct discriminator.

Gesture Result
Drag-select from textarea → release outside modal stays open ✅ (the bug being fixed)
mousedown + mouseup on backdrop closes ✅
Cancel button / Escape unaffected ✅

No staleness risk: every click is preceded by its paired mousedown, which refreshes the flag before onclick reads it. Taps on touch devices also work — browsers synthesize mousedown/mouseup/click after a tap.

Notes (all non-blocking)

  1. pressStartedOnBackdrop = false on line 34 is effectively dead code. Because every click is always preceded by a mousedown that sets the flag, this reset is never observable. Harmless and arguably defensive — keep it or drop it, your call.

  2. Edge case — mousedown on backdrop → drag into content → release inside: this still closes the dialog (the click target is <dialog> as the common ancestor, and the flag is true). This matches the pre-PR behavior exactly, so it's not a regression, and pressing on the dimmed backdrop is a dismiss intent anyway. Just flagging it for awareness — no action needed.

  3. Plain let for pressStartedOnBackdrop is correct. It's only read inside event handlers, never in the template or a reactive context, so it doesn't need $state — and frontend/CLAUDE.md's "runes exclusively" rule targets reactive state, which this isn't. Consistent with the existing dialogEl declaration.

  4. No regression test. frontend/src/lib/ already has WorktreeLabelDialog.test.ts, DiffDialog.test.ts, and SettingsDialog.test.ts. A test that dispatches mousedown on content then click on the dialog and asserts it stays open would lock this in. Optional given the 6-line change. Add test →

Style / TS

Event handlers explicitly typed as MouseEvent, matching frontend/CLAUDE.md. No any, no casts. Well-scoped fix. 👍
· Branch preview

@diegoimbert
diegoimbert merged commit dc41a31 into main May 20, 2026
4 checks passed
@diegoimbert
diegoimbert deleted the fix/dialog-close-on-drag-select branch May 20, 2026 15:29
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.

1 participant