feat(super-editor): render imported heading/bookmark cross-references (SD-2495)#2882
Open
feat(super-editor): render imported heading/bookmark cross-references (SD-2495)#2882
Conversation
… (SD-2495) Wire the sd:crossReference v3 translator into the v2 importer entity list so REF / NOTEREF / STYLEREF fields imported from DOCX actually produce PM crossReference nodes instead of being silently dropped by the dispatch loop. Walk nested run wrappers when extracting the field's cached display text, and render the cross-reference as an internal hyperlink when the instruction carries the \\h switch. Route clicks on internal #bookmark anchors through goToAnchor so rendered cross-references navigate to their target in the document. Fixes IT-949 — Word cross-references (e.g. "Section 15") now appear in the viewer and are searchable, matching Word's output.
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
Contributor
|
Status: PASS The PR is spec-compliant. Here's what I checked:
REF field switches (§17.16.5.51) — The
Field structure — The decoded output follows the spec's required complex field pattern: a One non-blocking observation: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62179b6a5f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+31
to
+35
| if (target && /\\h\b/.test(instruction)) { | ||
| const synthesized = buildFlowRunLink({ anchor: target }); | ||
| if (synthesized) { | ||
| run.link = run.link ? { ...run.link, ...synthesized, anchor: target } : synthesized; | ||
| } |
There was a problem hiding this comment.
This code now attaches run.link for \h cross-references, but paragraph coalescing still merges adjacent text runs without considering link metadata (mergeAdjacentRuns in converters/paragraph.ts). If a cross-reference run is adjacent to same-styled plain text (a common inline REF layout), the merge step can drop the synthesized link entirely or extend it onto neighboring text, so click-to-anchor behavior becomes inconsistent and surrounding words may become incorrectly linked. These synthesized link runs need to be excluded from merge compatibility (or merge logic must compare run.link).
Useful? React with 👍 / 👎.
…oll container #scrollPageIntoView wrote scrollTop to #visibleHost, which is typically overflow: visible and therefore not the actual scroll target. Anchor navigation (TOC clicks and SD-2495 cross-reference click-to-navigate) silently did nothing whenever the bookmark target was outside the current viewport — the PM selection moved but the viewport never scrolled. Write to #scrollContainer (the resolved scrollable ancestor) as the primary target, plus #visibleHost for backward compatibility with legacy layouts and the existing test harness that mocks scrollTop on the host element. This unblocks SD-2495's cross-reference click-to-navigate on docs where cross-references and their targets live on different pages.
Opt-in visual indicators for bookmark positions — mirrors Word's "Show bookmarks" (File > Options > Advanced). Off by default. - Pm-adapter bookmark-start and new bookmark-end converters emit gray `[` and `]` marker TextRuns when `layoutEngineOptions.showBookmarks` is true. Markers flow through pagination and line breaking as real characters, matching Word's own visual model. - Auto-generated bookmarks (`_Toc…`, `_Ref…`, `_GoBack`) are hidden even when the feature is on — matching Word. A `renderedBookmarkIds` set on the converter context pairs suppression so closing brackets don't orphan open ones. - PresentationEditor.setShowBookmarks toggles at runtime: clears the flow-block cache and schedules a re-render. - SuperDoc.setShowBookmarks is the public API passthrough. - Dev app gets a Show/Hide bookmarks toggle button in the header. - CSS: subtle gray, non-selectable so users don't include brackets in copied text. Bookmark name surfaces via the native title tooltip on the opening bracket.
Fills the test gaps surfaced by the testing-excellence review of this PR:
- crossReferenceImporter.integration.test.js (new, 4 tests): exercises the
full v2 body pipeline (preprocessor -> dispatcher -> entity handler ->
v3 translator). Asserts crossReferenceEntity is a member of the
defaultNodeListHandler entities list, so the exact root cause that
produced IT-949 ("Section 15" vanishing) fails loudly if a future
refactor drops the wire-up. Unit tests of the translator alone cannot
catch this — they bypass the dispatcher.
- EditorInputManager.anchorClick.test.ts (new, 4 tests): pins the
SD-2537 click-to-navigate routing. Clicking #bookmark hrefs routes
through goToAnchor (was TOC-only before). External and empty-fragment
hrefs are explicitly NOT routed.
- cross-reference.test.ts: added marks-propagation test (node.marks flow
into the emitted TextRun so italic/textStyle on xref text survives —
SD-2537 "preserve surrounding run styling" AC).
- bookmark-markers.test.ts: converted the `for` loop over auto-generated
bookmark names into `it.each`. Each input now reports per-case on
failure, complies with testing-excellence's "no control flow inside
test bodies" guideline.
- PresentationEditor.test.ts: documents why the scrollContainer-vs-
visibleHost branch of the SD-2495 scrollPageIntoView fix isn't unit-
testable here (happy-dom doesn't propagate inline overflow through
getComputedStyle, which is what findScrollableAncestor uses).
5 tasks
caio-pizzol
pushed a commit
to kendaller/superdoc
that referenced
this pull request
Apr 22, 2026
Behavior test (tests/behavior/tests/navigation/pageref-standalone-click.spec.ts): Covers the PR's load-bearing case - a PAGEREF \h field NOT wrapped in a <w:hyperlink>. The existing toc-anchor-scroll.spec.ts only exercises the wrapped-in-hyperlink shape, where the outer link mark already propagates via marksAsAttrs and the PR is a no-op. Fixtures exercise both \h and \H (case-insensitivity per ECMA-376 17.16.1). Preprocessor unit tests (ref/noteref/styleref): These three importer modules were added in superdoc-dev#2882 without tests. Each verifies the preprocessor produces a sd:crossReference node with the right fieldType and preserves the instruction text verbatim.
caio-pizzol
added a commit
that referenced
this pull request
Apr 22, 2026
…#2899) * feat(super-editor): render imported heading/bookmark cross-references (SD-2495) Wire the sd:crossReference v3 translator into the v2 importer entity list so REF / NOTEREF / STYLEREF fields imported from DOCX actually produce PM crossReference nodes instead of being silently dropped by the dispatch loop. Walk nested run wrappers when extracting the field's cached display text, and render the cross-reference as an internal hyperlink when the instruction carries the \\h switch. Route clicks on internal #bookmark anchors through goToAnchor so rendered cross-references navigate to their target in the document. Fixes IT-949 — Word cross-references (e.g. "Section 15") now appear in the viewer and are searchable, matching Word's output. * fix(presentation-editor): anchor nav writes scrollTop to the real scroll container #scrollPageIntoView wrote scrollTop to #visibleHost, which is typically overflow: visible and therefore not the actual scroll target. Anchor navigation (TOC clicks and SD-2495 cross-reference click-to-navigate) silently did nothing whenever the bookmark target was outside the current viewport — the PM selection moved but the viewport never scrolled. Write to #scrollContainer (the resolved scrollable ancestor) as the primary target, plus #visibleHost for backward compatibility with legacy layouts and the existing test harness that mocks scrollTop on the host element. This unblocks SD-2495's cross-reference click-to-navigate on docs where cross-references and their targets live on different pages. * feat(layout): show-bookmarks bracket indicators (SD-2454) Opt-in visual indicators for bookmark positions — mirrors Word's "Show bookmarks" (File > Options > Advanced). Off by default. - Pm-adapter bookmark-start and new bookmark-end converters emit gray `[` and `]` marker TextRuns when `layoutEngineOptions.showBookmarks` is true. Markers flow through pagination and line breaking as real characters, matching Word's own visual model. - Auto-generated bookmarks (`_Toc…`, `_Ref…`, `_GoBack`) are hidden even when the feature is on — matching Word. A `renderedBookmarkIds` set on the converter context pairs suppression so closing brackets don't orphan open ones. - PresentationEditor.setShowBookmarks toggles at runtime: clears the flow-block cache and schedules a re-render. - SuperDoc.setShowBookmarks is the public API passthrough. - Dev app gets a Show/Hide bookmarks toggle button in the header. - CSS: subtle gray, non-selectable so users don't include brackets in copied text. Bookmark name surfaces via the native title tooltip on the opening bracket. * test: close regression gaps for SD-2495 / SD-2454 / anchor-nav fix Fills the test gaps surfaced by the testing-excellence review of this PR: - crossReferenceImporter.integration.test.js (new, 4 tests): exercises the full v2 body pipeline (preprocessor -> dispatcher -> entity handler -> v3 translator). Asserts crossReferenceEntity is a member of the defaultNodeListHandler entities list, so the exact root cause that produced IT-949 ("Section 15" vanishing) fails loudly if a future refactor drops the wire-up. Unit tests of the translator alone cannot catch this — they bypass the dispatcher. - EditorInputManager.anchorClick.test.ts (new, 4 tests): pins the SD-2537 click-to-navigate routing. Clicking #bookmark hrefs routes through goToAnchor (was TOC-only before). External and empty-fragment hrefs are explicitly NOT routed. - cross-reference.test.ts: added marks-propagation test (node.marks flow into the emitted TextRun so italic/textStyle on xref text survives — SD-2537 "preserve surrounding run styling" AC). - bookmark-markers.test.ts: converted the `for` loop over auto-generated bookmark names into `it.each`. Each input now reports per-case on failure, complies with testing-excellence's "no control flow inside test bodies" guideline. - PresentationEditor.test.ts: documents why the scrollContainer-vs- visibleHost branch of the SD-2495 scrollPageIntoView fix isn't unit- testable here (happy-dom doesn't propagate inline overflow through getComputedStyle, which is what findScrollableAncestor uses). * feat(pm-adapter): synthesize internal link for PAGEREF with \h switch Mirrors the pattern added for crossReference in #2882. When a PAGEREF field instruction carries the `\h` switch, attach a FlowRunLink via `buildFlowRunLink({ anchor: bookmarkId })` so clicks on the rendered page number navigate to the referenced bookmark through the existing anchor-link routing (`EditorInputManager.#handleLinkClick` → `goToAnchor`). Previously the PAGEREF inline converter emitted the `pageReference` token run with `pageRefMetadata.bookmarkId` but no `link`, so the DOM layer never produced a clickable element for PAGEREFs. TOC entries and other hyperlinked page references imported from Word therefore failed to navigate on click, even though Word honored the `\h` switch. Depends on #2882 for `buildFlowRunLink` export and the generalized anchor-click routing. * fix(pm-adapter): match PAGEREF \h switch case-insensitively Word field switches are case-insensitive per the field-code grammar, so `\H` should produce the hyperlink the same as `\h`. Reviewer (codex-connector) flagged that the original `/\\h\b/` regex skipped the link synthesis for instructions like `PAGEREF _Toc123 \H`, leaving them non-navigable even though the author had requested hyperlink behavior. Adds the `i` flag and a regression test with an uppercase switch. * test: add coverage for standalone PAGEREF \h + REF-family preprocessors Behavior test (tests/behavior/tests/navigation/pageref-standalone-click.spec.ts): Covers the PR's load-bearing case - a PAGEREF \h field NOT wrapped in a <w:hyperlink>. The existing toc-anchor-scroll.spec.ts only exercises the wrapped-in-hyperlink shape, where the outer link mark already propagates via marksAsAttrs and the PR is a no-op. Fixtures exercise both \h and \H (case-insensitivity per ECMA-376 17.16.1). Preprocessor unit tests (ref/noteref/styleref): These three importer modules were added in #2882 without tests. Each verifies the preprocessor produces a sd:crossReference node with the right fieldType and preserves the instruction text verbatim. * test(superdoc): cover setShowBookmarks propagation and no-op guard Closes the Codecov patch-coverage gap on SuperDoc.js flagged on PR #2899. The uncovered setShowBookmarks method came in via the SD-2454 merge and had no test for its config mutation, no-op short-circuit, or Boolean() coercion. Models the existing setDisableContextMenu test pattern. * refactor(pm-adapter): drop TextRun casts + case-insensitive \h in cross-reference Addresses review feedback on #2899: - page-reference.ts: remove redundant `as TextRun` casts — textNodeToRun already returns TextRun, so the casts are noise (cross-reference.ts:34 already does this cleanly). Shorten the \h comment. - cross-reference.ts: add the `i` flag to the \h switch regex to match ECMA-376 §17.16.1, same fix as 7f19358 for page-reference. Add a regression test covering `\H`. * test: add unit coverage for scroll fan-out when ancestor != host Stubs window.getComputedStyle to mark a wrapper element as scrollable so #findScrollableAncestor returns the wrapper, then asserts both the wrapper and the visibleHost receive scrollTop. This pins the SD-2495 fix: a revert to the pre-fix one-liner (writing only to the visibleHost) now fails. --------- Co-authored-by: Tadeu Tupinamba <tadeu.tupiz@gmail.com> Co-authored-by: Caio Pizzol <97641911+caio-pizzol@users.noreply.github.com> Co-authored-by: Caio Pizzol <caio@superdoc.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three features + one pre-existing bug fix, packaged under the Internal Anchors epic (SD-2495).
What ships
\hvariants are clickable and navigate to the target bookmark.[/]indicators around bookmarked content. Off by default, toggleable.Why it was broken (one root cause, two symptoms)
The
sd:crossReferencev3 translator existed but was never wired into the v2 importer's entity list. The passthrough fallback refused to wrap it (because it was "registered"), and no entity claimed it, so the dispatch loop dropped every REF field on the floor. On top of that, even if the node had reached the renderer,#scrollPageIntoViewwrotescrollTopto the wrong DOM element, so click-to-navigate silently no-op'd on any real consumer layout.How the fix is structured
Files at a glance
SD-2536 (import)
v2/importer/crossReferenceImporter.js— new, 3-line entity bridgev2/importer/docxImporter.js— registers entity in dispatcher listv3/handlers/sd/crossReference/crossReference-translator.js— recursive text walkSD-2537 (render + navigate)
pm-adapter/.../cross-reference.ts— always emits TextRun; synthesizes link from\hpresentation-editor/pointer-events/EditorInputManager.ts— any#<bookmark>click routes togoToAnchor(was TOC-only)presentation-editor/PresentationEditor.ts—#scrollPageIntoViewwrites to#scrollContainerSD-2454 (bracket indicators)
pm-adapter/.../bookmark-start.ts,bookmark-end.ts— emit[/]TextRuns when enabledpm-adapter/converter-context.ts—showBookmarksflag +renderedBookmarkIdsto pair suppressionpainters/dom/src/renderer.ts+styles.ts— gray CSS, hover tooltip from bookmark namesuper-editor/PresentationEditor.ts—setShowBookmarks(boolean)runtime togglesuperdoc/src/core/SuperDoc.js— public API passthroughsuperdoc/src/dev/components/SuperdocDev.vue— toolbar toggle buttonHow to verify
Open the dev app, upload a DOCX with cross-references:
Open a doc with user-named bookmarks:
[name]…[brackets appear. Hover for the tooltip with the bookmark name. Click again to hide.Programmatic consumers:
Tests
@superdoc/pm-adapter(includes newcross-reference.test.ts,bookmark-markers.test.ts)super-editor(includes new integration test,EditorInputManager.anchorClick.test.ts, cross-ref translator extensions)Regression guards worth noting:
crossReferenceImporter.integration.test.jsasserts the entity is a member of the v2 entities list — if a future refactor drops the wire-up, this breaks immediately.EditorInputManager.anchorClick.test.tspins the generalized anchor routing so nobody reverts the branch to TOC-only.bookmark-markers.test.tscovers the orphan-bracket suppression (bookmarkEnd won't emit]if the matching start was suppressed).Test plan
\w \h,\r \h,\w \n \h) import; click navigation works_Toc…/_Ref…stay hidden even when on; brackets disappear when toggled offpnpm test:layout— in CI / follow-upExplicitly out of scope (noted for the record)
\w/\r/\noutline-number recomputation (F9 in Word). We use Word's cached text as-is.tableOfContentsEntry,sequenceField,citation,authorityEntry,tableOfAuthorities— separate follow-up ticket.