fix: anchor the context popup to the visible part of its selection - #169
Merged
Conversation
The popup positioned against a fixed span measured once when it opened, so it drifted from its text as the document scrolled and, for a selection taller than the viewport, landed past the top of the editor. A virtual anchor measuring the live selection replaces it. Floating UI re-queries that anchor through its context element, which is also what resolves the scroll ancestors it listens on. The scroll listener that closed the popup existed only to hide the stale anchor, so it goes with it.
Scrolling the selection away used to close the popup, discarding a command surface for a selection that is still live. The anchor now reports the selection where it actually is once none of it is visible, so Floating UI's referenceHidden hides the popup and returns it when the selection scrolls back; open state, focus, and the selection are all untouched. A popup the user is working in pins to a rect inside the viewport instead, so a scroll can neither hide it nor move it out from under a keyboard interaction. A keyboard popup pins from the moment it opens rather than from the focus it is about to take. happy-dom performs no layout, so the document element measured 0x0 and every reference read as fully clipped. The test setup now reports the window size there, which is what makes any of this assertable.
A selection taller than the visible area has no outside within reach, and one that fills the visible area leaves no room on either side. Both now anchor the popup to the first line of the selection the reader can see, so it lands inside the selection rather than below its last visible line, above its first, or past an edge that collision handling only pushes it further out of. The anchor also keeps a pixel of height. Floating UI reads a rect of no height resting on the clipping edge as fully clipped, so a selection spanning the whole visible area was hiding the popup outright, and a pinned anchor for a selection scrolled out of view would have hidden it too.
8 tasks
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
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
The context popup anchored to a
position: fixedspan measured once when it opened. Three defects followed from that one model, and this replaces the model rather than patching each.contextElement, which is also what resolves the scroll ancestors it listens on — without it no listener is attached at all anddetectOverflowfalls back to the document element.hideWhenDetachedhides the popup instead of closing it. The selection, the open state, and any focus the popup holds all survive, and the popup returns when the selection scrolls back.The scroll listener that closed the popup existed only to hide the stale anchor, and goes with it.
docs/specification.mdanddocs/reference.mdlose the focus-dependent scroll rule they stated.Related Issue
Closes #163
Verification
pnpm check:frontendon each of the three commits, green throughout. Final run: 95 files, 1008 tests, coverage 90.73 / 81.38 / 92.22 / 92.10 against the 88 / 78 / 89 / 89 floor. Frontend-only change, so backend checks were not run.Tests added or reworked:
src/features/editor/utils/contextPopupAnchor.test.ts(new) — 13 cases over the pure resolver: full and partial overlap, horizontal clamping, the three ways a selection can be too tall to sit beside, a fully visible selection that fills the viewport, and the live/pinned split for a selection with no visible part.src/features/editor/components/EditorContextPopup.test.tsx— the popup hides while its anchor is off screen and returns when it comes back, withoutonClose; a popup holding focus stays visible with an off-screen selection; a pointer popup measuresliveand a keyboard popuppinned; a focused popup reuses one rect across a new request. Both scroll cases now assert the popup stays open.src/features/editor/plugins/contextPopup.test.tsx— the anchor's rect spans the selection's visible ends, re-measures as the selection moves rather than replaying the rect it opened with, and carries the editor as its context element.Reviewer reproduction, in
pnpm tauri devwithcorpus/open on a document longer than the window:Mod+A, thenShift+F10— a keyboard selection does not open the popup on its own, which is Keyboard selection does not open the context popup that the documents promise #164. The popup lands inside the selection. Repeat with the selection scrolled so only its top, only its bottom, or neither edge is visible.Shift+F10on a small selection, then scroll. The focused popup stays put and stays visible.All four steps were confirmed manually on Windows. Steps 1, 2 and 4 passed on the first two commits; step 3 failed there — the popup landed below the last visible line, above the first, or vanished outright depending on which selection edges were on screen — and the third commit is the response to that. Step 3 was re-confirmed against it.
Automated coverage of visibility is possible at all only because
src/test/setup/dom.tsnow reports the window size fordocument.documentElement'sclientWidth/clientHeight. happy-dom performs no layout, so the document element measured 0×0 and Floating UI treated every reference as fully clipped. That setup file applies to every DOM test; the full suite passes with it.Notes
Three findings from manual testing that this pull request deliberately leaves alone. All three predate this branch and none touch anchoring.
DismissableLayer, which listens forfocusinon the document and dismisses when focus lands outside the layer; refocusing the editor qualifies. It was mostly masked by the scroll close. Suppressing it means overridingonFocusOutside, which also suppresses genuine focus-outside dismissals and reopens the focus model settled in Editor context popup cannot be reached or operated by keyboard #120 — worth its own issue.handleDOMEventsonview.domwhile its own drag tracking is root-level. The selection completes correctly; only the auto-open is missed. Filed as Pointer selection released outside the editor does not open the context popup #168.Mod+Aalone shows nothing. That is Keyboard selection does not open the context popup that the documents promise #164, which explicitly defers positioning to this issue.One residual case in scope but unfixed:
Shift+F10pressed while the popup is already hidden focuses a control that is stillvisibility: hiddenfor that tick, so focus does not land. Pinning makes the popup visible from the next reposition on. The clean fix is scrolling the selection into view on a keyboard request, which changes editor behavior beyond what this issue agreed.POPUP_CLEARANCEis the one tuned number here. It decides only whether a selection short enough to sit beside leaves room for the popup, and is sized by eye.