feat(annotate): placed comment markers for raw-HTML annotation - #1257
Merged
Conversation
Annotation state is no longer written into the visited page's DOM. A fixed, pointer-transparent, shadow-rooted overlay host (appended to the root element, outside page layout) now owns every committed annotation visual: - numbered placed-marker buttons (product-owned SVG speech bubble, accent-token colors, accessible 'Comment N' labels) projected at the user's selected relative point, re-resolved from durable anchors and reprojected on scroll/resize/mutation/animation via rAF-coalesced invalidation (never polled) - persistent highlight rectangles for text-range annotations (built from Range.getClientRects), replacing inline <mark> wrapping - the focused (blue) treatment as overlay rects covering EVERY rect of EVERY target, replacing the .focused class that querySelector applied to only the first mark of a multi-paragraph selection - the draft selection highlight as overlay rects from the live pending range Markers omit rather than guess: unresolved anchors, zero-size targets, viewport/clip-scrolled-away targets, and clamped points no longer visibly associated with their target all hide the marker. Viewport-edge clamping keeps the full marker reachable; coincident markers spread horizontally and deterministically. Numbering is parent-authoritative: HtmlViewer syncs the ordered saved-annotation list (panel order, index+1) to the bridge; grouped multi-select targets all carry their one annotation's number. The anchor DTO additively gains the normalized selected point (validated and clamped at the trust boundary), captured from pinpoint/shift/data-annotate clicks. Fixes the partial/auto blue highlight (focus-mark and scroll-to touched only the first mark) and page layout breakage (surroundContents plus padding/negative-margin marks mutated author content).
Mark/badge assertions become overlay assertions: markers are queried through the shadow overlay host, restoration binding is proven via scroll-to targets instead of inline mark containment, and page-DOM purity is asserted byte-for-byte across restores.
…tract Bridge-side (srcdoc.test.ts): layout neutrality (byte-identical page DOM across restore/sync/focus/scroll, host outside body, pointer-transparent), relative-point capture and reprojection with fresh geometry, unresolved- anchor and scrolled/clipped omission, viewport edge clamping with visually-detached omission, deterministic coincident spreading, parent- synced numbering override and renumbering, malformed sync rejection, full-coverage focus rects (partial-blue regression), overlay draft highlight, and hit-test yielding beneath markers. Parent-side (htmlPinpointProtocol.test.tsx): anchor-point validation (clamping, hostile-point dropping without losing the anchor), point propagation onto committed annotations, multi-target anchor points, and the ordered saved-annotation number sync (createdA order, index+1, globals excluded, never before bridge-ready).
…ch, honest edge clamping - M1: every painted highlight rect (committed comment/deletion, focus flash, draft selection) is now intersected with the target's clip-ancestor chain via shared clipBoundsFor()/clipRect(); rects with no visible remainder are dropped, so inner-scroll-container content scrolled out of its box no longer paints stripes over unrelated content. - M2: computed-style visibility gate (visibility:hidden/collapse, display:none, opacity:0) treats targets as unresolved-for-display, so markers/focus rects stop rendering over visible content stacked in the same box (e.g. visibility-toggled carousel slides). - M3: dead-target re-search (whole-document findTextRange + anchor re-resolution) is generation-gated: the generation advances only on text-capable signals (page mutations, settle events, frame loads), never on scroll/resize, and each target caches its last failed generation. - m3: clipBoundsFor skips plain static overflow clippers for position:fixed targets until a fixed containing block (transform/perspective/filter/ backdrop-filter/will-change) is reached. - m4: marker association is tested against the UNCLAMPED point; the 29px viewport inset is rendering-only, so fully visible edge-flush elements keep their markers (dead band removed; sliver test updated — it conflated viewport-edge clamping with clip-container omission). - m5: reconcile on document.fonts.ready and capture-phase subresource load (geometry-only: they never unlock dead-target re-search). - m7: Range.getClientRects containment filter drops border boxes that duplicate their own line rects (redline double-paint). - m9: settle events from viewer overlay nodes are identity-filtered out. - m10: one marker per resolved element per record during refresh. - m12: painting caps at 48 rects but the marker anchors to the TRUE last client rect.
The branch's '@media print { .pn-layer { display:none } }' hid ALL annotation visuals in print, but pre-overlay the inline highlight marks stayed visible in print on purpose (only pin badges were print-hidden). On beforeprint (plus a matchMedia('print') mirror for Safari), committed comment/deletion rects are re-projected into a temporary absolute-positioned light-DOM layer in document coordinates so it paginates with content, and torn down on afterprint. Markers remain print-hidden (parity: highlights print, markers don't). A screen media guard keeps the layer from ever flashing on screen, and any build error fails safe into printing without visuals.
…inpoint hover (M5, m6, m8, m11) - M5: clicking anywhere on a committed range highlight posts mark-click again (pre-overlay parity). Rects stay pointer-transparent — the document bubble click handler hit-tests the point against the painted committed rects (smallest wins on overlap, ties to the topmost/later annotation). Coexistence matches the old '.annotation-highlight' handler: capture-phase pinpoint annotate clicks stopPropagation() first, marker buttons stop propagation, shift-clicks and drag-selection tails are skipped, and [data-annotate] elements defer to a highlight under the click point. - m6: when the raw (pre-yield) pinpoint hit is a placed marker, the hover advertises the MARKER's identity (label 'Comment N', no annotate box) instead of labeling the element beneath — click and hover now agree. Annotating beneath still works by moving off the 25px bubble. - m8: the pinpoint hover label is kept AFTER the overlay host on the root element (re-appended when not last), so marker bubbles can never occlude it at equal z-index. - m11: clear-marks also clears the parent-synced number map so stale numbers cannot leak onto future records reusing an id.
…sync feed at 512 (m1, m2) - m1: parseBridgeMessage rejects mark-click ids longer than 256 chars — the one page-controlled string in the changed path that lacked a length cap (the bridge's own sync validation already caps ids at 256). - m2: the HtmlViewer sync-annotations effect slices the ordered collection to 512 entries AFTER the stable sort, mirroring the bridge-side MAX_SYNC_ANNOTATIONS bound so both sides agree on the first 512 numbers.
…ion coverage - M6: restores the migration-dropped EXTENT assertions via a test-only bridge introspection hook (committedRanges): the vim Visual commit binds exactly 'Alpha ', the visual-block commit exactly 'Whole block target', and the pin-restore scoped range binds inside the anchored element covering exactly 'Anchor target text' (the scroll proxy only checked the element target). - Regression tests for every behavioral fix: clip-tested highlight rects + focus flash (M1), style-hidden visibility gate (M2), generation-gated dead-target re-search (M3), print-parity layer lifecycle (M4), highlight click-to-select with smallest-wins overlap (M5), fixed-position clip exemption (m3) plus preserved clip-container omission, containment filter (m7), refresh dedup (m10), clear-marks numbering reset (m11), true-last-rect marker anchoring past the paint cap (m12), and marker-consistent pinpoint hover with label paint order (m6/m8). - Parent-side: sync feed truncation at 512 after the stable sort (m2) and the 256-char mark-click id cap (m1). - Test honesty: the hit-test-yield test now string-asserts the exact ':host([data-pn-hittest]) .pn-marker' pointer-events rule, since the elementFromPoint mock implements the yield itself. - Tests advance the re-search generation via the settle-event signal: happy-dom stops delivering body MutationObserver callbacks once the overlay host holds an SVG marker button (environment bug; isolated repro without any bridge code — real browsers are unaffected).
…-last read The m12 fix had removed the collection cap: rangeClientRects materialized EVERY client rect and the O(n^2) containment filter ran over the full list, per range target per rAF reconcile and synchronously per click hit-test. A large drag-selection or redline (the Range extent is uncapped — only the selection text is capped at 10k chars) yields thousands of rects, i.e. tens of millions of iterations per scroll frame. Collection now breaks at MAX_HIGHLIGHT_RECTS again, so the zero-size and containment (m7) filters operate on at most 48 entries, and the m12 requirement is met by reading the DOMRectList's final entry directly by index (with the marker-association union extended to that tail rect, and a zero-size tail falling back to the last paintable rect). Regression test: 60 mocked rects with a containing border box paint 47 (cap + containment) while the marker anchors at the true 60th rect; mutation-verified against both an uncapped collection and a capped-prefix last-rect read.
…lipping; dedup among placed markers - The M2 gate's opacity:0 leg hid markers for the legitimate invisible-hit-target pattern (transparent input stretched over a styled control — the pinpoint hit resolves the input and pre-overlay the marker rendered exactly over the visible control), and failed its own carousel motivation anyway: computed opacity does not inherit, so a container faded to 0 leaves descendants at computed 1. visibility:hidden/collapse and display:none remain the gate. - establishesFixedContainingBlock also treats contain layout/paint/strict/ content and container-type size/inline-size as establishing a fixed containing block, so such clippers correctly apply to fixed targets. - The per-record element dedup now runs among PLACED (visible) markers only: a target whose stored point is clipped away no longer consumes the element's slot and suppresses a sibling target whose point is visible. Tests updated/added and mutation-verified: opacity keeps the marker, contain:layout re-applies the clipper, and the visible sibling survives the dedup.
…on the root element
- The M3 generation gate could lock out re-search forever when a page swaps
the <body> element itself: the observer watched document.body, so
documentElement.replaceChild(newBody, oldBody) produced no record, no
generation bump, and a dead target whose one free retry ran against the
interim skeleton never retried again. The observer now watches
document.documentElement (same config), so body swaps and the new body's
content are in-subtree.
- Consequence handled: childList mutations on the root/body whose
added/removed nodes are ALL overlay-registered (host append, hover-label
re-append, print-layer insert/remove) are filtered out via
isOverlayOnlyMutation, so overlay writes neither bump the generation nor
schedule the reconcile frame that caused them. The print layer stays
overlay-registered through its async removal record (retired-layer
deregistration is deferred to the next lifecycle step).
- The print layer is appended to documentElement instead of body: a page
styling body { position: relative } made body's padding box the containing
block and shifted every stripe by body's document offset. On <html> the
containing block is the ICB, matching the viewport+scroll coordinates; a
positioned documentElement is accepted as out of scope (commented).
Tests: the bridge's observer is captured at load (happy-dom stops
delivering records once the overlay host holds an SVG marker — environment
bug, so scope tests assert the observed target and drive the callback with
synthetic records): body-swap unlock, overlay-only no-bump, and the
print-layer parent are all covered and mutation-verified.
backnotprop
added a commit
that referenced
this pull request
Aug 10, 2026
…bering, OpenCode 2 parity) (#1258) * fix(opencode): consolidate V2 system parts into one composed prompt (#1114) The OpenCode 2 adapter still shipped the pre-#1114 multi-part system injection: replacePlanningSystemParts kept one part per source and the generic reminder pushed a separate part, so Qwen3.x Jinja template corruption persisted for OpenCode 2 users. Mirror the V1 entry exactly: compose the stripped existing text plus additions into a single system part via composeSystemPrompt, and compose the generic reminder into the existing text instead of appending a second part. Also adds the regression tests for the bug class flagged in #1114's review: both helpers must read/compose the existing system text BEFORE truncating the array (a reorder to 'system.length = 0' first drops the host prompt and goes red here). * perf(annotate): harden the raw-HTML overlay reconcile (dead-target backoff, cull, batching) Bridge-script hardening for mutation-heavy pages and large annotation sets, plus the lost click-to-select hover affordance: - A: dead-target re-search now carries a wall-clock backoff (300ms doubling to a 5s cap, reset on success) ON TOP of the generation gate, plus a 2-searches-per-reconcile-pass budget with a scheduled follow-up pass for budget-skipped eligible targets. A page that mutates every frame advances domGeneration every frame, so the generation gate alone re-ran the whole-document TreeWalker sweep (and anchor re-resolution) per frame forever for permanently unresolvable targets. - B1: early viewport cull (64px margin) for element and range targets: wholly offscreen targets skip targetStyleHidden / getComputedStyle / clipBoundsFor / client-rect collection entirely and just omit their markers, which is what the visible pipeline produced anyway. - B2: read/write batching in renderAnnotationOverlay: highlight rects are queued during the read phase and flushed as one write phase, so the pass no longer forces a synchronous layout per record. - B3: restoreAnnotation defers its render through the existing rAF-coalesced reconcile scheduler; restoring N annotations now renders once instead of N full passes (searches stay synchronous for the mark-applied reply). DOM tests flush the frame via the suite's standard macrotask flush. - B4: zero-work observer gate: page mutations with no records, no pending draft, and pinpoint inactive still bump domGeneration but no longer schedule a reconcile frame. - D: hover affordance for click-to-select: the rAF-throttled mousemove hit-tests the pointer against the CACHED rendered committed rects and toggles a brightness class on that annotation's rect divs inside the shadow root. No page-DOM writes, rects stay pointer-transparent, and shadow-root writes are unobserved so there is no reconcile loop. - G: while a text drag is in progress in drag mode, placed markers yield pointer input (data-pn-hittest) so the 25px bubble cannot capture a selection drag; armed only by a >4px primary-button move from a non-overlay mousedown, so marker clicks and click-to-select paths are untouched. withMarkersYielded now restores (not clears) the attribute. New regression tests for A, B1, B3, B4, D; A/B1/B3 mutation-verified (fix reverted, test observed failing, fix restored). * fix(annotate): make on-page marker numbers match exportAnnotations numbering The HtmlViewer sync excluded GLOBAL_COMMENT annotations before numbering while exportAnnotations numbers '## N.' sections across the FULL list including globals — so an on-page 'Comment 2' could be '## 3.' in the feedback the agent reads. The sync now derives each marker's number from its position in the full createdA-sorted list (globals occupy a number but ship no entry, leaving the correct gaps on-page). Export format is unchanged. New buildSyncNumbering helper + tests asserting a mixed list yields identical numbers between the sync payload and exportAnnotations output (mutation-verified against the pre-fix ordering). * chore: sync stale workspace versions in bun.lock (0.26.1 -> 0.26.7) * docs: document raw-HTML overlay model, multi-target types, and known limitations - Data Types: add htmlAdditionalTargets to the Annotation listing plus the HtmlElementAnchor (including the optional normalized point used by placed markers) and HtmlAnnotationTarget shapes. - Annotation System: describe the post-#1257 raw-HTML surface (placed comment markers + overlay-projected highlights, no inline mark mutation; durable anchors persisted, disposable markers projected) and the print-parity limitation. - URL Sharing: note that share links intentionally drop HTML element anchors and additional targets (restore is text-search based, per sharing.multiTarget.test.ts). * test: fix Range.getClientRects stub typing in the B1 cull test * fix(annotate): hover-race teardown and unbounded one-shot dead-search passes Polish round on the overlay hardening: - Hover race (1): switching into pinpoint mode (or opening a draft) now tears hover down fully via clearHoverHighlight() — cancels the pending rAF hit test and clears the tracked position and id — and the rAF callback itself refuses to paint outside drag mode / with an open draft. Previously the pending callback re-applied the class after the mode switch and every flushQueuedHighlights re-painted it from the stale hoverHighlightId, leaving a permanent phantom hover. - One-shot budgets (3): beginDeadSearchPass takes a per-pass budget. Reconcile passes keep 2 (they repeat, skipped targets get follow-up frames); print and scroll-to are user-initiated one-shots with no follow-up and now run unbounded (backoff and generation gates still apply), so printing with 3+ dead-but-recoverable targets no longer silently prints fewer highlights. Both changes carry new regression tests, mutation-verified (fix reverted, test observed failing, fix restored). * fix(annotate): number markers by array position and cap entries after dropping globals The createdA sort made the export-match invariant false with external annotations: exportAnnotations' sort keys tie for every raw-HTML annotation (blockId '', startOffset 0), so its stable sort numbers the combined [...local, ...external] list in ARRAY order — and external annotations arrive appended with server-stamped createdA values that can interleave with local timestamps. buildSyncNumbering now numbers by array position of the input (verified to be the same combined list both consumers receive from packages/editor/App.tsx allAnnotations; the viewerAnnotations diffContext filter is order-preserving and vacuous on the raw-HTML surface). Also reorders the cap: number the full list, drop globals, THEN slice 512 entries — globals no longer waste sync capacity and a non-global the export numbers past position 512 still syncs while slots remain. Numbers may now exceed 512 (array positions); the bridge's own bound (100000) accepts them and its 512-entry cap still agrees with the sender. Tests updated: interleaved-external agreement with exportAnnotations (mutation-verified against the createdA sort) and slice-after-filter capacity. * docs(opencode): note the accepted cache-hint flattening trade-off in V2 consolidation
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
1 participant
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.
TLDR: Raw-HTML annotate no longer writes highlight state into the page's DOM. Annotations are now rendered as placed comment markers: numbered bubble buttons projected into a fixed, pointer-transparent overlay, positioned at the exact point you selected, re-resolved live as the page scrolls, mutates, or reflows. This fixes both reported highlight bugs (partial blue on multi-paragraph selections, and layout breakage on some pages) at their shared root cause, because nothing mutates author content anymore.
What changed
HtmlElementAnchor, extended with an optional normalizedpointcapturing where inside the target you clicked). The visible bubble is a projection computed fresh every frame from the re-resolved live target. Nothing ever restores stored screen coordinates.documentElement(pointer-events: none, maximal z-index) contains highlight rects and marker buttons. Only marker buttons accept pointer input; the page never reflows or shifts.<button>s with Plannotator-branded SVG speech bubbles, accessible labels (Comment N), centered on the selected relative point viatranslate(-50%, -50%). Grouped (shift-click multi-target) annotations show the same number on every target. Clicking a marker, or the highlighted text itself, selects the annotation in the panel.getClientRects, clip-tested, with a containment filter that removes double-painted border boxes on multi-block redlines.beforeprint; markers stay print-hidden like the old badges.sync-annotationsmessage (parent-authoritative numbering, bounded 512 both sides, shape-validated); anchorpointvalidated fail-closed on both sides;mark-clickids capped at 256. The point field is placement-only and cannot influence element resolution.What did not change
Review process
Three adversarial review lenses (geometry/lifecycle, protocol/security against hostile documents, regressions/test honesty) produced 6 majors and 12 minors; all were fixed, then a dedicated verification pass audited the fix commits themselves and found 1 major and 3 minors, also fixed. 28 targeted mutations were used to prove the new regression tests fail against broken code.
Validation
bun run typecheckcleanbun test: 3144 pass, 0 failDOM_TESTS=1): 175 + 6 + 8 pass, 0 fail; srcdoc suite 39 to 66 tests, protocol suite 30 to 40${)Known trade-offs, documented in code: happy-dom cannot exercise real multi-line client rects or
elementFromPoint, so a manual browser pass on pages with nested scrollers, sticky targets, and zoom is recommended before release; annotations past 512 fall back to registration-order numbering.