From 29fc3a11b89090c5a811f1f09aa617d2e6413251 Mon Sep 17 00:00:00 2001 From: Chai Date: Mon, 24 Aug 2026 12:56:02 -0400 Subject: [PATCH] fix(reader): keep text selection on the current page --- packages/app-expo/assets/reader/reader.html | 188 +++++++++--------- .../reader/paginator-touch-navigation.test.js | 138 +++++++++++++ packages/foliate-js/paginator-touch.js | 93 +++++++++ packages/foliate-js/paginator.js | 89 ++++++--- 4 files changed, 381 insertions(+), 127 deletions(-) create mode 100644 packages/app-expo/src/lib/reader/paginator-touch-navigation.test.js create mode 100644 packages/foliate-js/paginator-touch.js diff --git a/packages/app-expo/assets/reader/reader.html b/packages/app-expo/assets/reader/reader.html index 03d2d186a..0cd08c935 100644 --- a/packages/app-expo/assets/reader/reader.html +++ b/packages/app-expo/assets/reader/reader.html @@ -343,17 +343,29 @@ } } - function getMobileReflowMaxInlineSize() { - const viewportHeight = window.innerHeight || document.documentElement.clientHeight || 720; - const viewportWidth = window.innerWidth || document.documentElement.clientWidth || 393; - return Math.ceil(Math.max(720, viewportHeight, viewportWidth)); - } - function applyMobileReflowRendererSizing(renderer) { if (!renderer) return; renderer.setAttribute('max-column-count', '1'); renderer.setAttribute('max-block-size', '1440px'); - renderer.setAttribute('max-inline-size', getMobileReflowMaxInlineSize() + 'px'); + } + + // Mobile is always single-page (no spread), so there is no column gutter to + // balance — the foliate gap only contributes the page's internal side + // padding. Same derivation as the desktop reader: paper = 99% of the window + // (breathing room even at margin 0), outer text inset = margin + 0.5%·H. + // All against renderer.clientWidth (foliate resolves the gap % against it). + function applyMobileReflowMargins(renderer, margin) { + if (!renderer) return; + const windowWidth = + renderer.clientWidth || window.innerWidth || document.documentElement.clientWidth || 393; + const paper = windowWidth * 0.99; + const g = margin / paper; + renderer.setAttribute('margin-top', margin + 'px'); + renderer.setAttribute('margin-bottom', margin + 'px'); + renderer.setAttribute('margin-left', '0px'); + renderer.setAttribute('margin-right', '0px'); + renderer.setAttribute('gap', g * 100 + '%'); + renderer.setAttribute('max-inline-size', paper - margin + g * windowWidth + 'px'); } function applyRendererFlowMode(renderer) { @@ -1781,11 +1793,8 @@ const renderer = el.renderer; if (renderer && !el.isFixedLayout) { applyMobileReflowRendererSizing(renderer); - // Default gap/margin - const margin = msg.pageMargin || 16; - const gapPercent = Math.max(1, Math.round((margin / 393) * 100)); - renderer.setAttribute('gap', gapPercent + '%'); - renderer.setAttribute('margin', margin + 'px'); + // Default margins (synchronized with the desktop reader formula) + applyMobileReflowMargins(renderer, msg.pageMargin || 16); } else if (renderer && el.isFixedLayout) { const paginatedLayout = msg.paginatedLayout === 'single' || msg.paginatedLayout === 'double' @@ -2047,9 +2056,7 @@ const BASELINE_FONT_SIZE = 16; const layoutScale = currentFontSize / BASELINE_FONT_SIZE; const margin = Math.round(settings.pageMargin * layoutScale); - const gapPercent = Math.max(1, Math.round((margin / 393) * 100)); - renderer.setAttribute('gap', gapPercent + '%'); - renderer.setAttribute('margin', margin + 'px'); + applyMobileReflowMargins(renderer, margin); } if (view && view.isFixedLayout) { @@ -5045,8 +5052,8 @@ diff --git a/packages/app-expo/src/lib/reader/paginator-touch-navigation.test.js b/packages/app-expo/src/lib/reader/paginator-touch-navigation.test.js new file mode 100644 index 000000000..49aae9e55 --- /dev/null +++ b/packages/app-expo/src/lib/reader/paginator-touch-navigation.test.js @@ -0,0 +1,138 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; +import { + PaginatorTouchTracker, + SelectionPositionGuard, + hasActiveTextSelection, +} from "../../../../foliate-js/paginator-touch.js"; + +const touch = { screenX: 430, screenY: 520 }; + +describe("paginator touch navigation ownership", () => { + it("restores the gesture start after movement becomes text selection", () => { + const tracker = new PaginatorTouchTracker(); + tracker.start(touch, 100, 624); + tracker.markScrolled(); + + expect( + hasActiveTextSelection([ + { + doc: { + getSelection: () => ({ isCollapsed: false, toString: () => "selected text" }), + }, + }, + ]), + ).toBe(true); + expect(tracker.cancel()).toBe(624); + expect(tracker.state).toBeUndefined(); + expect(tracker.scrolled).toBe(false); + }); + + it("does not request restoration before the paginator moves", () => { + const tracker = new PaginatorTouchTracker(); + tracker.start(touch, 100, 624); + + expect(tracker.cancel()).toBeNull(); + }); + + it("retains the aligned touch start when selection takes ownership before swipe movement", () => { + const tracker = new PaginatorTouchTracker(); + tracker.start(touch, 100, 1080); + + expect(tracker.takeSelectionStart(1410)).toBe(1080); + expect(tracker.state).toBeUndefined(); + expect(tracker.scrolled).toBe(false); + }); + + it("falls back to the current position when selection starts without tracked touch state", () => { + expect(new PaginatorTouchTracker().takeSelectionStart(1080)).toBe(1080); + }); + + it("returns the completed gesture state, including scroll inertia samples", () => { + const tracker = new PaginatorTouchTracker(); + const state = tracker.start(touch, 100, 624); + state.dx = 180; + state.dt = 300; + state.scrollSamples.push({ velocity: 0.25, time: 250 }); + tracker.markScrolled(); + + expect(tracker.finish()).toMatchObject({ + dx: 180, + dt: 300, + startPosition: 624, + scrollSamples: [{ velocity: 0.25, time: 250 }], + }); + expect(tracker.state).toBeUndefined(); + expect(tracker.scrolled).toBe(false); + }); + + it("ignores collapsed and whitespace-only selections", () => { + expect( + hasActiveTextSelection([ + { doc: { getSelection: () => ({ isCollapsed: true, toString: () => "text" }) } }, + { doc: { getSelection: () => ({ isCollapsed: false, toString: () => " " }) } }, + ]), + ).toBe(false); + }); +}); + +describe("native selection position ownership", () => { + it("restores unowned native selection drift but ignores sub-pixel noise", () => { + const guard = new SelectionPositionGuard(); + guard.begin(1080); + guard.begin(1410); + + expect(guard.correctionFor(1410)).toBe(1080); + expect(guard.correctionFor(1080.4)).toBeNull(); + }); + + it("allows explicit edge navigation and rebases protection afterward", () => { + const guard = new SelectionPositionGuard(); + guard.begin(1080); + guard.beginNavigation(); + + expect(guard.correctionFor(2160)).toBeNull(); + guard.finishNavigation(2160); + expect(guard.correctionFor(2450)).toBe(2160); + }); + + it("releases position ownership when selection ends", () => { + const guard = new SelectionPositionGuard(); + guard.begin(1080); + guard.end(); + + expect(guard.active).toBe(false); + expect(guard.correctionFor(1410)).toBeNull(); + }); +}); + +const paginatorSource = readFileSync( + new URL("../../../../foliate-js/paginator.js", import.meta.url), + "utf8", +); + +describe("paginator selection cancellation wiring", () => { + it("guards native selection drift while preserving explicit edge navigation and inertia", () => { + expect(paginatorSource).toContain( + "doc.addEventListener('selectstart', () => this.#beginTextSelection())", + ); + expect( + paginatorSource.match( + /if \(this\.#hasActiveTextSelection\(\)\) \{\s*this\.#beginTextSelection\(\)\s*return\s*\}/g, + ), + ).toHaveLength(3); + expect(paginatorSource).toMatch( + /this\.#container\.addEventListener\('scroll', \(\) => \{\s*const restorePosition = this\.#selectionPosition\.correctionFor\(this\.containerPosition\)/, + ); + expect(paginatorSource).toMatch( + /#beginTextSelection\(\) \{\s*if \(this\.scrolled\) \{\s*this\.#cancelTouchNavigation\(\)\s*return/, + ); + expect(paginatorSource).toContain("this.#selectionPosition.end()"); + expect(paginatorSource).toMatch( + /this\.#selectionPosition\.beginNavigation\(\)\s*try \{\s*if \(direction === 'backward'\) await this\.prev\(\)\s*else await this\.next\(\)[\s\S]*?finally \{\s*this\.#selectionPosition\.finishNavigation\(this\.containerPosition\)/, + ); + expect(paginatorSource).toContain( + "if (!this.#navigationLocked) this.#startScrollInertia(state)", + ); + }); +}); diff --git a/packages/foliate-js/paginator-touch.js b/packages/foliate-js/paginator-touch.js new file mode 100644 index 000000000..a55d354db --- /dev/null +++ b/packages/foliate-js/paginator-touch.js @@ -0,0 +1,93 @@ +export const hasActiveTextSelection = (contents) => { + for (const { doc } of contents ?? []) { + const selection = doc?.getSelection?.(); + if (selection && !selection.isCollapsed && selection.toString().trim()) return true; + } + return false; +}; + +export class PaginatorTouchTracker { + state; + scrolled = false; + + start(touch, timeStamp, startPosition) { + this.state = { + x: touch?.screenX, + y: touch?.screenY, + t: timeStamp, + vx: 0, + xy: 0, + dx: 0, + dy: 0, + dt: 0, + scrollVelocity: 0, + scrollSamples: [], + startX: touch?.screenX, + startY: touch?.screenY, + startPosition, + didPreventDefault: false, + }; + this.scrolled = false; + return this.state; + } + + markScrolled() { + this.scrolled = true; + } + + cancel() { + const restorePosition = + this.scrolled && Number.isFinite(this.state?.startPosition) ? this.state.startPosition : null; + this.state = undefined; + this.scrolled = false; + return restorePosition; + } + + takeSelectionStart(currentPosition) { + const position = Number.isFinite(this.state?.startPosition) + ? this.state.startPosition + : currentPosition; + this.state = undefined; + this.scrolled = false; + return position; + } + + finish() { + const state = this.scrolled ? (this.state ?? null) : null; + this.state = undefined; + this.scrolled = false; + return state; + } +} + +export class SelectionPositionGuard { + position; + navigating = false; + + get active() { + return Number.isFinite(this.position); + } + + begin(position) { + if (!this.active && Number.isFinite(position)) this.position = position; + } + + correctionFor(currentPosition) { + if (!this.active || this.navigating || !Number.isFinite(currentPosition)) return null; + return Math.abs(currentPosition - this.position) > 0.5 ? this.position : null; + } + + beginNavigation() { + if (this.active) this.navigating = true; + } + + finishNavigation(position) { + if (this.active && Number.isFinite(position)) this.position = position; + this.navigating = false; + } + + end() { + this.position = undefined; + this.navigating = false; + } +} diff --git a/packages/foliate-js/paginator.js b/packages/foliate-js/paginator.js index 00008efab..46272e295 100644 --- a/packages/foliate-js/paginator.js +++ b/packages/foliate-js/paginator.js @@ -1,3 +1,9 @@ +import { + PaginatorTouchTracker, + SelectionPositionGuard, + hasActiveTextSelection, +} from './paginator-touch.js' + const wait = ms => new Promise(resolve => setTimeout(resolve, ms)) const debounce = (f, wait, immediate) => { @@ -974,8 +980,8 @@ export class Paginator extends HTMLElement { #mediaQuery = matchMedia('(prefers-color-scheme: dark)') #mediaQueryListener #scrollBounds - #touchState - #touchScrolled + #touchNavigation = new PaginatorTouchTracker() + #selectionPosition = new SelectionPositionGuard() #scrollInertiaFrame = null #scrollInertiaToken = 0 #visibilityHandler = () => { @@ -1175,6 +1181,11 @@ export class Paginator extends HTMLElement { } }, 250) this.#container.addEventListener('scroll', () => { + const restorePosition = this.#selectionPosition.correctionFor(this.containerPosition) + if (restorePosition !== null) { + this.containerPosition = restorePosition + return + } if (!this.#isAnimating) this.dispatchEvent(new Event('scroll')) // Keep the per-view backgrounds glued to the content while a swipe // drag scrolls the container (no animation runs then). During the @@ -1243,6 +1254,7 @@ export class Paginator extends HTMLElement { doc.addEventListener('touchmove', this.#onTouchMove.bind(this), opts) doc.addEventListener('touchend', this.#onTouchEnd.bind(this)) doc.addEventListener('touchcancel', this.#onTouchCancel.bind(this)) + doc.addEventListener('selectstart', () => this.#beginTextSelection()) }) this.addEventListener('relocate', ({ detail }) => { @@ -1459,6 +1471,7 @@ export class Paginator extends HTMLElement { debugSelectionPaging('edge-hold-fire', holdDetail(hold, performance.now(), { reason: 'hold-timeout', })) + this.#selectionPosition.beginNavigation() try { if (direction === 'backward') await this.prev() else await this.next() @@ -1467,6 +1480,7 @@ export class Paginator extends HTMLElement { error: String(error), })) } finally { + this.#selectionPosition.finishNavigation(this.containerPosition) setSelectionNavigationLock(false) } }, SELECTION_EDGE_HOLD_MS) @@ -1566,6 +1580,9 @@ export class Paginator extends HTMLElement { doc.addEventListener('keydown', () => isKeyboardSelecting = true) doc.addEventListener('keyup', () => isKeyboardSelecting = false) doc.addEventListener('selectionchange', () => { + const sel = doc.getSelection() + if (!sel.rangeCount || sel.isCollapsed || sel.type !== 'Range') + this.#selectionPosition.end() if (this.scrolled) { debugSelectionPaging('selectionchange-skip', { reason: 'scrolled' }) return @@ -1575,7 +1592,6 @@ export class Paginator extends HTMLElement { debugSelectionPaging('selectionchange-skip', { reason: 'no-last-visible-range' }) return } - const sel = doc.getSelection() if (!sel.rangeCount) { cancelSelectionEdgeHold('selection-cleared') debugSelectionPaging('selectionchange-skip', { reason: 'no-range-count', type: sel.type }) @@ -2148,27 +2164,32 @@ export class Paginator extends HTMLElement { if (dir) return doGoTo() this.#scrollToPage(page, 'snap') } + #hasActiveTextSelection() { + return hasActiveTextSelection(this.getContents?.() ?? []) + } + #cancelTouchNavigation() { + const restorePosition = this.#touchNavigation.cancel() + if (restorePosition !== null) this.containerPosition = restorePosition + } + #beginTextSelection() { + if (this.scrolled) { + this.#cancelTouchNavigation() + return + } + const startPosition = this.#touchNavigation.takeSelectionStart(this.containerPosition) + this.#selectionPosition.begin(startPosition) + const restorePosition = this.#selectionPosition.correctionFor(this.containerPosition) + if (restorePosition !== null) this.containerPosition = restorePosition + } #onTouchStart(e) { this.#cancelScrollInertia() if (this.#navigationLocked) return - const contents = this.getContents?.() ?? [] - for (const { doc } of contents) { - const selection = doc?.getSelection?.() - if (selection && !selection.isCollapsed && selection.toString().trim()) return + if (this.#hasActiveTextSelection()) { + this.#beginTextSelection() + return } const touch = e.changedTouches[0] - this.#touchState = { - x: touch?.screenX, y: touch?.screenY, - t: e.timeStamp, - vx: 0, xy: 0, - dx: 0, dy: 0, - dt: 0, - scrollVelocity: 0, - scrollSamples: [], - startX: touch?.screenX, - startY: touch?.screenY, - didPreventDefault: false, - } + this.#touchNavigation.start(touch, e.timeStamp, this.containerPosition) // Hint to browser that scrolling will occur for better GPU layer management const pv = this.#primaryView if (pv?.element) { @@ -2209,7 +2230,7 @@ export class Paginator extends HTMLElement { if (state.scrollSamples.length > 5) state.scrollSamples.shift() e.preventDefault() - this.#touchScrolled = true + this.#touchNavigation.markScrolled() const previous = this.containerPosition this.containerPosition = previous + delta @@ -2222,12 +2243,11 @@ export class Paginator extends HTMLElement { } } #onTouchMove(e) { - const state = this.#touchState + const state = this.#touchNavigation.state if (this.#navigationLocked || !state) return - const contents = this.getContents?.() ?? [] - for (const { doc } of contents) { - const selection = doc?.getSelection?.() - if (selection && !selection.isCollapsed && selection.toString().trim()) return + if (this.#hasActiveTextSelection()) { + this.#beginTextSelection() + return } if (state.pinched) return state.pinched = globalThis.visualViewport.scale > 1 @@ -2241,7 +2261,7 @@ export class Paginator extends HTMLElement { // pre-empting them. if (this.hasAttribute('no-swipe')) return if (e.touches.length > 1) { - if (this.#touchScrolled) e.preventDefault() + if (this.#touchNavigation.scrolled) e.preventDefault() return } const touch = e.changedTouches[0] @@ -2273,7 +2293,7 @@ export class Paginator extends HTMLElement { state.dx += dx state.dy += dy state.dt += dt - this.#touchScrolled = true + this.#touchNavigation.markScrolled() if (!this.hasAttribute('animated') || this.hasAttribute('eink')) return if (!this.#vertical && Math.abs(state.dx) >= Math.abs(state.dy) && !this.hasAttribute('eink') && (!isStylus || Math.abs(dx) > 1)) { this.scrollBy(dx, 0) @@ -2282,15 +2302,19 @@ export class Paginator extends HTMLElement { } } #onTouchEnd() { + if (this.#hasActiveTextSelection()) { + this.#beginTextSelection() + return + } // Remove will-change hint to free GPU resources // if (this.#view?.element) { // this.#view.element.style.willChange = 'auto' // } - if (!this.#touchScrolled) return - this.#touchScrolled = false + const state = this.#touchNavigation.finish() + if (!state) return if (this.scrolled) { - if (!this.#navigationLocked) this.#startScrollInertia(this.#touchState) + if (!this.#navigationLocked) this.#startScrollInertia(state) return } if (this.#navigationLocked) return @@ -2301,15 +2325,14 @@ export class Paginator extends HTMLElement { // anything that doesn't work requestAnimationFrame(() => { if (globalThis.visualViewport.scale === 1) { - const { vx, vy, dx, dy, dt } = this.#touchState + const { vx, vy, dx, dy, dt } = state this.snap(vx, vy, dx, dy, dt) } }) } #onTouchCancel() { this.#cancelScrollInertia() - this.#touchState = null - this.#touchScrolled = false + this.#cancelTouchNavigation() } #cancelScrollInertia() { this.#scrollInertiaToken += 1