Skip to content

Mode2UpLit: skip page-flip when a text selection is active#1540

Open
jbuckner wants to merge 1 commit intomasterfrom
fix/preserve-text-selection-on-page-click
Open

Mode2UpLit: skip page-flip when a text selection is active#1540
jbuckner wants to merge 1 commit intomasterfrom
fix/preserve-text-selection-on-page-click

Conversation

@jbuckner
Copy link
Copy Markdown
Contributor

Summary

Mode2UpLit.handlePageClick unconditionally flips the page on any left-click mouseup inside a .BRpagecontainer. When a user drags across text to select it, releasing the mouse at the end of the selection fires mouseup and triggers the page-flip, clobbering the selection.

Reproduction

  1. Open any text item with OCR in 2up mode (e.g. archive.org/details/goody).
  2. Drag across a few words to select them.
  3. Release the mouse anywhere except directly on a .BRwordElement or .BRspace — e.g. on blank space between words or on the page margin.
  4. The page flips and the selection is cleared.

Why the text_selection plugin doesn't catch it

The text_selection plugin attaches a mouseup.textSelectPluginHandler to the text layer that calls event.stopPropagation(), which works — but only when the mouseup target is inside the text layer. If the drag ends on blank space or the page image, the plugin's handler doesn't fire and the event bubbles up to .br-mode-2up__book's @mouseup=${handlePageClick}.

Fix

Check window.getSelection().toString().trim() at the top of handlePageClick and bail if it has content. A single early-return — no new event listeners, no timing dependencies.

handlePageClick = (ev) => {
  if (ev.which == 3 && this.br.protected) return false;
  if (ev.which != 1) return;
  if (window.getSelection?.()?.toString().trim()) return;  // ← new
  // …existing flip logic
}

Verification

Offshoot works around this in https://git.archive.org/www/offshoot/-/merge_requests/1049 by attaching a capture-phase mouseup listener on the BookReader root container that calls stopPropagation when a selection is active. Upstream-fixing it here lets offshoot drop that workaround entirely.

Manually verified on offshoot's review app: selection now survives the mouseup in every case — on-word, between-word, on-margin, crossing-page-boundary.

Risk

Low. The check fires only when there's an actual non-whitespace selection, which is unambiguously a selection-drag. No effect on tap-to-flip behavior (tap has no selection at mouseup time).

Mode2UpLit.handlePageClick is bound to mouseup on the 2up book
container and unconditionally flips the page when the mouseup target
is inside a .BRpagecontainer. Because mouseup also fires at the end
of a text-selection drag, releasing the mouse after selecting text
triggers the page-flip and clobbers the selection.

The text_selection plugin tries to prevent this by attaching its own
mouseup handler on the text layer that calls stopPropagation, but
only when the mouseup lands on a BRwordElement or BRspace. If the
drag ends on blank space between words or on the page margin, the
plugin's handler doesn't fire and the flip still runs.

Check `window.getSelection().toString().trim()` at the top of
handlePageClick and bail if it has content. Safe in all browsers —
the getSelection API is universally supported and reports the
currently active selection synchronously on mouseup.
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.97%. Comparing base (2908b1e) to head (eb609ba).

Files with missing lines Patch % Lines
src/BookReader/Mode2UpLit.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1540      +/-   ##
==========================================
- Coverage   68.98%   68.97%   -0.02%     
==========================================
  Files          65       65              
  Lines        5556     5557       +1     
  Branches     1229     1230       +1     
==========================================
  Hits         3833     3833              
- Misses       1689     1690       +1     
  Partials       34       34              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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