feat(footnote): word-like footnote interactions (SD-3400)#3696
Draft
tupizz wants to merge 36 commits into
Draft
Conversation
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
tupizz
force-pushed
the
tadeu/sd-3400-feature-footnote-interactions
branch
from
c542fe8 to
aca7065
Compare
…-3400) On the last page there is no continuation target, so the SD-2656 bodyMaxY-anchored maxReserve collapses to ~0 once the body fills the page: the planner can place nothing, reserves[pageIndex] stays 0, the body never yields, and every anchored footnote is silently dropped (no error). Reproduced on Footnote tests.docx: 0 of 6 footnotes rendered. Add a terminal-page reserve bump mirroring the existing carry-forward bump: when a footnote is anchored on the last page and the placed reserve is short of its demand, reserve that demand (capped at the physical band) so the next relayout pass shrinks the body and the footnote renders on its anchor page, matching Word. Guarded on reserves[pageIndex] < clusterDemand so pages whose footnote already placed fully are untouched (no gap regression on non-dense pages or multi-page splits). Footnote tests.docx now renders all 6 footnotes (grows 1 to 2 pages).
Note content is painted as generic .superdoc-fragment elements marked contenteditable=false, so the browser showed a default arrow over editable note text instead of an I-beam. Add a dedicated ensureFootnoteStyles injector (mirroring the other per-concern ensure*Styles) that sets cursor: text on fragments whose block-id starts with footnote-/endnote-/__sd_semantic_footnote- /__sd_semantic_endnote-. Wired into the renderer's one-time style injection.
… (SD-3400) Double-clicking a footnote/endnote reference marker in the body now opens the corresponding note. The painted reference is a superscript run carrying data-pm-start but no note id, so #handleDoubleClick resolves the PM node at that position; when it is a footnoteReference/endnoteReference it builds the note target and calls the existing activateRenderedNoteSession, which focuses the note session and scrolls it into view. Single-click behavior is unchanged.
…-3400)
Add PresentationEditor.activateNoteSession(target): opens a footnote/endnote
note session without a pointer, focusing the note and scrolling it into view
with the caret at the note's start. Makes #activateRenderedNoteSession's click
coords optional so the no-coords path skips hit-testing and lands at note start.
This closes the last gap in the insert-footnote flow: the existing
document.footnotes.insert() API creates the body marker + note entry (and the
notes part if absent) and returns the new noteId; a custom toolbar action then
calls activateNoteSession({ storyType, noteId }) so focus moves into the new
note and the user can type immediately. Insert stays in the document API and
focus stays in the presentation layer; the toolbar action composes the two
(kept off the default toolbar).
Word-like two-step delete from the body. The first Backspace with a collapsed
caret immediately after a footnote/endnote reference selects the marker (a
TextSelection spanning the atom, since footnoteReference is selectable:false);
the second Backspace sees a non-empty selection, so the new command returns
false and the chain falls through to deleteSelection, which removes the marker.
Removal/renumber then cascade through the existing pipeline (the renderer only
paints notes that still have a body reference).
New selectFootnoteMarkerBefore / selectFootnoteMarkerAfter commands wired into
the Backspace chain (after selectInlineSdtBeforeRunStart, before
backspaceAtomBefore) and Delete chain (after selectInlineSdtAfterRunEnd).
footnoteReference is intentionally NOT added to the backspaceAtomBefore
allowlist — staged selection + deleteSelection mirrors the SDT precedent.
Verified end-to-end: 1st press selects marker, 2nd press deletes it; note drops
from the area and remaining notes renumber (6 to 5; {2:1,3:2,4:3,5:4,6:5}).
Clearing all content of a footnote/endnote in the note area now deletes the whole footnote: commitNoteRuntime detects empty content and calls footnotesRemoveWrapper, which deletes the body reference node AND removes the OOXML note element (when no other reference remains). The document then renumbers through the existing pipeline. This is symmetric with the body-side staged delete — deleting from either side removes both the marker and the note (per product decision: remove on both sides), and it avoids the orphaned-ref state that previously left numbering inconsistent. Guarded on the body reference still existing so a stale/duplicate commit is a no-op. Wiring covered by unit tests (mocking the removal boundary, which is itself covered by footnote-wrappers.test.ts).
…hain (SD-3400)
Two manual-testing regressions:
Staged delete: each reference is wrapped in its own run, so a caret at the
start of the following run (the common position after clicking past the
superscript) saw nodeBefore as the run wrapper, failed the marker check, and
fell through to normal backspace — deleting the previous letter. The boundary
branches now unwrap a neighboring run whose trailing/leading child is a note
reference. Delete-key mirror gets the same treatment.
Double-click navigation: real pointer events land on the selection overlay
above the pages, so closest('[data-pm-start]') on the event target missed the
painted reference and the dblclick did nothing. The resolver now walks the
elementsFromPoint hit chain, mirroring the rendered-note resolver.
…SD-3400) Make FootnoteInsertInput.at optional: omitting it inserts the reference at the current selection head, which is what a toolbar action needs (place a marker at the current cursor location). docapi contract gates pass. Add editor.commands.insertFootnote(): inserts an empty footnote at the caret (creating the footnotes part with separators when the document has none) and activates the new note session so the user can immediately type the note text. Sets preventDispatch on the chain transaction because the document API dispatches its own compound transactions. Intentionally not registered in the default toolbar (per SD-3400) — any custom toolbar action can call it.
…delete (SD-3400) Three UX gaps from manual testing: Clickability: painted body reference markers now carry data-note-reference / data-note-id (stamped in buildReferenceMarkerRun, covers endnotes too) and get a pointer cursor plus a hover pill, signalling that the number is interactive. Focus feedback: while a note session is open, the note's fragments at the page bottom get the sd-note-session-active highlight (tint + accent bar + one-time pulse). Applied on activation, re-applied after every paint (fragments are rebuilt), removed on exit, and self-healing when the session ends through any path. Paint-only - no layout impact. Instant area-delete: clearing all content of a note that previously had content now auto-exits the session, which commits the both-sides removal immediately - no click back into the document required. Freshly inserted empty notes are exempt until they have held content, so insert-and-type is unaffected.
Lighter tint (0.12 to 0.07 alpha), thinner accent bar (2px to 1px) pushed 3px away from the note line via a masked box-shadow pair, gentler activation pulse. Feedback from manual review: the previous bar read too heavy next to the text.
…n (SD-3400) The painted reference digit is ~6x11px, which made the hover affordance and double-click navigation nearly impossible to acquire with a real mouse (the handler itself is robust — verified with realistic per-event sequences). An invisible ::after halo expands the interactive target to roughly 16x19px: hover, pointer cursor, and double-click all hit the marker span, with no text movement (pseudo-element is absolutely positioned off a position:relative span). Also wire an Insert footnote button into the dev app header as the demo of the custom-toolbar action: it calls editor.commands.insertFootnote(), which inserts at the caret and focuses the new note. The default product toolbar remains untouched per SD-3400.
…-3400) Opening a note session now brings the note into view. The scroll is smart: no-op when the note's fragment is already fully visible, otherwise it smooth-centers the fragment in the scroll container. Double-clicked notes are already painted and scroll immediately; freshly inserted notes only paint after the post-insert relayout, so the request stays pending and completes from the layoutUpdated hook once the fragment exists. Cleared on session exit. Verified live: double-click with the note band off-screen scrolls 0 to 490 with the note fully visible; toolbar insert scrolls 0 to 751 onto the new note.
Behavior-preserving modularization of the SD-3400 footnote interaction work, gated by the existing suites (111 super-editor tests + 16 layout-bridge footnote tests) and a browser smoke pass. - notes/note-target.ts: single source of truth for RenderedNoteTarget, parseRenderedNoteTarget, isSameRenderedNoteTarget, and the block-id prefix mapping. Removes the duplicated definitions in EditorInputManager and PresentationEditor. - notes/NoteSessionCoordinator.ts: extracts the active-note UX (highlight, smart scroll, emptied-note commit) out of PresentationEditor into a small collaborator with injected deps, following the dom/ coordinator precedent. PresentationEditor delegates via onActivated/onPaint/onExit; the logic is now unit-tested in jsdom (7 tests) instead of browser-only. - pointer-events/note-reference-hit.ts: pure resolver for double-clicked body reference markers (closest + elementsFromPoint walk); EditorInputManager keeps a thin delegating method. - extensions/footnote/insert-footnote.js: insertFootnoteAtCursor as a plain importable function; the PM command is now a 3-line shim, keeping the extension as adapter (schema, NodeView, command registration) with logic in modules. Covered by its own tests. - incrementalLayout.ts: dedupe the cluster-demand and band-cap computations shared by the carry-forward and terminal-page reserve bumps (clusterDemandFor/maxBandFor). - note-story-runtime.ts: split commitNoteRuntime into removeEmptiedNote / commitRichNoteContent / commitPlainTextNoteContent.
The note-session empty watch runs isNoteContentEmpty on activation; session editors whose doc is not a real PM document (detached/mocked editors in the PresentationEditor suite) threw doc.descendants is not a function and broke five existing footnote-interaction tests in CI. Emptiness triggers removal of the footnote, so an uninspectable doc must read as NOT empty: never delete on uncertainty.
…g types (SD-3400) Replaces the runtime duck-typing guard added in ddf80d7. The guard was a type-system lie: isNoteContentEmpty declares a ProseMirrorNode parameter and then cast it to question its own type. The real defect was an incomplete test double: PresentationEditor.test.ts mocks story editors whose state.doc had content.size and textBetween but no descendants, violating the doc contract the note-session empty watch relies on. Fix at the source: the mock doc now implements descendants consistently with the text it already claims to contain. isNoteContentEmpty keeps its honest contract and the coordinator keeps typed ProseMirrorNode docs with no casts. In production the session editor is always a real Editor, so no defensive runtime checks are warranted.
Reject footnotes.insert when the target editor is a note, header, or footer story (ECMA-376 17.11.14: a footnoteReference inside a footnote or endnote is non-conformant) and strip any reference nodes that reach a note commit through paste. Emptying a note in the note area now removes the note everywhere: every body reference (footnote and endnote ids are independent namespaces, so resolution is type-aware) plus the OOXML element, in one compound mutation.
The second Backspace/Delete on a staged-selected marker previously fell through to deleteSelection, which removed only the PM reference and left an orphaned w:footnote element in the notes part (exported to DOCX). Route the staged delete through removeNoteReferenceAt, extracted from footnotesRemoveWrapper: position-addressed removal that prunes the OOXML element when the deleted marker was the note's last reference of that type. Wired before deleteSelection in both keymap chains, keeping the 'remove on both sides' rule symmetric with the note-area delete.
Stamp w:pStyle FootnoteText/EndnoteText on note paragraphs generated from plain text, matching Word's note body styling (without it, exported new footnotes render at the Normal style size in Word). When bootstrapping a missing notes part, also write the special-note list to settings.xml (ECMA-376 17.11.9): w:footnotePr/w:endnotePr listing the separator (-1) and continuation separator (0), which strict consumers require before loading separators. Imported documents keep their settings untouched.
Double-clicking a REF/NOTEREF cross-reference that points at a footnote or endnote opens the corresponding note session. Word's cross-reference bookmark wraps the original note reference in the body, so the resolver follows crossReference.attrs.target to the named bookmarkStart and scans its content for a note reference. Cross-references to anything else (headings, tables) fall through to default double-click behavior.
Browser verification on footnote-tests.docx shows right-to-left drags in note content produce the same range as left-to-right with anchor and head swapped, across single-line, multi-line, past-margin, and escape-above-note drags. Pin the direction-agnostic drag path: anchor stays at the mousedown hit while the head follows the pointer backward.
Real imports emit cross-reference bookmarks as empty bookmarkStart and bookmarkEnd markers matched by id, with the note reference between them (verified on the NVCA fixture); the container shape from the schema is also supported. Scan the marker pair's document range for the note reference. Live-verified: double-clicking the REF field opens the referenced footnote session.
Marker font chain is now: explicit first-run value, then the document default run properties (w:docDefaults, half-points converted to px), then the constant. Notes whose first paragraph has no sized run (just emptied or inserted) keep markers sized with the document instead of snapping to the constant, so font changes resize the marker predictably. Keystroke perf with 94 footnotes (NVCA): buildFootnotesInput costs 7.3ms median / 9.2ms max per keystroke, under the 16ms frame budget, so no footnote measure cache is required.
End-to-end Playwright specs through the presentation surface: double click on a body reference marker opens the note session; staged Backspace selects then removes the marker, prunes the w:footnote element, and renumbers; clearing all note content removes the footnote on both sides and exits the session; insertFootnote places a marker at the cursor on a document without footnotes and focuses the new note. Also adds an unmocked area-delete integration test exercising the real removal pipeline (removeNoteEverywhere -> removeNoteElement) against a real footnotes part.
Add the footnote-tests-B fixture (explicit separator and continuation separator notes plus the settings.xml special-footnote list) and a roundtrip test asserting regular ids, separator types with reserved ids, the 17.11.9 special list, and body reference order all survive import and export.
The note caret/selection overlay mapped session positions through a visible-text-offset bridge that counts structural paragraph tokens on the painted side (pm-range gaps between lines) but not on the hidden editor side (raw text walk). Each Enter in a note drifted the caret 4 positions backward, so typing and arrow navigation rendered the caret inside the previous paragraph's text. Resolve note caret and selection geometry directly from the painted lines' session-coordinate pm ranges (data-pm-start/end), falling back to the offset bridge when a position is not painted (structural gaps, hidden tracked content). Single-paragraph notes are unaffected; multi paragraph carets now land on their own lines (live-verified).
Real-keystroke regression for the SD-3400 caret drift report: build a note with an empty middle paragraph, walk the caret up and down with arrows, and assert the painted caret lands on each paragraph's own line and that typing lands where the caret points.
Pressing Enter in a note session ran the linked-style clearing heuristic (splitRunToParagraph and splitBlock both call it on at-end splits) once the linked-styles cache populated on the shared converter. FootnoteText is a linked style (w:link FootnoteTextChar) but has no w:next, so Word keeps it on Enter; clearing it made new note paragraphs render at the document default size, appearing as random font growth mid-note. Skip the clearing for note story sessions. The heuristic stays active for body and header/footer flows it was built for.
Verified audit of every paragraph-creation, caret, and selection path in note sessions. Four fixes: Caret: 'selectionUpdate' fires before 'update', so the immediate selection-overlay flush ran before the epoch/layout gates armed and the caret rendered against the pre-change paint on every Enter/Backspace. Doc-changing transactions now defer to the post-paint flush; selection only changes keep the immediate path. When the pm-range resolver misses while a rerender is in flight (fresh unpainted paragraph), the overlay now reschedules after paint instead of bridging against stale geometry. Geometry: positions on paragraph-boundary tokens between painted lines snap forward to the next line instead of failing into the offset bridge; positions beyond the painted lines still return null so the post-paint retry handles them. Styles: the remaining paragraph-creation paths that dropped the note paragraph style are now note-session aware: createParagraphNear and liftEmptyBlock re-stamp the source paragraph's properties, joinBackward/ joinForward restore the survivor's style when a join loses it, and clearNodes preserves paragraphProperties. Pinned with a real-keymap burst test (Enter x3, type, Enter x2, type, Backspace x4, type) with the linked-styles cache armed.
A 360-op real-keystroke fuzzer (typing, Enter, Backspace, arrows, Home/ End over wrapped and empty paragraphs) exposed persistent caret misses: the painter skips repainting unchanged note paragraphs, so their painted data-pm ranges drift after edits shift positions — absolute pm resolution picked lines from neighboring paragraphs whose stale ranges overlapped the selection. Resolve the note caret by block identity instead: find the paragraph's sdBlockId in the session doc, scope resolution to that block's painted fragment, and translate the position into the fragment's coordinate space via the block-start delta (exact for unchanged and fresh blocks). Empty paragraphs anchor at their content start. Painted lines are also sorted by pm range before scanning, since fragments come back in DOM insertion order after incremental repaints. The fuzzer stays as a regression suite (3 deterministic seeds, ground truth computed from the session doc plus visible text, independent of painted pm attributes).
ArrowUp/ArrowDown in a note validated their hit test against the adjacent painted line's data-pm range and, on mismatch, binary-searched inside that range. Painted ranges of unchanged note paragraphs drift after edits above them (the painter skips repainting), so the stale window resolved to a position one line away and the caret skipped lines (user repro: ArrowUp jumped two lines at once). Translate the adjacent line's range into current session coordinates before the plausibility check and the goal-X fallback, anchoring on the line's paragraph block (sdBlockId lookup in the live doc, shifted by the fragment-start delta). Home/End boundary resolution gets the same translation. Pinned with a behavior test that drifts the painted ranges by editing paragraph 1 and then walks ArrowUp from the bottom asserting exactly one visual line per press.
Root cause of the SD-3400 note caret/selection/arrow family: the painter keeps reused fragments' data-pm attributes fresh through the body transaction mapping, but story fragments (notes, headers/footers) were excluded because the body mapping is the wrong coordinate space — so their painted positions went permanently stale after any edit above them, and every DOM-based consumer (caret overlay, selection rects, vertical arrow navigation, click mapping) read wrong positions. The resolved paint item carries fresh story positions every paint, so reused story fragments now get their attributes shifted by the delta between the fragment's fresh first-line position and the painted one — exact for unchanged blocks and for continuation fragments. The fuzzer now asserts the direct invariant: painted note line ranges never overlap once paint settles.
Replaying the user's recorded arrow session exposed two defects in vertical navigation inside note sessions: the goal column drifted right on every press, and hit testing could return positions near the note end for an adjacent line's Y. Root cause: the pipeline converted between layout and client coordinate spaces that disagree for note surfaces (computeCaretLayoutRect, denormalizeClientPoint, and the binary-search fallback each spoke a different space), and the +-5 position plausibility tolerance accepted hits up to ~50px off-column. Note sessions now resolve the target column natively: the goal column comes from the painted caret overlay (true client space) and the adjacent line position from caretRangeFromPoint on the painted line's text, mapped to a position through the leaf pm attributes. No layout/client conversions remain in the note path; body navigation keeps its existing pipeline with the hit test now fed client-space coordinates directly. The ArrowUp regression test asserts both one line per press and goal-column preservation.
NodeFilter and Node are browser globals not defined in the JS lint environment; take them from ownerDoc.defaultView, which is also more correct for cross-document usage. Fixes the lint failure on CI.
Header/footer and rendered-note fragments carry data-pm-start values in their own story coordinate space. The double-click note-reference resolver fed those positions into the BODY document's nodeAt, which throws for positions past the body size and aborted the double-click handler before header/footer activation ran, so footers with behindDoc textboxes could no longer be activated. Resolve only elements outside header/footer containers and note fragments, and bounds-check the position. (SD-3400)
caretRangeFromPoint is WebKit/Blink-only; firefox exposes caretPositionFromPoint. Without it, note sessions silently fell back to the mixed-coordinate hitTest path and the goal column drifted by tens of pixels on every vertical arrow. Normalize both APIs into one helper. (SD-3400)
tupizz
force-pushed
the
tadeu/sd-3400-feature-footnote-interactions
branch
from
3d7b874 to
6844904
Compare
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
2 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.
Implements SD-3400 Feature: Footnote Interactions: create, navigate, select, edit, and delete footnotes with Word-like behavior from both the body and the footnote area. All ticket Must-dos are in scope; nothing is deferred.
Prerequisite fix
Footnotes were silently dropped when the body filled a terminal page: the SD-2656 bodyMaxY-anchored reserve collapses to ~0 on the last page (no continuation target), the planner places nothing, and the body never yields. Reproduced on the ticket's own fixture (0 of 6 footnotes rendered). Fixed with a terminal-page reserve bump mirroring the existing carry-forward bump, guarded so already-placed footnotes are untouched.
Footnote tests.docxnow renders all 6 notes. Validated against the layout corpus footnote set (7 docs, no layout changes).Interactions delivered
w:footnoteelement when it was the note's last reference (no orphaned elements in the exported file); renumbering cascades. Forward Delete mirrors it. Handles the marker-wrapped-in-its-own-run caret boundaries.FootnoteInsertInput.atis now optional (omitted = insert at the caret), andeditor.commands.insertFootnote()inserts an empty note (creating the footnotes part with separators when absent) and moves focus into it. Inserting from inside a footnote, endnote, header, or footer story is rejected (ECMA-376 §17.11.14: a footnoteReference inside a note is non-conformant), and reference nodes that reach a note via paste are stripped at commit. Built for custom toolbars and intentionally not registered in the default toolbar; the dev app header has a demo button.buildFootnotesInputcosts 7.3ms median / 9.2ms max per keystroke, inside the 16ms frame budget, so no footnote measure cache is needed.Endnotes share the marker/navigation/removal plumbing (
endnoteReferenceis handled by the same resolvers, type-aware removal, and CSS).Word fidelity
w:pStyle FootnoteText/EndnoteText(Word always stamps it; without it new footnotes export at the Normal size).w:footnotePrwith ids -1 and 0) to settings.xml; imported documents keep their settings untouched.Architecture
Logic lives outside the ProseMirror extension: the extension keeps schema, NodeView, and thin command shims. New modules:
presentation-editor/notes/note-target.ts: single source of truth for note-target parsing.presentation-editor/notes/NoteSessionCoordinator.ts: highlight + smart scroll + emptied-note commit, unit-tested in jsdom.pointer-events/note-reference-hit.ts: pure pointer-to-note-target resolver (body markers + cross-references).extensions/footnote/insert-footnote.js/delete-note-marker.js: insert/staged-delete orchestration as plain functions.plan-engine/footnote-wrappers.ts:removeNoteReferenceAt(position-addressed single-reference removal with element pruning, shared by the document API and the keymap) andremoveNoteEverywhere(type-aware full removal for emptied notes).Tests