fix(viewer): keep Add-comment popover attached to highlighted text on scroll#326
Open
yumike wants to merge 8 commits into
Open
fix(viewer): keep Add-comment popover attached to highlighted text on scroll#326yumike wants to merge 8 commits into
yumike wants to merge 8 commits into
Conversation
… scroll Previously the popover captured a viewport-fixed rect at mouseup, so any scroll detached it from the selection. Track the Range and re-measure on scroll/resize, mirroring how `observeElement` already does for element anchors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
If live reload or navigation swapped the article DOM while a selection was active, the cached Range's start/end nodes got detached and getBoundingClientRect() returned zeros, briefly jumping the Add-comment popover to the top-left corner. Clear selectionRange when page.data identity changes so the popover dismisses cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The scroll/resize handler reassigned `selectionRect` to a new object on every tick, even when x/y were identical — triggering Svelte to re-run the Popover's reactive bindings during scrolls. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the bespoke scroll/resize listener wiring and equality guard in PageContent with a sibling to useAnchorOffset that tracks a Range's viewport rect. Per-field $state writes give the no-op-skip behavior implicitly, so the equality guard added in dce6249 is gone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nd observeElement Generalize observeElement to accept Element | Range — ResizeObserver attaches only when the target is an Element. useRangeRect now delegates its scroll/resize listeners through observeElement instead of re-implementing the capture-phase scroll + resize pair, removing a near-duplicate of useAnchorOffset's wiring. Also consolidate the makeRange test fixture into the shared __fixtures__/resize-observer-mock module alongside makeAnchor (sharing a fakeRect builder), trim a few change-narration comments, and fix the cloneRange comment in PageContent — the real reason to clone is that Selection.getRangeAt(0) returns a live Range, not anything about \$state.raw identity tracking. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
useRangeRect and useAnchorOffset had identical state shape, identical observeElement wiring, and identical getter return — only the target type differed. Generalize useAnchorOffset over `Element | Range` and delete the duplicate hook. Push the "measured = false when target becomes null" reset into useAnchorOffset itself, so popovers anchored to elements also hide cleanly when their anchor disappears. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`observeElement` now accepts `Element | Range`, so rename it to `observeTarget` to match the contract. Export `fakeRect` from the shared resize-observer mock and consume it in `overlay-testing`'s `mockRect`, removing the inlined DOMRect literal. Extend `makeAnchor` to accept a getter (mirroring `makeRange`) and collapse four hand-rolled `getBoundingClientRect` blocks in the hook tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces a duplicate $effect in useAnchorOffset that re-subscribed to the same getter just to flip `measured` to false. Drops `as unknown as Range` in the test fixture by using a real `document.createRange()`, and converts the remaining inline DOMRect literals in tests to fakeRect / makeAnchor's getter form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
{x, y}snapshot atmouseupand thePopoverprimitive doesn't subscribe to scroll in free mode, so any scroll detached the popover from the selection.Rangeitself (cloned to decouple from the liveSelection) and re-measure onscroll(capture phase, so embedded ancestor scroll containers count) +resize— same listener patternobserveElementalready uses for element anchors.Test plan
npm -w @rwdocs/viewer run check— 0 errors, 0 warningsnpm -w @rwdocs/viewer run test— 361/361 unit tests passnpx playwright test --project=chromium e2e/comments.spec.ts— 19/19 e2e tests pass, including newpopover follows the highlighted text on scrollmake format,make lintclean🤖 Generated with Claude Code