Repository navigation
fix(gui): portal select dropdowns to avoid clipping #393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6e8190b
cc08705
e3d6a86
33aaa5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import type { CSSProperties } from "react"; | ||
|
|
||
| export interface SelectMenuTriggerRect { | ||
| top: number; | ||
| bottom: number; | ||
| left: number; | ||
| right: number; | ||
| width: number; | ||
| height: number; | ||
| } | ||
|
|
||
| export interface SelectMenuStyleOptions { | ||
| align?: "left" | "right"; | ||
| placement?: "below" | "right"; | ||
| menuHeight?: number; | ||
| } | ||
|
|
||
| const MENU_GAP_PX = 4; | ||
| const FLIP_GAP_PX = 8; | ||
| const VIEWPORT_PAD_PX = 8; | ||
| const MAX_MENU_HEIGHT_PX = 280; | ||
| const MIN_MENU_HEIGHT_PX = 120; | ||
| const BESIDE_MIN_WIDTH_PX = 160; | ||
|
|
||
| function viewportHeight() { | ||
| return typeof window !== "undefined" ? window.innerHeight : 800; | ||
| } | ||
|
|
||
| function viewportWidth() { | ||
| return typeof window !== "undefined" ? window.innerWidth : 1024; | ||
| } | ||
|
|
||
| export function computeSelectMenuStyle( | ||
| trigger: SelectMenuTriggerRect, | ||
| { align = "left", placement = "below", menuHeight = MAX_MENU_HEIGHT_PX }: SelectMenuStyleOptions = {}, | ||
| ): CSSProperties { | ||
| const measuredHeight = Math.min(Math.max(menuHeight, MIN_MENU_HEIGHT_PX), MAX_MENU_HEIGHT_PX); | ||
| const vh = viewportHeight(); | ||
| const vw = viewportWidth(); | ||
|
|
||
| if (placement === "right") { | ||
| const spaceBelow = vh - trigger.top - VIEWPORT_PAD_PX; | ||
| const spaceAbove = trigger.top - VIEWPORT_PAD_PX; | ||
| const openAbove = measuredHeight + FLIP_GAP_PX > spaceBelow && spaceAbove > spaceBelow; | ||
| const left = Math.max(VIEWPORT_PAD_PX, Math.min(trigger.right + 6, vw - BESIDE_MIN_WIDTH_PX - VIEWPORT_PAD_PX)); | ||
|
|
||
| if (openAbove) { | ||
| return { | ||
| position: "fixed", | ||
| left, | ||
| bottom: vh - trigger.top + FLIP_GAP_PX, | ||
| minWidth: BESIDE_MIN_WIDTH_PX, | ||
| maxHeight: Math.max(MIN_MENU_HEIGHT_PX, Math.min(MAX_MENU_HEIGHT_PX, trigger.top - VIEWPORT_PAD_PX - MENU_GAP_PX)), | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| position: "fixed", | ||
| top: trigger.top, | ||
| left, | ||
| minWidth: BESIDE_MIN_WIDTH_PX, | ||
| maxHeight: Math.max(MIN_MENU_HEIGHT_PX, Math.min(MAX_MENU_HEIGHT_PX, vh - trigger.top - VIEWPORT_PAD_PX)), | ||
| }; | ||
| } | ||
|
|
||
| const width = Math.max(trigger.width, 0); | ||
| const spaceBelow = vh - trigger.bottom - VIEWPORT_PAD_PX; | ||
| const spaceAbove = trigger.top - VIEWPORT_PAD_PX; | ||
| const flipUp = measuredHeight + MENU_GAP_PX > spaceBelow && spaceAbove > spaceBelow; | ||
|
|
||
| if (flipUp) { | ||
| const style: CSSProperties = { | ||
| position: "fixed", | ||
| bottom: vh - trigger.top + FLIP_GAP_PX, | ||
| minWidth: width, | ||
| maxHeight: Math.max(MIN_MENU_HEIGHT_PX, spaceAbove - MENU_GAP_PX), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a portaled select with many options opens near the bottom, this branch can return nearly the entire space above the trigger (for example, 688px), overriding the existing 280px CSS cap and the Useful? React with 👍 / 👎. |
||
| }; | ||
| if (align === "right") { | ||
| style.right = vw - trigger.right; | ||
| } else { | ||
| style.left = Math.max(VIEWPORT_PAD_PX, Math.min(trigger.left, vw - VIEWPORT_PAD_PX - width)); | ||
| } | ||
| return style; | ||
| } | ||
|
|
||
| const style: CSSProperties = { | ||
| position: "fixed", | ||
| top: trigger.bottom + MENU_GAP_PX, | ||
| minWidth: width, | ||
| maxHeight: Math.max(MIN_MENU_HEIGHT_PX, Math.min(MAX_MENU_HEIGHT_PX, spaceBelow - MENU_GAP_PX)), | ||
| }; | ||
| if (align === "right") { | ||
| style.right = vw - trigger.right; | ||
| } else { | ||
| style.left = Math.max(VIEWPORT_PAD_PX, Math.min(trigger.left, vw - VIEWPORT_PAD_PX - width)); | ||
| } | ||
| return style; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| /* Shared UI primitives built on the design-system classes in styles.css. */ | ||
| import { useEffect, useRef, useState, type CSSProperties, type ReactNode } from "react"; | ||
| import { useCallback, useEffect, useLayoutEffect, useRef, useState, type CSSProperties, type ReactNode } from "react"; | ||
| import { createPortal } from "react-dom"; | ||
| import { IconCheck, IconAlert } from "./icons"; | ||
| import { IconChevron } from "./icons"; | ||
| import { computeSelectMenuStyle } from "./select-position"; | ||
|
|
||
| export function Switch({ on, onClick, disabled, label }: { on: boolean; onClick: () => void; disabled?: boolean; label?: string }) { | ||
| return ( | ||
|
|
@@ -23,7 +25,7 @@ export function Notice({ tone, children }: { tone: "ok" | "err"; children: React | |
|
|
||
| export interface SelectOption { value: string; label: React.ReactNode } | ||
|
|
||
| export function Select({ value, options, onChange, disabled, label, style, align, placement, dropdownStyle }: { | ||
| export function Select({ value, options, onChange, disabled, label, style, align, placement, dropdownStyle, portal = false }: { | ||
| value: string; | ||
| options: SelectOption[]; | ||
| onChange: (value: string) => void; | ||
|
|
@@ -33,23 +35,91 @@ export function Select({ value, options, onChange, disabled, label, style, align | |
| align?: "left" | "right"; | ||
| placement?: "below" | "right"; | ||
| dropdownStyle?: CSSProperties; | ||
| portal?: boolean; | ||
| }) { | ||
| const [open, setOpen] = useState(false); | ||
| const [menuStyle, setMenuStyle] = useState<CSSProperties | undefined>(); | ||
| const ref = useRef<HTMLDivElement>(null); | ||
| const triggerRef = useRef<HTMLButtonElement>(null); | ||
| const menuRef = useRef<HTMLDivElement>(null); | ||
| const current = options.find(o => o.value === value); | ||
|
|
||
| const reposition = useCallback((menuHeight?: number) => { | ||
| if (!portal) return; | ||
| const trigger = triggerRef.current; | ||
| if (!trigger) return; | ||
| setMenuStyle(computeSelectMenuStyle(trigger.getBoundingClientRect(), { | ||
| align, | ||
| placement, | ||
| menuHeight, | ||
| })); | ||
| }, [align, placement, portal]); | ||
|
|
||
| useEffect(() => { | ||
| if (!open) return; | ||
| const close = (e: MouseEvent) => { if (ref.current && !ref.current.contains(e.target as Node)) setOpen(false); }; | ||
| const close = (e: MouseEvent) => { | ||
| const target = e.target as Node; | ||
| if (ref.current?.contains(target) || menuRef.current?.contains(target)) return; | ||
| setOpen(false); | ||
| }; | ||
| const esc = (e: KeyboardEvent) => { if (e.key === "Escape") setOpen(false); }; | ||
| document.addEventListener("mousedown", close); | ||
| document.addEventListener("keydown", esc); | ||
| return () => { document.removeEventListener("mousedown", close); document.removeEventListener("keydown", esc); }; | ||
| }, [open]); | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!open || !portal) return; | ||
| reposition(); | ||
| const onViewportChange = () => reposition(menuRef.current?.offsetHeight); | ||
| window.addEventListener("resize", onViewportChange); | ||
| window.addEventListener("scroll", onViewportChange, true); | ||
| return () => { | ||
| window.removeEventListener("resize", onViewportChange); | ||
| window.removeEventListener("scroll", onViewportChange, true); | ||
| }; | ||
| }, [open, options.length, portal, reposition]); | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (!open || !portal || !menuRef.current || !triggerRef.current) return; | ||
| const nextHeight = menuRef.current.offsetHeight; | ||
| if (!nextHeight) return; | ||
| const nextStyle = computeSelectMenuStyle(triggerRef.current.getBoundingClientRect(), { | ||
| align, | ||
| placement, | ||
| menuHeight: nextHeight, | ||
| }); | ||
| setMenuStyle(prev => { | ||
| if (prev?.top === nextStyle.top && prev?.bottom === nextStyle.bottom && prev?.maxHeight === nextStyle.maxHeight) return prev; | ||
| return nextStyle; | ||
| }); | ||
| }, [align, open, options.length, placement, portal]); | ||
|
|
||
| const dropdown = open ? ( | ||
| <div | ||
| ref={menuRef} | ||
| className={`select-dropdown${portal ? " select-dropdown-portal" : ""}${align === "right" ? " select-dropdown-right" : ""}${placement === "right" ? " select-dropdown-beside" : ""}`} | ||
| role="listbox" | ||
| aria-label={label} | ||
| style={portal ? { ...menuStyle, zIndex: 60, ...dropdownStyle } : dropdownStyle} | ||
| > | ||
| {options.map(o => ( | ||
| <button | ||
| key={o.value} | ||
| type="button" | ||
| role="option" | ||
| aria-selected={o.value === value} | ||
| className={`select-option${o.value === value ? " active" : ""}`} | ||
| onClick={() => { onChange(o.value); setOpen(false); }} | ||
| >{o.label}</button> | ||
| ))} | ||
| </div> | ||
| ) : null; | ||
|
|
||
| return ( | ||
| <div ref={ref} className="custom-select" style={{ position: "relative", display: "inline-block", ...style }}> | ||
| <button | ||
| ref={triggerRef} | ||
| type="button" | ||
| className="select-trigger" | ||
| onClick={() => !disabled && setOpen(o => !o)} | ||
|
|
@@ -61,20 +131,7 @@ export function Select({ value, options, onChange, disabled, label, style, align | |
| <span>{current?.label ?? value}</span> | ||
| <IconChevron style={{ width: 12, height: 12, color: "var(--muted)", transform: open ? "rotate(90deg)" : "none", transition: "transform .12s" }} /> | ||
| </button> | ||
| {open && ( | ||
| <div className={`select-dropdown${align === "right" ? " select-dropdown-right" : ""}${placement === "right" ? " select-dropdown-beside" : ""}`} role="listbox" aria-label={label} style={dropdownStyle}> | ||
| {options.map(o => ( | ||
| <button | ||
| key={o.value} | ||
| type="button" | ||
| role="option" | ||
| aria-selected={o.value === value} | ||
| className={`select-option${o.value === value ? " active" : ""}`} | ||
| onClick={() => { onChange(o.value); setOpen(false); }} | ||
| >{o.label}</button> | ||
| ))} | ||
| </div> | ||
| )} | ||
| {portal ? (dropdown && createPortal(dropdown, document.body)) : dropdown} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a keyboard user activates one of the new portaled Claude selects, focus remains on the trigger, while this portal places the option buttons at the end of Useful? React with 👍 / 👎. |
||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import { afterEach, beforeEach, expect, test } from "bun:test"; | ||
| import { Window } from "happy-dom"; | ||
| import { act } from "react"; | ||
| import type { Root } from "react-dom/client"; | ||
| import { Select } from "../src/ui"; | ||
|
|
||
| // Mounted regressions for #340 / PR #393: the portal fix must (1) render an opt-in portaled | ||
| // dropdown under document.body (outside the clipping card) with fixed positioning, and (2) leave a | ||
| // NON-portal Select (the language menu contract) as a descendant of .custom-select keeping the | ||
| // .select-dropdown-beside class so its glass fallback + mobile upward-placement CSS still apply. | ||
|
|
||
| const globals = ["document", "window", "navigator", "IS_REACT_ACT_ENVIRONMENT"] as const; | ||
| let previousGlobals: Record<(typeof globals)[number], unknown>; | ||
| let testWindow: Window; | ||
|
|
||
| beforeEach(() => { | ||
| previousGlobals = Object.fromEntries(globals.map((key) => [key, Reflect.get(globalThis, key)])) as typeof previousGlobals; | ||
| testWindow = new Window({ url: "http://localhost/" }); | ||
| Object.defineProperties(globalThis, { | ||
| document: { configurable: true, value: testWindow.document }, | ||
| window: { configurable: true, value: testWindow }, | ||
| navigator: { configurable: true, value: testWindow.navigator }, | ||
| }); | ||
| (globalThis as typeof globalThis & { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| testWindow.close(); | ||
| for (const key of globals) { | ||
| Object.defineProperty(globalThis, key, { configurable: true, value: previousGlobals[key] }); | ||
| } | ||
| }); | ||
|
|
||
| const OPTIONS = [ | ||
| { value: "a", label: "Alpha" }, | ||
| { value: "b", label: "Beta" }, | ||
| ]; | ||
|
|
||
| async function mountAndOpen(node: React.ReactElement): Promise<{ container: HTMLElement; root: Root }> { | ||
| const { createRoot } = await import("react-dom/client"); | ||
| // A bounded, clipping card the Claude settings dropdowns live inside. | ||
| const card = document.createElement("div"); | ||
| card.className = "settings-card"; | ||
| card.style.overflow = "hidden"; | ||
| document.body.append(card); | ||
| let root!: Root; | ||
| await act(async () => { | ||
| root = createRoot(card); | ||
| root.render(node); | ||
| }); | ||
| // Open the dropdown by clicking the trigger. | ||
| const trigger = card.querySelector<HTMLButtonElement>("button.select-trigger"); | ||
| await act(async () => { trigger?.dispatchEvent(new testWindow.MouseEvent("click", { bubbles: true }) as unknown as MouseEvent); }); | ||
| return { container: card, root }; | ||
| } | ||
|
|
||
| test("a portal Select renders its dropdown under document.body, outside the clipping card", async () => { | ||
| const { container, root } = await mountAndOpen( | ||
| <Select value="a" options={OPTIONS} onChange={() => {}} label="Backend" portal />, | ||
| ); | ||
| const listbox = document.body.querySelector<HTMLElement>('[role="listbox"]'); | ||
| expect(listbox).not.toBeNull(); | ||
| // The dropdown must NOT be nested inside the clipping card (that's the whole point of #340). | ||
| expect(container.contains(listbox)).toBe(false); | ||
| // Portaled dropdowns get the portal marker class and fixed positioning. | ||
| expect(listbox?.className).toContain("select-dropdown-portal"); | ||
| await act(async () => { root.unmount(); }); | ||
| }); | ||
|
|
||
| test("a non-portal Select (language-menu contract) stays inside .custom-select and keeps .select-dropdown-beside", async () => { | ||
| const { container, root } = await mountAndOpen( | ||
| <Select value="a" options={OPTIONS} onChange={() => {}} label="Language" placement="right" />, | ||
| ); | ||
| // Not portaled: nothing lands directly under body outside the mount container. | ||
| const bodyListboxes = Array.from(document.body.querySelectorAll<HTMLElement>('[role="listbox"]')); | ||
| expect(bodyListboxes.length).toBe(1); | ||
| const listbox = bodyListboxes[0]; | ||
| // It remains a descendant of the .custom-select wrapper (contextual glass + mobile rules apply). | ||
| expect(container.querySelector(".custom-select")?.contains(listbox)).toBe(true); | ||
| expect(listbox.className).toContain("select-dropdown-beside"); | ||
| expect(listbox.className).not.toContain("select-dropdown-portal"); | ||
| await act(async () => { root.unmount(); }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the viewport logic chooses the flip-up path, the returned style only sets
bottom, but the same element still has.select-dropdown { top: calc(100% + 4px) }(or.select-dropdown-beside { top: 0 }) from CSS because the portal class only changesposition. With bothtopandbottomapplied, dropdowns near the bottom will be laid out at the stale top offset or stretched instead of appearing above the trigger; settop: "auto"in the flipped style (and similarly clear conflicting offsets) so the upward placement actually takes effect.Useful? React with 👍 / 👎.