From 6ecff25dd132bff3c54b5643871039751775e94f Mon Sep 17 00:00:00 2001 From: Gabriel Chittolina Date: Tue, 26 May 2026 16:26:31 -0300 Subject: [PATCH 1/2] fix: enhance anchor navigation logic in TOC --- .../utils/AnchorNavigation.test.ts | 61 ++++++++++++++- .../utils/AnchorNavigation.ts | 32 +++++++- .../fld-preprocessors/tc-preprocessor.js | 24 +++++- .../fld-preprocessors/tc-preprocessor.test.js | 75 +++++++++++++++++++ 4 files changed, 183 insertions(+), 9 deletions(-) create mode 100644 packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.test.js diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/utils/AnchorNavigation.test.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/utils/AnchorNavigation.test.ts index 6e3cbb7dee..69ca6de59d 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/utils/AnchorNavigation.test.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/utils/AnchorNavigation.test.ts @@ -141,7 +141,7 @@ describe('goToAnchor', () => { expect(result).toBe(true); }); - it('should use nextFragmentY when bookmark is in a gap between fragments', async () => { + it('should use nextFragmentY when bookmark gap is closer to next fragment', async () => { const scrollContainer = createMockScrollContainer({ scrollTop: 0, rectTop: 0 }); const layout = makeLayout([ { @@ -152,18 +152,71 @@ describe('goToAnchor', () => { ], }, ]); - // Bookmark at position 50 — in the gap between fragments + // Bookmark at position 55 — gap to next (5) is smaller than gap to prev (15) const deps = makeDeps({ layout, scrollContainer, - bookmarks: new Map([['heading1', 50]]), + bookmarks: new Map([['heading1', 55]]), }); await goToAnchor(deps); // Should use nextFragmentY = 200 from the second fragment const call = (scrollContainer.scrollTo as ReturnType).mock.calls[0][0]; - expect(call.top).toBe(100 + 200); // pageRect.top + nextFragmentY * zoom(1) + expect(call.top).toBe(100 + 200); + }); + + it('falls back to next fragment on a PM-distance tie (preserves prior behaviour)', async () => { + const scrollContainer = createMockScrollContainer({ scrollTop: 0, rectTop: 0 }); + const layout = makeLayout([ + { + number: 1, + fragments: [ + { kind: 'para', pmStart: 0, pmEnd: 40, y: 72 }, + { kind: 'para', pmStart: 60, pmEnd: 100, y: 200 }, + ], + }, + ]); + // Bookmark at position 50 — equidistant from both fragments + const deps = makeDeps({ + layout, + scrollContainer, + bookmarks: new Map([['heading1', 50]]), + }); + + await goToAnchor(deps); + + const call = (scrollContainer.scrollTo as ReturnType).mock.calls[0][0]; + expect(call.top).toBe(100 + 200); + }); + + it('prefers the previous fragment when the bookmark sits just past it (SD-3227)', async () => { + // Mirrors the SD-3227 customer doc: the _Toc bookmark for the Section 1.3 + // heading is embedded inside a hidden TC field and ends up at a PM + // position right after the paragraph's visible runs. It must still + // resolve to that paragraph, not to the next heading paragraph. + const scrollContainer = createMockScrollContainer({ scrollTop: 0, rectTop: 0 }); + const layout = makeLayout([ + { + number: 1, + fragments: [ + { kind: 'para', pmStart: 0, pmEnd: 25, y: 72 }, + { kind: 'para', pmStart: 40, pmEnd: 80, y: 300 }, + ], + }, + ]); + // Bookmark at position 28 — only 3 past prev.pmEnd, 12 before next.pmStart + const deps = makeDeps({ + layout, + scrollContainer, + bookmarks: new Map([['heading1', 28]]), + }); + + await goToAnchor(deps); + + const call = (scrollContainer.scrollTo as ReturnType).mock.calls[0][0]; + // Should land on the first fragment (y=72), not the next one (y=300). + expect(call.top).toBe(100 + 72); }); it('should handle Window as scrollContainer', async () => { diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/utils/AnchorNavigation.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/utils/AnchorNavigation.ts index 3492c3719b..fba12251cf 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/utils/AnchorNavigation.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/utils/AnchorNavigation.ts @@ -135,6 +135,9 @@ export async function goToAnchor({ let nextFragmentPage: number | null = null; let nextFragmentStart: number | null = null; let nextFragmentY: number | null = null; + let prevFragmentPage: number | null = null; + let prevFragmentEnd: number | null = null; + let prevFragmentY: number | null = null; for (const page of layout.pages) { for (const fragment of page.fragments) { @@ -156,14 +159,35 @@ export async function goToAnchor({ nextFragmentStart = fragStart; nextFragmentY = fragment.y; } + + // SD-3227: also track the last fragment that ends at or before our + // position. Bookmarks embedded inside hidden field instructions (e.g. + // `_Toc…` inside a TC field) often land at a PM position right after + // the paragraph's visible runs — they belong to that paragraph, but + // sit outside `fragment.pmEnd` (which is derived from visible runs). + // We resolve this by picking whichever neighbour is PM-closer below. + if (fragEnd <= pmPos && (prevFragmentEnd === null || fragEnd > prevFragmentEnd)) { + prevFragmentPage = page.number - 1; + prevFragmentEnd = fragEnd; + prevFragmentY = fragment.y; + } } if (pageIndex != null) break; } - // Use the page of the next fragment if bookmark is in a gap - if (pageIndex == null && nextFragmentPage != null) { - pageIndex = nextFragmentPage; - fragmentY = nextFragmentY; + // No fragment contained the bookmark — choose the neighbour whose PM + // range is closer. Ties go to the next fragment to preserve the prior + // gap-between-paragraphs behaviour. + if (pageIndex == null) { + const prevGap = prevFragmentEnd != null ? pmPos - prevFragmentEnd : Infinity; + const nextGap = nextFragmentStart != null ? nextFragmentStart - pmPos : Infinity; + if (prevFragmentPage != null && prevGap < nextGap) { + pageIndex = prevFragmentPage; + fragmentY = prevFragmentY; + } else if (nextFragmentPage != null) { + pageIndex = nextFragmentPage; + fragmentY = nextFragmentY; + } } } diff --git a/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.js b/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.js index 54223fcd26..247dfab370 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.js @@ -1,5 +1,13 @@ /** * Processes a TC (table of contents entry) instruction and creates an `sd:tableOfContentsEntry` node. + * + * SD-3227: `w:bookmarkStart`/`w:bookmarkEnd` runs that the customer embeds + * inside the TC field's instruction (e.g. heading `_Toc...` targets) are + * lifted back out as siblings of the synthesized entry. The PM + * `tableOfContentsEntry` node is `atom: true`, so any bookmark left inside it + * would be invisible to `buildPositionMap` and `bookmarkStartNodeToBlocks`, + * leaving TOC navigation with no resolvable target. + * * @param {import('../../v2/types/index.js').OpenXmlNode[]} nodesToCombine The nodes to combine. * @param {string} instrText The instruction text. * @param {import('../../v2/docxHelper').ParsedDocx} [_docx] The docx object (unused). @@ -7,7 +15,20 @@ * @returns {import('../../v2/types/index.js').OpenXmlNode[]} */ export function preProcessTcInstruction(nodesToCombine, instrText, _docx, instructionTokens = null) { + const leadingBookmarks = []; + const trailingBookmarks = []; + const entryElements = []; + for (const node of nodesToCombine) { + if (node?.name === 'w:bookmarkStart') { + leadingBookmarks.push(node); + } else if (node?.name === 'w:bookmarkEnd') { + trailingBookmarks.push(node); + } else { + entryElements.push(node); + } + } return [ + ...leadingBookmarks, { name: 'sd:tableOfContentsEntry', type: 'element', @@ -15,7 +36,8 @@ export function preProcessTcInstruction(nodesToCombine, instrText, _docx, instru instruction: instrText, ...(instructionTokens ? { instructionTokens } : {}), }, - elements: nodesToCombine, + elements: entryElements, }, + ...trailingBookmarks, ]; } diff --git a/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.test.js b/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.test.js new file mode 100644 index 0000000000..790ff45029 --- /dev/null +++ b/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.test.js @@ -0,0 +1,75 @@ +import { describe, it, expect } from 'vitest'; +import { preProcessTcInstruction } from './tc-preprocessor.js'; + +describe('preProcessTcInstruction', () => { + it('creates a single sd:tableOfContentsEntry when no bookmarks are embedded', () => { + const instrText = 'TC "Section 1.1 Certain Basic Terms" \\f C \\l "2"'; + + const result = preProcessTcInstruction([], instrText); + + expect(result).toHaveLength(1); + expect(result[0]).toMatchObject({ + name: 'sd:tableOfContentsEntry', + type: 'element', + attributes: { instruction: instrText }, + elements: [], + }); + expect(result[0].attributes).not.toHaveProperty('instructionTokens'); + }); + + it('includes instructionTokens when provided', () => { + const tokens = [{ type: 'text', text: 'TC "x" \\l "2"' }]; + + const result = preProcessTcInstruction([], 'TC "x" \\l "2"', null, tokens); + + expect(result[0].attributes.instructionTokens).toEqual(tokens); + }); + + it('hoists embedded bookmarkStart/bookmarkEnd out of the TC entry (SD-3227)', () => { + const bookmarkStart = { + name: 'w:bookmarkStart', + type: 'element', + attributes: { 'w:id': '7', 'w:name': '_Toc230123327' }, + elements: [], + }; + const bookmarkEnd = { + name: 'w:bookmarkEnd', + type: 'element', + attributes: { 'w:id': '7' }, + elements: [], + }; + const filler = { name: 'w:something-else', type: 'element', elements: [] }; + const instrText = 'TC "Section 1.1 Certain Basic Terms" \\f C \\l "2"'; + + const result = preProcessTcInstruction([bookmarkStart, filler, bookmarkEnd], instrText); + + expect(result).toEqual([ + bookmarkStart, + { + name: 'sd:tableOfContentsEntry', + type: 'element', + attributes: { instruction: instrText }, + elements: [filler], + }, + bookmarkEnd, + ]); + }); + + it('preserves multiple embedded bookmark pairs around a single entry', () => { + const start1 = { name: 'w:bookmarkStart', type: 'element', attributes: { 'w:id': '1', 'w:name': '_Toc1' } }; + const start2 = { name: 'w:bookmarkStart', type: 'element', attributes: { 'w:id': '2', 'w:name': '_Toc2' } }; + const end1 = { name: 'w:bookmarkEnd', type: 'element', attributes: { 'w:id': '1' } }; + const end2 = { name: 'w:bookmarkEnd', type: 'element', attributes: { 'w:id': '2' } }; + + const result = preProcessTcInstruction([start1, start2, end1, end2], 'TC "x"'); + + expect(result.map((n) => n.name)).toEqual([ + 'w:bookmarkStart', + 'w:bookmarkStart', + 'sd:tableOfContentsEntry', + 'w:bookmarkEnd', + 'w:bookmarkEnd', + ]); + expect(result[2].elements).toEqual([]); + }); +}); From d37361ecf4b9fff327a28aa603dcc351b5dbf36c Mon Sep 17 00:00:00 2001 From: Gabriel Chittolina Date: Tue, 26 May 2026 18:12:49 -0300 Subject: [PATCH 2/2] fix: preserve bookmarkStart/end order --- .../fld-preprocessors/tc-preprocessor.js | 36 +++++++++++-------- .../fld-preprocessors/tc-preprocessor.test.js | 36 ++++++++++++++++--- 2 files changed, 53 insertions(+), 19 deletions(-) diff --git a/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.js b/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.js index 247dfab370..becae4f1ec 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.js @@ -1,12 +1,19 @@ /** * Processes a TC (table of contents entry) instruction and creates an `sd:tableOfContentsEntry` node. * - * SD-3227: `w:bookmarkStart`/`w:bookmarkEnd` runs that the customer embeds - * inside the TC field's instruction (e.g. heading `_Toc...` targets) are - * lifted back out as siblings of the synthesized entry. The PM - * `tableOfContentsEntry` node is `atom: true`, so any bookmark left inside it - * would be invisible to `buildPositionMap` and `bookmarkStartNodeToBlocks`, - * leaving TOC navigation with no resolvable target. + * SD-3227: `w:bookmarkStart`/`w:bookmarkEnd` runs that sit inside the TC + * field's instruction (e.g. heading `_Toc...` targets) are lifted back out + * as siblings of the synthesized entry. The PM `tableOfContentsEntry` node + * is `atom: true`, so any bookmark left inside it would be invisible to + * `buildPositionMap` and `bookmarkStartNodeToBlocks`, leaving TOC + * navigation with no resolvable target. + * + * The hoist preserves each marker's original relative order. Bookmark + * markers that appear before the first non-bookmark node go before the + * entry; markers that appear once non-bookmark content has been seen go + * after the entry. This keeps `w:id`-matched start/end pairs intact and + * avoids the crossed-range corruption you'd get from bucketing all starts + * before and all ends after the entry. * * @param {import('../../v2/types/index.js').OpenXmlNode[]} nodesToCombine The nodes to combine. * @param {string} instrText The instruction text. @@ -15,20 +22,21 @@ * @returns {import('../../v2/types/index.js').OpenXmlNode[]} */ export function preProcessTcInstruction(nodesToCombine, instrText, _docx, instructionTokens = null) { - const leadingBookmarks = []; - const trailingBookmarks = []; + const before = []; + const after = []; const entryElements = []; + let seenContent = false; for (const node of nodesToCombine) { - if (node?.name === 'w:bookmarkStart') { - leadingBookmarks.push(node); - } else if (node?.name === 'w:bookmarkEnd') { - trailingBookmarks.push(node); + const isBookmarkMarker = node?.name === 'w:bookmarkStart' || node?.name === 'w:bookmarkEnd'; + if (isBookmarkMarker) { + (seenContent ? after : before).push(node); } else { + seenContent = true; entryElements.push(node); } } return [ - ...leadingBookmarks, + ...before, { name: 'sd:tableOfContentsEntry', type: 'element', @@ -38,6 +46,6 @@ export function preProcessTcInstruction(nodesToCombine, instrText, _docx, instru }, elements: entryElements, }, - ...trailingBookmarks, + ...after, ]; } diff --git a/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.test.js b/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.test.js index 790ff45029..b97a256833 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.test.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.test.js @@ -55,21 +55,47 @@ describe('preProcessTcInstruction', () => { ]); }); - it('preserves multiple embedded bookmark pairs around a single entry', () => { + it('keeps adjacent bookmark pairs in their original order (no crossed ranges)', () => { + // Bucketing all starts before and all ends after the entry would turn + // [start1, end1, start2, end2] into [start1, start2, entry, end1, end2], + // making bookmark 1's range cover bookmark 2 and vice versa. const start1 = { name: 'w:bookmarkStart', type: 'element', attributes: { 'w:id': '1', 'w:name': '_Toc1' } }; - const start2 = { name: 'w:bookmarkStart', type: 'element', attributes: { 'w:id': '2', 'w:name': '_Toc2' } }; const end1 = { name: 'w:bookmarkEnd', type: 'element', attributes: { 'w:id': '1' } }; + const start2 = { name: 'w:bookmarkStart', type: 'element', attributes: { 'w:id': '2', 'w:name': '_Toc2' } }; const end2 = { name: 'w:bookmarkEnd', type: 'element', attributes: { 'w:id': '2' } }; - const result = preProcessTcInstruction([start1, start2, end1, end2], 'TC "x"'); + const result = preProcessTcInstruction([start1, end1, start2, end2], 'TC "x"'); + + expect(result).toEqual([ + start1, + end1, + start2, + end2, + { + name: 'sd:tableOfContentsEntry', + type: 'element', + attributes: { instruction: 'TC "x"' }, + elements: [], + }, + ]); + }); + + it('keeps post-content bookmark markers after the entry in their original order', () => { + const start = { name: 'w:bookmarkStart', type: 'element', attributes: { 'w:id': '1', 'w:name': '_Toc1' } }; + const filler = { name: 'w:something-else', type: 'element', elements: [] }; + const end1 = { name: 'w:bookmarkEnd', type: 'element', attributes: { 'w:id': '1' } }; + const trailingStart = { name: 'w:bookmarkStart', type: 'element', attributes: { 'w:id': '2', 'w:name': '_Toc2' } }; + const trailingEnd = { name: 'w:bookmarkEnd', type: 'element', attributes: { 'w:id': '2' } }; + + const result = preProcessTcInstruction([start, filler, end1, trailingStart, trailingEnd], 'TC "x"'); expect(result.map((n) => n.name)).toEqual([ - 'w:bookmarkStart', 'w:bookmarkStart', 'sd:tableOfContentsEntry', 'w:bookmarkEnd', + 'w:bookmarkStart', 'w:bookmarkEnd', ]); - expect(result[2].elements).toEqual([]); + expect(result[1].elements).toEqual([filler]); }); });