From 5def8de380c5b1908e0b486369b3647217b19256 Mon Sep 17 00:00:00 2001 From: timfsus2ing Date: Tue, 17 Mar 2026 03:01:21 +0400 Subject: [PATCH] fix: register destinations only once for wrapped text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When text with an `id` wraps across multiple pages, the layout engine creates multiple fragments that all inherit the same `id` prop. Previously, each fragment would call `addNamedDestination`, causing later fragments to overwrite earlier ones. This broke reverse links (endnote → superscript) because the destination would point to the wrong page. Solution: - Track registered destinations using a Set in RenderOptions - Only register the FIRST occurrence of each ID - Subsequent occurrences (from wrapping) are silently skipped This ensures destinations always point to the first page where the content appears, enabling correct bidirectional links in academic documents with endnotes/footnotes. Fixes #2110, #2377, #2700 Co-Authored-By: Claude --- .../vite/src/examples/endnote/index.tsx | 49 ++++++++++++++ packages/examples/vite/src/examples/index.ts | 2 + packages/render/src/index.ts | 6 +- .../render/src/operations/setDestination.ts | 15 +++-- packages/render/src/primitives/renderNode.ts | 2 +- packages/render/src/types.ts | 1 + .../tests/operations/setDestination.test.ts | 66 ++++++++++++++++++- 7 files changed, 133 insertions(+), 8 deletions(-) create mode 100644 packages/examples/vite/src/examples/endnote/index.tsx diff --git a/packages/examples/vite/src/examples/endnote/index.tsx b/packages/examples/vite/src/examples/endnote/index.tsx new file mode 100644 index 000000000..3bfdfc3c7 --- /dev/null +++ b/packages/examples/vite/src/examples/endnote/index.tsx @@ -0,0 +1,49 @@ +import React from 'react'; +import { Page, Document, Link, Text, View } from '@react-pdf/renderer'; + +/** + * Demonstrates bidirectional endnote links. + * + * Key insight: The destination `id` should be on a block-level element (View), + * NOT on inline text. Links should wrap text directly for proper clickable areas. + */ +const Endnote = () => ( + + + + + This demonstrates bidirectional endnote links. Click the superscript + below to jump to the endnote, then click the back link to return. + + + + Here is some body text with a reference to an endnote{' '} + ¹ + {' '}that provides additional context. This demonstrates the "first + occurrence wins" fix for wrapped text destinations. + + + + + + + + 1. + This is the endnote explaining the reference. The link below should + take you back to the paragraph on page 1. + + + + ← Back to reference ¹ + + + + +); + +export default { + id: 'endnote', + name: 'Endnote', + description: 'Bidirectional endnote links', + Document: Endnote, +}; diff --git a/packages/examples/vite/src/examples/index.ts b/packages/examples/vite/src/examples/index.ts index 7981f733c..31fcb8517 100644 --- a/packages/examples/vite/src/examples/index.ts +++ b/packages/examples/vite/src/examples/index.ts @@ -1,6 +1,7 @@ import duplicatedImages from './duplicated-images'; import ellipsis from './ellipsis'; import emoji from './emoji'; +import endnote from './endnote'; import fontFamilyFallback from './font-family-fallback'; import fontWeight from './font-weight'; import fractals from './fractals'; @@ -25,6 +26,7 @@ const EXAMPLES = [ duplicatedImages, ellipsis, emoji, + endnote, fontFamilyFallback, fontWeight, fractals, diff --git a/packages/render/src/index.ts b/packages/render/src/index.ts index 05e1c4889..d663206b1 100644 --- a/packages/render/src/index.ts +++ b/packages/render/src/index.ts @@ -6,7 +6,11 @@ import { Context } from './types'; const render = (ctx: Context, doc: SafeDocumentNode) => { const pages = doc.children || []; - const options = { imageCache: new Map(), fieldSets: [] }; + const options = { + imageCache: new Map(), + fieldSets: [], + registeredDestinations: new Set(), + }; pages.forEach((page) => renderNode(ctx, page, options)); diff --git a/packages/render/src/operations/setDestination.ts b/packages/render/src/operations/setDestination.ts index 188ca7f90..5bbda3134 100644 --- a/packages/render/src/operations/setDestination.ts +++ b/packages/render/src/operations/setDestination.ts @@ -1,13 +1,20 @@ import { SafeNode } from '@react-pdf/layout'; -import { Context } from '../types'; +import { Context, RenderOptions } from '../types'; -const setDestination = (ctx: Context, node: SafeNode) => { +const setDestination = (ctx: Context, node: SafeNode, options: RenderOptions) => { if (!node.box) return; if (!node.props) return; - if ('id' in node.props) { - ctx.addNamedDestination(node.props.id!, 'XYZ', null, node.box.top, null); + if ('id' in node.props && node.props.id) { + const id = node.props.id; + + // Only register the first occurrence of each ID to prevent + // wrapped text fragments from overwriting the destination + if (!options.registeredDestinations.has(id)) { + options.registeredDestinations.add(id); + ctx.addNamedDestination(id, 'XYZ', null, node.box.top, null); + } } }; diff --git a/packages/render/src/primitives/renderNode.ts b/packages/render/src/primitives/renderNode.ts index f014013bb..5f0dea351 100644 --- a/packages/render/src/primitives/renderNode.ts +++ b/packages/render/src/primitives/renderNode.ts @@ -88,7 +88,7 @@ const renderNode = (ctx: Context, node: SafeNode, options: RenderOptions) => { if (cleanUpFn) cleanUpFn(ctx, node, options); - setDestination(ctx, node); + setDestination(ctx, node, options); renderDebug(ctx, node); ctx.restore(); diff --git a/packages/render/src/types.ts b/packages/render/src/types.ts index 393289a4e..0b7a9c924 100644 --- a/packages/render/src/types.ts +++ b/packages/render/src/types.ts @@ -22,4 +22,5 @@ export type Context = typeof PDFKitDocument & { export interface RenderOptions { imageCache: Map; fieldSets: (typeof PDFKitReference)[]; + registeredDestinations: Set; } diff --git a/packages/render/tests/operations/setDestination.test.ts b/packages/render/tests/operations/setDestination.test.ts index 543633018..a33d0bc66 100644 --- a/packages/render/tests/operations/setDestination.test.ts +++ b/packages/render/tests/operations/setDestination.test.ts @@ -12,8 +12,9 @@ describe('operations setDestination', () => { const box = { top: 20 }; const props = { id: 'test' }; const doc = { type: P.View, style: {}, props, box } as SafeNode; + const options = { registeredDestinations: new Set() }; - setDestination(ctx, doc); + setDestination(ctx, doc, options); expect(ctx.addNamedDestination.mock.calls).toHaveLength(1); expect(ctx.addNamedDestination.mock.calls[0][0]).toBe('test'); @@ -23,9 +24,70 @@ describe('operations setDestination', () => { test('should not call addNamedDestination method to passed context if id missed', () => { const ctx = createCTX(); const doc = { type: P.View, style: {}, props: {} } as SafeNode; + const options = { registeredDestinations: new Set() }; - setDestination(ctx, doc); + setDestination(ctx, doc, options); expect(ctx.addNamedDestination.mock.calls).toHaveLength(0); }); + + test('should only register the first occurrence of duplicate IDs (first wins)', () => { + const ctx = createCTX(); + const options = { registeredDestinations: new Set() }; + + // First fragment (page 1) + const fragment1 = { + type: P.Text, + style: {}, + props: { id: 'wrapped-text' }, + box: { top: 100 }, + } as SafeNode; + + // Second fragment (page 2) - should be ignored + const fragment2 = { + type: P.Text, + style: {}, + props: { id: 'wrapped-text' }, + box: { top: 200 }, + } as SafeNode; + + setDestination(ctx, fragment1, options); + setDestination(ctx, fragment2, options); + + // Should only be called once for the first fragment + expect(ctx.addNamedDestination.mock.calls).toHaveLength(1); + expect(ctx.addNamedDestination.mock.calls[0][0]).toBe('wrapped-text'); + expect(ctx.addNamedDestination.mock.calls[0][3]).toBe(100); // First fragment's y position + + // Verify the ID was tracked + expect(options.registeredDestinations.has('wrapped-text')).toBe(true); + }); + + test('should allow different IDs to be registered', () => { + const ctx = createCTX(); + const options = { registeredDestinations: new Set() }; + + const node1 = { + type: P.Text, + style: {}, + props: { id: 'destination-1' }, + box: { top: 100 }, + } as SafeNode; + + const node2 = { + type: P.Text, + style: {}, + props: { id: 'destination-2' }, + box: { top: 200 }, + } as SafeNode; + + setDestination(ctx, node1, options); + setDestination(ctx, node2, options); + + // Both should be registered + expect(ctx.addNamedDestination.mock.calls).toHaveLength(2); + expect(ctx.addNamedDestination.mock.calls[0][0]).toBe('destination-1'); + expect(ctx.addNamedDestination.mock.calls[1][0]).toBe('destination-2'); + expect(options.registeredDestinations.size).toBe(2); + }); });