Skip to content

fix(core): skip data-highlight-exclude subtrees in the live selection#51

Merged
JaceThings merged 2 commits into
mainfrom
fix/selection-exclude-subtrees
Jun 10, 2026
Merged

fix(core): skip data-highlight-exclude subtrees in the live selection#51
JaceThings merged 2 commits into
mainfrom
fix/selection-exclude-subtrees

Conversation

@JaceThings

Copy link
Copy Markdown
Owner

Summary

  • Core fix. highlightSelection now carves [data-highlight-exclude] subtrees out of the painted selection ranges. The live marker paints by range geometry (getClientRects), which covers user-select: none text too, so a select-all (Cmd/Ctrl+A) previously banded over opted-out regions. The runs around excluded elements keep their exact geometry, and the document's own selection is left untouched, so single-range browsers are unaffected.
  • New helper. excludeMarkedSubtrees() in the targeting layer, reusing the existing isExcluded/EXCLUDE_ATTR logic that the page-scan path already had, plus regression tests.
  • Website. The docs demo cards (PaperCard) are marked data-highlight-exclude, so Cmd+A no longer paints their read-only quote text and the Highlight/Underline/Overline/Strike legend.
  • Docs. The core README and the highlightSelection JSDoc note that the attribute is honoured by the live marker.
  • Changeset. Patch bump for the linked packages.

Test plan

  • core unit tests pass (212, including 4 new excludeMarkedSubtrees tests)
  • core typecheck clean
  • website typecheck clean
  • Verified in-browser on /docs: after Cmd+A the global marker paints the page prose (126 bands) and 0 bands land over any demo card — quote and legend stay clean, while each card's own intended highlight remains

The selection marker paints by range geometry, which covers user-select:none
text too, so a select-all banded over opted-out subtrees. highlightSelection
now carves [data-highlight-exclude] out of the painted ranges, keeping the
runs around them whole. Mark the docs demo cards so Cmd+A no longer paints
their read-only quote and legend. Document the behaviour in the README and
the highlightSelection JSDoc.
@JaceThings JaceThings added the review Request Claude code review label Jun 10, 2026
Comment thread packages/core/src/targeting/include-exclude.ts
Comment thread apps/website/src/components/docs/PaperCard.tsx Outdated
@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review summary

Clean, focused fix — no blocking issues.

What it does. excludeMarkedSubtrees() carves [data-highlight-exclude] subtrees out of the selection ranges before painting. The gap-splitting loop preserves exact geometry of the runs between excluded elements, and every edge case (collapsed ranges, range fully inside an exclusion, no exclusions, adjacent exclusions) is handled correctly.

Integration. One call site in highlightSelection's rAF render path — the ranges → painted rename keeps the diff small and the intent clear.

Tests. 4 new cases cover the interesting shapes (middle, whole-range, none, multiple). Good coverage.

Security / Safety / Performance. Nothing to flag. querySelectorAll + intersectsNode inside a rAF callback is well within budget for typical pages, and the function degrades to a no-op in SSR (hasDomWithRange guard).

Two non-blocking nits posted inline (comment verbosity, outermost-only filter simplification).

LGTM — ship it.

- replace the quadratic outermost-only scan with an ancestor closest() check
- trim the PaperCard opt-out comment to one line
@JaceThings JaceThings added review Request Claude code review and removed review Request Claude code review labels Jun 10, 2026
@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review — clean ✅

Reviewed all 6 changed files (116 additions) across security, performance, safety, and format.

No blocking issues. No new inline comments.

What I checked

Category Result
🔒 Security No issues — excludeMarkedSubtrees operates on DOM ranges with a constant attribute selector; no user-controlled selectors or injection surface.
⚡ Performance Acceptable — querySelectorAll + intersectsNode + closest() runs per rAF frame during active selection, but all are native DOM calls bounded by the number of [data-highlight-exclude] elements. Fine for any realistic page.
🛡️ Safety No issues — optional chaining covers the null paths (root?.closest, el.parentElement?.closest), hasDomWithRange() guards SSR, and the cursor/gap range cloning pattern is correct.
✂️ Format Clean — both nits from the prior run were addressed in the second commit.

Previous comments

Both prior nits (O(n²) outermost-only scan → closest() ancestor check; verbose PaperCard comment → one-liner) were fixed. Threads resolved.

Notes

  • Good test coverage: the 4 new tests hit the main carve-out cases (middle exclusion, fully-excluded range, no exclusions, multiple exclusions).
  • The range-splitting logic (clone cursor → setEndBefore / setStartAfter) correctly preserves gap geometry without mutating the original selection.
  • Clean separation: excludeMarkedSubtrees reshapes only the painted ranges; the document selection is untouched.

@JaceThings JaceThings removed the review Request Claude code review label Jun 10, 2026
@JaceThings JaceThings merged commit 180b2f1 into main Jun 10, 2026
5 checks passed
@JaceThings JaceThings deleted the fix/selection-exclude-subtrees branch June 10, 2026 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant