Skip to content

fix(viewer): show Add-comment popover for selections released outside the article#577

Merged
yumike merged 1 commit into
mainfrom
fix/add-comment-popover-outside-article
Jun 24, 2026
Merged

fix(viewer): show Add-comment popover for selections released outside the article#577
yumike merged 1 commit into
mainfrom
fix/add-comment-popover-outside-article

Conversation

@yumike

@yumike yumike commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Problem

Selecting a line's first word by dragging right-to-left releases the mouse to the left of <article>, so the article-scoped onmouseup never fired and the "Add comment" popover never appeared. The same happened for any selection released past the article's right edge or below it.

The direction-dependence is the tell: Range.getBoundingClientRect() is direction-independent, so a positioning bug would break both directions — only the mouse-release location differs, which is the event-delivery problem.

Fix

Move selection capture into useSelectionPopover, which already owns the document-level selectionchange dismiss listener:

  • Adds a sibling document-level mouseup listener (gated on comments.enabled, capture-only, cleaned up on teardown) that captures a non-collapsed selection whenever it lies inside the article — so a release anywhere is caught.
  • Removes the public capture() method (single owner now); the article handler keeps only the collapsed-click → highlight toggle.
  • The Add-comment button preventDefaults its mousedown to keep the live selection/focus intact on click.

Edge cases hardened in review

  • A selection extended past the article boundary (which never collapses) now drops the popover instead of stranding it at the old anchor.
  • Disabling comments clears any captured selection, so it can't re-surface stale if comments are re-enabled mid-session.

Testing

  • New hook unit tests (jsdom) covering capture-on-document-mouseup, in-article containment, gating/listener-detach, dismiss, null-on-outside, and clear().
  • New real-mouse Playwright e2e that drags right-to-left releasing in the gutter, asserts the popover appears, then real-clicks it to open the draft. (The existing synthetic selectText helper dispatches a mouseup on <article> and can't reproduce this.)
  • Full viewer unit suite (598) green; svelte-check and eslint clean; existing comment e2e unaffected.

🤖 Generated with Claude Code

… the article

Selecting a line's first word by dragging right-to-left releases the mouse to the
left of the <article>, so the article-scoped `onmouseup` never fired and the
"Add comment" popover never appeared. The same applied to any selection released
past the article's right edge or below it.

Move selection capture into `useSelectionPopover`, which already owns the
document-level `selectionchange` dismiss listener. It now adds a sibling
document-level `mouseup` listener (gated on `comments.enabled`, capture-only)
that captures a non-collapsed selection whenever it lies inside the article, so a
release anywhere is caught. The public `capture()` method is removed; the article
handler keeps only the collapsed-click highlight toggle; and the Add-comment
button preventDefaults its `mousedown` to keep the live selection intact on click.

Edge cases hardened during review: a selection extended past the article boundary
(which never collapses) now drops the popover instead of stranding it at the old
anchor, and disabling comments clears any captured selection so it can't
re-surface stale if comments are re-enabled mid-session.

Tested with new hook unit tests (jsdom) and a real-mouse Playwright e2e that
drags right-to-left releasing in the gutter and then clicks the popover.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yumike yumike merged commit b9046d3 into main Jun 24, 2026
18 checks passed
@yumike yumike deleted the fix/add-comment-popover-outside-article branch June 24, 2026 03:28
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