feat(pm-adapter): synthesize internal link for PAGEREF with \h switch#2899
feat(pm-adapter): synthesize internal link for PAGEREF with \h switch#2899caio-pizzol merged 12 commits intosuperdoc-dev:mainfrom
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.
…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).
Mirrors the pattern added for crossReference in superdoc-dev#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 superdoc-dev#2882 for `buildFlowRunLink` export and the generalized anchor-click routing.
![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: e7a142c49a
ℹ️ 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".
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.
|
Good catch — addressed in 7f19358. Added the Worth noting the original |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Closes the Codecov patch-coverage gap on SuperDoc.js flagged on PR superdoc-dev#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.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@kendaller thanks for jumping in on this. tested against Word and locally - click behavior matches. pushed a behavior test, the REF/NOTEREF/STYLEREF preprocessor tests, and a setShowBookmarks test to close the Codecov gap. one thing left: please also add the i flag to cross-reference.ts:31 per ECMA-376 §17.16.1. one small thing inline.
…ss-reference Addresses review feedback on superdoc-dev#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`.
|
@caio-pizzol thanks for pushing the extra test coverage. c421354 drops the |
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.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @kendaller! thanks for addressing last round.
i pushed a follow-up commit with a unit test that pins the scroll fix. it fakes window.getComputedStyle to mark a wrapper element as scrollable, then checks both the wrapper and the host get the scroll position. verified it catches a revert to the old one-line version.
lgtm.
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.43 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.45 |
|
🎉 This PR is included in template-builder v1.6.0-next.8 The release is available on GitHub release |
|
🎉 This PR is included in esign v2.3.0-next.45 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.28.0-next.8 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.8 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.6.0-next.44 |
4 participants
Summary
When a
PAGEREFfield instruction includes the\hswitch, attach aFlowRunLinkto the rendered token run so clicks on the page number navigate to the referenced bookmark. Mirrors the pattern added forcrossReferencein #2882.Demo
https://scale.zoom.us/clips/share/gMY1cl5NR0q6jYm3kjQPnQ
Motivation
Word TOCs and other hyperlinked page references use
PAGEREF _Toc<id> \h— the\hswitch tells Word to render the page number as a hyperlink. Before this change, the PAGEREF inline converter emitted apageReferencetoken run withpageRefMetadata.bookmarkIdbut nolink, so the DOM layer never produced a clickable element and clicks did nothing.Fix
In
pageReferenceNodeToBlock, after building the token run, callbuildFlowRunLink({ anchor: bookmarkId })wheninstructionmatches/\\h\b/and assign the result torun.link. The existing anchor-click routing added in #2882 (EditorInputManager.#handleLinkClick→goToAnchor) then handles navigation automatically.Dependency
This branch is cut from #2882 because it uses
buildFlowRunLinkfrommarks/links.ts, added in that PR. Expected merge order: #2882 → this. Happy to rebase once #2882 lands if the target changes.Test plan
page-reference.test.tscovering:\h\his absenthsubstring as\hpageReferencetoken +pageRefMetadata.bookmarkIdpnpm run lint— 0 errorspnpm run format:check— clean