Let the host choose where the context menu renders - #17
Open
rolfheij-sil wants to merge 12 commits into
Open
rolfheij-sil wants to merge 12 commits into
rolfheij-sil wants to merge 12 commits into
Conversation
The context menu portalled to document.body unconditionally, so a host that scales its content could not have the menu scale with it. EditorOptions gains a contextMenuContainer getter, resolved only while the menu is open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
The suite could not tell open-time resolution from a per-render call, so a refactor back to resolving during render would have stayed green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
CSS zoom multiplies a fixed-position element's own left/top, so a menu inside a zoomed container landed at the factor times its intended point. Divide the placement by the container's currentCSSZoom. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
A fixed-position menu is not clipped by a scrolling ancestor, so a menu given a container still had to be bounded deliberately. Narrow the container's rect by every clipping ancestor and the viewport, and clamp to that. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
The stylesheet's 200px list cap is a pre-zoom length, so a scaled menu could outgrow a short pane. Cap the menu at the visible box's height divided by the zoom factor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
Regenerates the committed dist/ and etc/platform-editor.api.md for platform-editor to pick up EditorOptions.contextMenuContainer from the preceding context-menu-container commits, via `nx extract-api` so the rolled-up dist/index.d.ts stays self-contained (no bare workspace imports). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
Two problems with the menu once it portals into a host-supplied container. The menu is `position: fixed`, so its offsets are resolved against the viewport only while nothing above it establishes a containing block. A popover wrapper carrying a `transform` takes that role over, and the menu then opens a wrapper's-worth away from the pointer, often off-window. Detecting such an ancestor is fragile — `filter`, `perspective` and `contain` do it too — so the placement now reads back where the menu actually landed and shifts it by the error, in the container's pre-zoom units. Only the container path pays for the extra read; portalled to `document.body` the placement is written once, as before. The scroll-to-close listener is on the capture phase, so it also received the scrolls the menu's own list raises, closing the menu as soon as the user wheeled towards a clipped item. Scrolls originating inside the menu are now left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
Carries the context menu's placement correction and its scroll guard into the bundle the consuming repo copies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
…measure its scale Two gaps in the container-hosted placement, both found reviewing the change against its own claim of keeping the menu inside the pane. The menu's width was never capped. It is a fixed 200 own-pixels that the zoom multiplies, so it renders 400 wide at 200 % and 600 at 300 %; once that exceeds the pane, the horizontal clamp has nothing left to give, collapses to the box's leading edge, and the remainder paints past it. At the 300 px responsive design target that starts around 150 %. Both axes are now capped. Placement trusted `currentCSSZoom` for the ratio between the menu's own units and rendered pixels. That reports CSS `zoom` only, so a `transform: scale()` anywhere above the menu — an open animation still running, for one — left a residual error the single correction pass could not remove, and on an engine without `currentCSSZoom` the menu landed at the containing block's origin rather than at the pointer: worse than not scaling at all. The ratio is now measured from the one length we wrote ourselves, which sees zoom and transform alike, and the origin is recovered from where the menu actually landed. That solves for the offsets directly instead of correcting, and is exact in one pass for any axis-aligned scale and translation. Also hardened, same pass: - A clipping ancestor with no layout of its own collapsed the visible box, capping the menu to nothing and pinning it to a corner. An empty or inverted box now falls back to the viewport. - The container getter runs after `preventDefault()`, so one that threw left the user with no context menu at all. It falls back to unscaled. - Closing the menu kept the host's element referenced for the plugin's lifetime. `stubMenuLayout` now scales the reported size by the same factor as the position and honours the caps, so the simulation matches the browser it stands in for; without that no test could exercise clamping at zoom other than 1. Three tests added: the width cap, placement under a scaling ancestor, and the empty-box fallback. Each was checked by reverting the line it covers. 1672 pass. `contextMenuContainer` carries the package's EXPERIMENTAL prose marker on both TSDoc surfaces, and documents that the getter must be stable across renders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-URL: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
…gain Re-opening the menu while it is already showing reuses the same element rather than remounting it: the `contextmenu` handler sets a fresh state object whether or not the menu is open, and the component only returns null once it is closed. Reachable from a second right-click and from the keyboard menu key. The element still carried the previous open's `max-width`, so the scale measured from its rendered width read the capped size — 1.5 rather than 2 in a 300px pane at 200 % — which lifted the cap and placed the menu at a negative offset. The same stale caps survived into an open that portals to `document.body`, silently constraining a menu that should not be capped at all. The caps are cleared at the top of the layout effect, before the measurement and before the no-container branch. Covered by a test that opens twice without closing; it reads 200px instead of 150px without the reset. A getter that throws is now also logged rather than only swallowed — silently unscaled looks exactly like the option never being passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-URL: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
…tter Both branches were added without tests. The throwing-getter one is a real gap: reverting the catch makes it red, so nothing pinned the fallback before. The body-portalled case turned out not to work the way the review assumed. Changing the portal target remounts the menu, so a container-hosted open cannot leak its caps onto a later unscaled one — removing the cap reset leaves that path green. The test stays, because the outcome is worth pinning, but it now asserts the remount it actually depends on and says so; the reset itself is covered by the container-to-container re-open test. The three bounding properties are named once, so a cap added later cannot be written without also being cleared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-URL: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
The comment claimed the caps would otherwise survive into an open portalled to `document.body`. They cannot: a change of portal target remounts the menu. The reset exists for the same-container re-open, where a surviving `max-width` would narrow the width the scale is measured from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-URL: https://claude.ai/code/session_019fLmoQjwRthY9GjcBZLpYa
Open
2 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.
The editor's right-click context menu always portalled to
document.body. A host that scales its content (Platform.Bible zooms a pane's content with CSSzoom) therefore got a menu at interface scale beside text twice its size, and the menu was bounded by the window rather than by the pane.EditorOptionsgains an optionalcontextMenuContainergetter naming the element to render into. Inside that element the menu inherits the area's zoom and is bounded by it. Hosts that pass nothing are unaffected.What the plugin does when a container is supplied
zoom.zoommultiplies aposition: fixedelement's ownleft/top, so writing viewport pixels straight would place the menu at the factor times its intended point. The factor is measured, not read fromcurrentCSSZoom: atransform: scale()above the menu multiplies it too andcurrentCSSZoomcannot see that, so the menu's rendered width is compared against the 200px it was written with.position: fixedelement is not clipped by a scrolling ancestor, so staying inside the pane has to be computed rather than inherited.overflow-y: auto. Width matters as much as height: the menu's 200px is fixed, so the zoom multiplies it, and past roughly 150 % in a narrow pane the horizontal clamp has nothing left to give and the surplus paints past the pane's edge.transformon any ancestor (unlikezoom) re-parents the containing block of aposition: fixeddescendant, which is exactly what happens inside a Radix popover. Rather than trying to detect that — it is also caused byfilter,contain,will-changeand others — the menu measures where it went, derives the origin, and solves for the offsets. Exact in one pass for any axis-aligned scale and translation.The getter is resolved once, when the menu opens, inside the
contextmenuhandler, and never during render: an opaque call during render makes the React Compiler abandon optimizing the component.Browser behaviour this rests on
Measured in Chromium rather than assumed:
position: fixedinsidezoom: 2get scaled?left: 100pxrenders at viewport x = 200zoomcreate a containing block for it?transformancestor create one?350/180insidezoom:2insidetranslate(620,310)rendered at 1320/670 instead of 700/360Element.currentCSSZoomavailable?zoom— but not atransform: scale(), which is why placement measures insteadpxlengths scale underzoom?That last row is why the menu's hard-coded
14pxfont and200pxwidth are unchanged: they already scale correctly, and altering them would only restyle the menu at 100 %.Also fixed
Scrolling the menu closed it. The plugin's capture-phase
scrolllistener onwindowalso receives scroll events from descendants, so wheeling a scrollable menu dismissed it and made clipped items unreachable by mouse. Scrolls originating inside the menu are now ignored; scrolls elsewhere still close it. This was partly pre-existing — the inner list has always been scrollable — so the fix helps unzoomed hosts too.Note for reviewers: the generated diff
packages/platform/dist/andpackages/platform/etc/platform-editor.api.mdare committed build output, regenerated withnx extract-api platform-editor.dist/index.jsshows roughly 5k changed lines for a small source change — that is bundler identifier churn, not hand-written code.verify-committed-dist.mjsrebuilds and byte-compares it, and passes.Tests
ContextMenuPluginhad no test file; it now has 18 tests. Each asserts exact pixel values rather than inequalities, and each was proven to fail against the line it covers before being kept. Fullshared-reactsuite green (1672 passed).The layout stub scales the size it reports by the same factor as the position and honours the caps the code writes, so it models the browser rather than restating the implementation's own assumption — without that, no test could exercise clamping at a zoom other than 1. The stub is still a model: that CSS
zoomscales aposition: fixeddescendant this way is established by the Chromium measurements above and by hand testing, not by the suite.Verified by hand at 200 %: the menu scales, lands at the pointer, stays inside the pane in a split layout, and behaves inside the footnote editor pop-up. The width cap and the measured-scale placement landed after that hand test and need re-checking, in particular in a narrow pane at 200 % and above.
Known, not addressed here
The menu's list carries the stylesheet's own
max-height: 200pxwith its scrollbar hidden (scrollbar-width: none), and at ~32px an item a 7-item menu already exceeds it at 100 % zoom — so the last item is reachable only by wheel, with no scrollbar to show it. That is pre-existing and independent of this change; the height cap here adds a second, visible scroll region on top of it in a short pane. Arrow-key navigation also does not scroll the selected item into view, same pre-existing cause.And one the width cap introduces: the list items carry
min-width: 180px, so where the cap lands below roughly 196 of the menu's own pixels — a pane under about 390px at 200 % — the items cannot shrink to fit and the labels are clipped behind a horizontal scrollbar instead. Keeping the menu inside the pane is still the better of the two failures, and letting the items shrink means changing the shared stylesheet, which reaches hosts that pass no container at all.AI-assisted — session
🤖 Generated with Claude Code
This change is