Bugfix/issue 384 viewport clamping#401
Conversation
…opover hover The handleMouseEnter callback was mutating the active text selection range whenever the user moved their cursor over the popup. This caused two bugs: - Issue QuoteVote#382: The highlighted text grew/jumped unexpectedly on hover. - Violated the acceptance criteria for QuoteVote#384 which requires no regressions. Removes the handler entirely so hovering the popover is a no-op with respect to the active selection. Updates the regression test to assert that onSelect is NOT called on mouseenter. All other positioning/clamping/portal logic for RC1-032 is retained.
|
@OtavioXimarelli is attempting to deploy a commit to the Louis Girifalco's projects Team on Vercel. A member of the Team first needs to authorize it. |
motirebuma
left a comment
There was a problem hiding this comment.
PR #401 Review — Viewport Clamping for Highlight Popup
Nice fix! The root cause was clear and the solution is clean. Here are a few quick thoughts:
The good stuff
- Portal via
createPortalis exactly the right move — escapes all those peskyoverflow: hiddenancestors. - The new single-algorithm positioning is so much cleaner than the old 3-branch desktop/tablet/mobile split.
- Removing
handleMouseEnterwas the right call — mutating a liveRangeon hover was always going to cause trouble. - SSR guard with
mountedstate is necessary and correct fordocumentaccess. - Tests updated correctly —
document.querySelectorinstead ofcontainer.querySelectormakes sense for portals.
Small things to consider
- The double
requestAnimationFrametrick works, but a quick comment explaining why two frames would help the next person reading this. - The
eslint-disablecomment onsetMountedinsideuseEffectlooks unnecessary —setStatein effects is totally normal.
Before merging
⚠️ This PR has a merge conflict and needs a rebase ontomainbefore it can land.
Verdict: ✅ Approve (after rebase) — solid work, good test coverage, no regressions.
|
@OtavioXimarelli It was a simple merge conflict, and I resolved it for you. This one is ready to merge. Thank you!. |
There was a problem hiding this comment.
Pull request overview
Moves the text-selection highlight popover to render in a document.body portal and updates its positioning logic so it clamps horizontally within the viewport and flips vertically when there isn’t enough space above the selection, addressing overflow clipping and off-screen rendering.
Changes:
- Render
SelectionPopoverviacreatePortal(..., document.body)with page-absolute positioning (scrollX/scrollY) and viewport clamping. - Add client-mount guarding plus scroll/resize-driven repositioning while the popover is visible.
- Update unit tests to query the portal-rendered popover from
documentrather than the RTL container.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| quotevote-frontend/src/components/VotingComponents/SelectionPopover.tsx | Portals the popover to document.body and replaces legacy positioning branches with viewport-aware clamping + vertical flip, including scroll/resize recomputation. |
| quotevote-frontend/src/tests/components/VotingComponents/SelectionPopover.test.tsx | Adjusts tests to locate the popover in document.body after portal rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Run on next frames to handle potential layout adjustments and avoid cascading renders lint rule | ||
| const rafId1 = requestAnimationFrame(computePopoverBox) | ||
| const rafId2 = requestAnimationFrame(() => requestAnimationFrame(computePopoverBox)) | ||
|
|
||
| window.addEventListener('resize', computePopoverBox) | ||
| window.addEventListener('scroll', computePopoverBox, { passive: true }) | ||
|
|
||
| return () => { | ||
| cancelAnimationFrame(rafId1) | ||
| cancelAnimationFrame(rafId2) | ||
| window.removeEventListener('resize', computePopoverBox) | ||
| window.removeEventListener('scroll', computePopoverBox) | ||
| } |
| it('applies custom topOffset', () => { | ||
| const { container } = render( | ||
| render( | ||
| <SelectionPopover {...defaultProps} showPopover={true} topOffset={50} />, | ||
| ) | ||
| const popover = container.querySelector('#selectionPopover') | ||
| const popover = document.querySelector('#selectionPopover') | ||
| expect(popover).toBeInTheDocument() | ||
| }) |
PR — RC1-032: Render highlight popup in portal and clamp within viewport
Branch:
bugfix/issue-384-viewport-clamping→mainCloses: #384
Labels:
bugrc1highlight-popuplayoutfrontendProblem
The highlight popup was rendered as a regular child element inside
SelectionPopover's DOM parent. This caused two distinct bugs:1. Overflow clipping
Any ancestor with
overflow: hidden(present in multiple layout containers) clipped the popup at the parent's edge. Selecting text near a container boundary made the popup appear cut off or invisible.2. No viewport clamping
The old
computePopoverBoxpositioned the popup relative to the[data-selectable]element's bounding box without checking viewport edges. Selecting text near the left or right edge caused the popup to render partially or fully off-screen.The old positioning used three separate layout branches (desktop / tablet right-side / mobile) with coordinates relative to the parent container, all of which could still overflow the viewport:
There was also a
handleMouseEntercallback (introduced in the same branch) that mutated the live selection range on hover — a regression against the no-regressions acceptance criterion.Fix
SelectionPopover.tsx— four concrete changes:1. SSR guard (
mountedstate)Added a
mountedstate flag set in auseEffect. The component returnsnullon the server to avoiddocumentaccess during SSR:2. Portal rendering
The popup div is now rendered via
createPortal(…, document.body), escaping all ancestor overflow constraints. It overlays the full page regardless of the DOM hierarchy:3. Unified viewport-aware positioning with horizontal clamping and vertical flip
Replaced the three-branch layout with a single algorithm that works at all viewport widths:
Coordinates are now in page-absolute space (
viewport coord + scrollX/Y) so the absolutely-positioned portal element tracks the selection correctly even on scrolled pages.4. Resize and scroll recalculation
While the popup is visible,
computePopoverBoxis re-run viarequestAnimationFrameon the next two frames (to allow the portal to paint and report its measured size), and on everyresizeand passivescrollevent:5. Removed
handleMouseEnterRemoved the
handleMouseEntercallback andonMouseEnterprop that were introduced in the first commit on this branch. The callback mutated the activeRangeon hover, which is the root cause of #382 and a direct regression against this issue's acceptance criterion.SelectionPopover.test.tsx— two targeted changes:container.querySelector('#selectionPopover')→document.querySelector('#selectionPopover')throughout, because the popup now renders intodocument.bodyvia the portal and is not a child of the React test container.'handles mouse enter to expand selection'(which asserted the buggy behavior withtoBeDefined()) with'does not alter selection or call onSelect when mouse enters the popover'assertingexpect(onSelect).not.toHaveBeenCalled().Test results
Acceptance criteria
document.bodyportalscrollX/scrollYoffsets applied)resizeandscrollwhile visiblenulluntil mounted on the client