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..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,5 +1,20 @@ /** * Processes a TC (table of contents entry) instruction and creates an `sd:tableOfContentsEntry` node. + * + * 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. * @param {import('../../v2/docxHelper').ParsedDocx} [_docx] The docx object (unused). @@ -7,7 +22,21 @@ * @returns {import('../../v2/types/index.js').OpenXmlNode[]} */ export function preProcessTcInstruction(nodesToCombine, instrText, _docx, instructionTokens = null) { + const before = []; + const after = []; + const entryElements = []; + let seenContent = false; + for (const node of nodesToCombine) { + const isBookmarkMarker = node?.name === 'w:bookmarkStart' || node?.name === 'w:bookmarkEnd'; + if (isBookmarkMarker) { + (seenContent ? after : before).push(node); + } else { + seenContent = true; + entryElements.push(node); + } + } return [ + ...before, { name: 'sd:tableOfContentsEntry', type: 'element', @@ -15,7 +44,8 @@ export function preProcessTcInstruction(nodesToCombine, instrText, _docx, instru instruction: instrText, ...(instructionTokens ? { instructionTokens } : {}), }, - elements: nodesToCombine, + elements: entryElements, }, + ...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 new file mode 100644 index 0000000000..b97a256833 --- /dev/null +++ b/packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/tc-preprocessor.test.js @@ -0,0 +1,101 @@ +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('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 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, 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', + 'sd:tableOfContentsEntry', + 'w:bookmarkEnd', + 'w:bookmarkStart', + 'w:bookmarkEnd', + ]); + expect(result[1].elements).toEqual([filler]); + }); +});