SD-2663 - feature: support table of contents hovering#3333
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! |
![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: 4544d0019d
ℹ️ 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".
| const pendingTocLink = this.#pendingTocLinkNav; | ||
| this.#pendingTocLinkNav = null; | ||
| if (pendingTocLink && !this.#dragThresholdExceeded) { | ||
| this.#handleLinkClick(event, pendingTocLink); |
There was a problem hiding this comment.
The deferred TOC path always calls #handleLinkClick on pointerup when #dragThresholdExceeded is false, but it never verifies that the pointer was released on the same TOC link (or even inside it). Because this handler stores the link from pointerdown, a user can press on a TOC entry, move off the link, and still trigger goToAnchor on release, which removes the expected ability to cancel the action by releasing elsewhere.
Useful? React with 👍 / 👎.
| if (metadata.instruction) { | ||
| block.attrs.tocInstruction = metadata.instruction; | ||
| } | ||
| if (metadata.tocId) { |
There was a problem hiding this comment.
the tocId comment promises a docPartObj uniqueId or parent sdBlockId fallback, but the SDT call sites only pass docPartObjectId ?? undefined. since tableOfContentsHandler/genericDocPartHandler default the id to '' (empty string) when w:id is missing, the if (metadata.tocId) guard here drops it and group hover silently degrades to single-entry. fall back to the parent block's sdBlockId here, or stop defaulting the importer to empty string?
| @@ -3093,6 +3093,10 @@ export class DomPainter { | |||
| // Add TOC-specific styling class | |||
| if (isTocEntry) { | |||
| fragmentEl.classList.add('superdoc-toc-entry'); | |||
There was a problem hiding this comment.
the painter still hardcodes 'superdoc-toc-entry' while DOM_CLASS_NAMES.TOC_ENTRY is right there and used in PresentationEditor. swap so the class name has one source of truth. same thing at EditorInputManager.ts:1365.
| expect(goToAnchor).toHaveBeenCalledWith('#_Toc123'); | ||
| }); | ||
|
|
||
| it('does not navigate on TOC pointerdown alone — deferral guard', () => { |
There was a problem hiding this comment.
the whole reason for deferring nav to pointerup is to let drag-select work — but only the pointerdown-alone and clean-click paths are covered. can we add a pointerdown → pointermove past the drag threshold → pointerup case asserting goToAnchor is not called? otherwise a regression that ignores #dragThresholdExceeded slips through.
| } | ||
|
|
||
| /** TOC analogue of {@link #handleStructuredContentBlockMouseEnter}, keyed by `data-toc-id`. */ | ||
| #handleTocEntryMouseEnter = (event: MouseEvent) => { |
There was a problem hiding this comment.
none of the new hover logic has tests — group highlight on enter, cross-group clear on leave, same-group re-entry guard, #reapplyTocGroupHover after paint, and the zoom-scaled gap-fill math are all uncovered. at minimum a unit test with two .superdoc-toc-entry[data-toc-id] fragments verifying toc-group-hover lands on both would catch the most likely regressions.
| this.#lastHoveredTocGroup = null; | ||
| } | ||
|
|
||
| #setHoveredTocGroup(id: string) { |
There was a problem hiding this comment.
the five new methods mirror the SDT pair almost line-for-line (#handleX{MouseEnter,Leave}, #setHoveredX, #clearHoveredX, #reapplyXHover) plus a second mouseover/mouseout pair on the same host. fine for now since TOC has the gap-fill side effect, but a parameterized HoverGroupCoordinator would collapse both and the next hover-group concept won't multiply by five again. worth extracting now or wait until the third one shows up?
|
|
||
| #queryTocEntryElementsById(id: string): HTMLElement[] { | ||
| if (!this.#painterHost) return []; | ||
| const escapedId = typeof CSS !== 'undefined' && CSS.escape ? CSS.escape(id) : id.replace(/"/g, '\\"'); |
There was a problem hiding this comment.
the CSS.escape fallback ladder is inlined here for the third time in this file — there's already a file-scope escapeAttrValue helper around line 10287. can we reuse that here?
luccas-harbour
left a comment
luccas-harbour
left a comment
There was a problem hiding this comment.
hey @chittolinag!
codex flagged this issue below which I had noticed while testing this manually: https://www.loom.com/share/8047b874a3894628bbc6d13936ee8f43
you can see as I move the mouse around the TOC, the background flashes.
- [P2] Preserve TOC hover while crossing entry gaps — packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts:6773-6781
When the cursor moves from one TOC entry into the paragraph-spacing gap,relatedTargetis the underlying page/container rather than another.superdoc-toc-entry(the gap filler is a pseudo-element, not a DOM entry), so this immediately clearstoc-group-hover. That makes the intended continuous grey hover block disappear/flicker in the exact spacing gap that--toc-gap-belowis trying to cover; keep the group active while the pointer is inside the measured gap or otherwise make the gap participate in the hover target.
claude found some other things that I thought were relevant. I added them as inline comments, hopefully they should be easy fixes!
thanks!
4 participants
Introducing a new hover state for TOCs. Like MS Word does, when you hover in a TOC, we provide a visual feedback to the user with a grey background.