fix(reader): draw annotation highlights over bullet lists (#4552)

Highlights spanning paragraphs and a bullet list painted the paragraphs but not the list items: the overlayer split ranges with a hard-coded 'p, h1, h2, h3, h4' selector before collecting client rects, so li/blockquote/td text fell into no sub-range and produced no SVG rects. Bump foliate-js to split by text nodes (plus img/svg) instead, which covers every block type while still excluding the block border boxes that over-highlight blank space. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-06-12 16:34:34 +08:00
parent 28767fecd9
commit 12ac7ae6c0
4 changed files with 229 additions and 1 deletions
@@ -63,6 +63,7 @@
 - [Design system → DESIGN.md](feedback_design_system_doc.md) — codify recurring UI/UX rules in `apps/readest-app/DESIGN.md`; never `pl/pr/ml/mr/text-left/text-right` (RTL); §5 boxed list anatomy has uniform `min-h-14` rows and chromeless controls

 ## Reader UI Fixes
+- [Overlayer splitRange by text nodes](overlayer-splitrange-textnodes.md) — highlight SVG missed bullet-list text when range also touched a `<p>`: `#splitRangeByParagraph`'s `'p,h1-h4'` selector dropped `li` (3rd whack-a-mole after f087826/920676b); fix = walk text nodes + `img,svg` in overlayer.js, never block-tag selectors; jsdom test stubs `Range.prototype.getClientRects`
 - [Android image callout freeze](android-image-callout-freeze.md) — long-press `<img>` fires WebView native callout that collides with app touch handlers → whole-app freeze; `-webkit-touch-callout: none` doesn't inherit so put `.no-context-menu` on an ancestor of the image (`.no-context-menu img` rule in globals.css); seen on book covers (#4345) + image preview/zoom (#4420, `ImageViewer.tsx`)
 - [ProgressBar focus-ring line (#4397)](progressbar-focus-ring-4397.md) — decorative `.progressinfo` footer was `tabIndex={-1}` → Android long-press focused it → stray content-width focus-ring line at the bottom every page; fix = drop tabIndex (role='presentation' must not be focusable); ffmpeg-the-video debugging + live-browser `:focus-visible` confirmation
 - [Table dark-mode tint regression (#4419)](table-dark-mode-tint-4419.md) — `blockquote, table *` color-mix tint in `getColorStyles` must stay gated on `overrideColor` (gate added #2377, removed #4055, re-broke → #4419); safe now that #4392 light-bg rewriters handle #4028 zebra legibility; SAME rule paints vertical-TOC `.space`/▉ spacer cells (▉ U+2589 = blank glyph, contours=0) → "spacing changes" symptom; both fixed by the gate
@@ -0,0 +1,16 @@
+---
+name: overlayer-splitrange-textnodes
+description: "Overlayer highlight rects — split range by TEXT NODES, not a block-tag selector; li/blockquote text was silently dropped from SVG when highlight also touched a <p>"
+metadata: 
+  node_type: memory
+  type: project
+  originSessionId: 58d4b1a1-8823-4474-9966-c96727692e3f
+---
+
+Bug class (3rd occurrence): `Overlayer` in `packages/foliate-js/overlayer.js` splits a range into sub-ranges before `getClientRects()` so fully-contained block elements don't contribute border boxes (over-highlighting blank space — fork commit f087826 #10). The split was `querySelectorAll('p, h1, h2, h3, h4')`; 920676b added headings after the same hole; June 2026 the hole reappeared for `<li>`: a highlight spanning paragraphs + a bullet list drew NO rects over the list (li text fell into no sub-range), while a highlight entirely inside the list worked via the `splitRanges.length === 0 → [range]` fallback.
+
+**Fix:** `#splitRange` walks TEXT NODES (TreeWalker, `FILTER_REJECT` on non-intersecting elements prunes subtrees) plus replaced elements (`img, svg`), clipping first/last text nodes to the range boundaries. Text-node line rects never include block border boxes, cover every block type, and can't double-paint (`li > p` nesting). Shared `#getRects(range)` used by both `add()` and `redraw()`.
+
+**Why:** any hard-coded block-tag selector is whack-a-mole (li, blockquote, dd, td, div-paragraphs…) and adding container tags double-paints nested matched tags.
+
+**How to apply:** when highlight/overlay rects miss some content or over-paint blank space, check `#splitRange` in overlayer.js first. Test at `src/__tests__/document/overlayer-highlight-blocks.test.ts` — jsdom has no `Range.getClientRects`; stub the prototype to record `range.toString()` per call and assert covered text. Dev-web picked up the symlinked foliate-js edit on page reload (no server restart needed for next dev, unlike [[paginator-gutter-bleed-asymmetry-4394]]'s note).
@@ -0,0 +1,211 @@
+// Regression test for: highlights spanning a bullet list are not drawn over
+// the list items.
+//
+// Overlayer splits a range into sub-ranges before collecting client rects so
+// that fully-contained block elements don't contribute their border boxes
+// (which over-highlight blank space between lines/paragraphs). The split was
+// keyed on a hard-coded selector ('p, h1, h2, h3, h4'), so any text living in
+// other block containers — li, blockquote, dd, td, ... — fell into no
+// sub-range and was silently dropped from the drawn SVG rects whenever the
+// range also touched a matching paragraph/heading.
+//
+// The fix splits by text nodes (plus replaced elements like img/svg), which
+// covers every block type and still never yields block border boxes.
+import { afterEach, beforeEach, describe, expect, it } from 'vitest';
+
+import { Overlayer } from 'foliate-js/overlayer.js';
+
+interface RectLike {
+  left: number;
+  top: number;
+  right: number;
+  bottom: number;
+  width: number;
+  height: number;
+}
+
+// Records what content each getClientRects() call covered, so assertions can
+// check which parts of the document contribute rects to the overlay.
+let covered: string[] = [];
+
+const describeRange = (range: Range): string => {
+  const text = range.toString();
+  const frag = range.cloneContents();
+  return frag.querySelector('img, svg') ? `<replaced>${text}` : text;
+};
+
+function stubGetClientRects(this: Range): DOMRectList {
+  covered.push(describeRange(this));
+  const i = covered.length;
+  const rect: RectLike = {
+    left: 0,
+    top: i * 20,
+    right: 100,
+    bottom: i * 20 + 10,
+    width: 100,
+    height: 10,
+  };
+  return [rect] as unknown as DOMRectList;
+}
+
+const realGetClientRects = Object.getOwnPropertyDescriptor(Range.prototype, 'getClientRects');
+
+const drawSpy = () => {
+  const drawn: RectLike[] = [];
+  const draw = (rects: RectLike[]) => {
+    drawn.push(...rects);
+    return document.createElementNS('http://www.w3.org/2000/svg', 'g');
+  };
+  return { drawn, draw };
+};
+
+const byId = (id: string) => {
+  const el = document.getElementById(id);
+  if (!el) throw new Error(`missing #${id}`);
+  return el;
+};
+
+const textNode = (id: string) => {
+  const node = byId(id).firstChild;
+  if (!node || node.nodeType !== Node.TEXT_NODE) throw new Error(`#${id} has no text node`);
+  return node as Text;
+};
+
+describe('Overlayer rect splitting across block types', () => {
+  beforeEach(() => {
+    covered = [];
+    Object.defineProperty(Range.prototype, 'getClientRects', {
+      value: stubGetClientRects,
+      configurable: true,
+      writable: true,
+    });
+  });
+
+  afterEach(() => {
+    document.body.innerHTML = '';
+    if (realGetClientRects) {
+      Object.defineProperty(Range.prototype, 'getClientRects', realGetClientRects);
+    } else {
+      delete (Range.prototype as { getClientRects?: unknown }).getClientRects;
+    }
+  });
+
+  it('draws rects for list items when a highlight spans paragraphs and a bullet list', () => {
+    document.body.innerHTML = `
+      <section>
+        <p id="p1">The final role of taxes is to encourage certain behaviors.</p>
+        <p id="p2">To summarize, taxes have four functions:</p>
+        <ul>
+          <li id="li1">Make people accept the currency.</li>
+          <li>Redistribute income.</li>
+          <li>Reduce consumption to fight inflation.</li>
+          <li id="li4">Deter undesirable behaviors.</li>
+        </ul>
+      </section>`;
+    const range = document.createRange();
+    range.setStart(textNode('p1'), 0);
+    range.setEnd(textNode('li4'), textNode('li4').length);
+
+    const { drawn, draw } = drawSpy();
+    const overlayer = new Overlayer(document);
+    overlayer.add('annotation', range, draw);
+
+    const text = covered.join('');
+    expect(text).toContain('To summarize, taxes have four functions:');
+    expect(text).toContain('Make people accept the currency.');
+    expect(text).toContain('Redistribute income.');
+    expect(text).toContain('Reduce consumption to fight inflation.');
+    expect(text).toContain('Deter undesirable behaviors.');
+    expect(drawn.length).toBeGreaterThan(0);
+
+    // The range must stay split per block: a single getClientRects call over
+    // the whole range would include block border boxes and over-highlight the
+    // blank space between paragraphs (the reason the split exists).
+    for (const chunk of covered) {
+      expect(chunk.includes('To summarize') && chunk.includes('Make people')).toBe(false);
+    }
+  });
+
+  it('does not double-draw text of paragraphs nested inside list items', () => {
+    document.body.innerHTML = `
+      <section>
+        <p id="p1">Intro paragraph.</p>
+        <ul>
+          <li id="li1"><p id="nested">Paragraph inside a list item.</p></li>
+        </ul>
+      </section>`;
+    const range = document.createRange();
+    range.setStart(textNode('p1'), 0);
+    range.setEnd(textNode('nested'), textNode('nested').length);
+
+    const { draw } = drawSpy();
+    const overlayer = new Overlayer(document);
+    overlayer.add('annotation', range, draw);
+
+    const hits = covered.filter((t) => t.includes('Paragraph inside a list item.'));
+    expect(hits).toHaveLength(1);
+  });
+
+  it('covers other text blocks such as blockquotes', () => {
+    document.body.innerHTML = `
+      <section>
+        <p id="p1">Before the quote.</p>
+        <blockquote id="quote">Quoted text in a blockquote.</blockquote>
+        <p id="p2">After the quote.</p>
+      </section>`;
+    const range = document.createRange();
+    range.setStart(textNode('p1'), 0);
+    range.setEnd(textNode('p2'), textNode('p2').length);
+
+    const { draw } = drawSpy();
+    const overlayer = new Overlayer(document);
+    overlayer.add('annotation', range, draw);
+
+    const text = covered.join('');
+    expect(text).toContain('Before the quote.');
+    expect(text).toContain('Quoted text in a blockquote.');
+    expect(text).toContain('After the quote.');
+  });
+
+  it('clips the first and last blocks to the range boundaries', () => {
+    document.body.innerHTML = `
+      <section>
+        <p id="p1">To summarize, taxes have four functions:</p>
+        <ul>
+          <li id="li1">Make people accept the currency.</li>
+        </ul>
+      </section>`;
+    const start = textNode('p1');
+    const end = textNode('li1');
+    const range = document.createRange();
+    range.setStart(start, 'To summarize, taxes have '.length);
+    range.setEnd(end, 'Make people'.length);
+
+    const { draw } = drawSpy();
+    const overlayer = new Overlayer(document);
+    overlayer.add('annotation', range, draw);
+
+    const chunks = covered.filter((t) => t.trim() !== '');
+    expect(chunks[0]).toBe('four functions:');
+    expect(chunks[chunks.length - 1]).toBe('Make people');
+  });
+
+  it('still draws a rect for replaced elements like images inside the range', () => {
+    document.body.innerHTML = `
+      <section>
+        <p id="p1">Before the image. <img src="cover.png" alt=""> After the image.</p>
+      </section>`;
+    const p = byId('p1');
+    const range = document.createRange();
+    range.selectNodeContents(p);
+
+    const { draw } = drawSpy();
+    const overlayer = new Overlayer(document);
+    overlayer.add('annotation', range, draw);
+
+    expect(covered.some((t) => t.startsWith('<replaced>'))).toBe(true);
+    const text = covered.join('');
+    expect(text).toContain('Before the image.');
+    expect(text).toContain('After the image.');
+  });
+});