Skip to content

Commit 610b6e6

Browse files
committed
docs(combobox): add openspec change for menu jump fix (WC-3406)
1 parent 55543fe commit 610b6e6

5 files changed

Lines changed: 238 additions & 0 deletions

File tree

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
schema: tdd-refactor
2+
created: 2026-07-09
Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
## Test Cases
2+
3+
<!--
4+
Unit tests use Jest + RTL and mock `hooks/useFloatingMenu` via
5+
`src/hooks/__mocks__/useFloatingMenu.ts` (the mock returns stable refs + a marker
6+
style so specs don't depend on real layout). The real middleware behavior (flip,
7+
size/shrink) is not exercisable in jsdom (no layout), so it is verified by e2e /
8+
manual showcase rather than unit tests. Unit tests assert the WIRING and the
9+
alwaysOpen bypass; e2e asserts the observable no-jump / shrink behavior.
10+
-->
11+
12+
### Reproduction Tests
13+
14+
- Menu wiring uses floating-ui, not the old style hook - `useMenuStyle` is gone and the
15+
wrapper is positioned via floating refs (unit)
16+
- **Given**: `SingleSelection` rendered open with a mocked `useFloatingMenu`
17+
- **When**: component mounts and the menu opens
18+
- **Then**: the menu wrapper (`.widget-combobox-menu`) receives the floating ref and the
19+
`floatingStyles` from the hook; no import of `useMenuStyle` remains in the module graph
20+
21+
- Menu placement does not oscillate near the viewport bottom (e2e)
22+
- **Given**: a Combobox positioned so there is less space below than the menu's natural
23+
height, with enough options to fill the menu
24+
- **When**: the menu is opened and left open
25+
- **Then**: the menu settles in a single placement within one animation frame and its
26+
`top` (bounding rect) does not change across subsequent frames (no flip-flop)
27+
28+
- Menu does not jump when the option count changes while open (e2e)
29+
- **Given**: an open Combobox near the viewport bottom with a text filter
30+
- **When**: the user types to reduce, then clear, the number of matching options
31+
- **Then**: the menu re-anchors smoothly and does not flip between above/below the input
32+
33+
### Edge Cases
34+
35+
- Menu shrinks and scrolls instead of overflowing when space is tight (e2e)
36+
- **Given**: a Combobox with many options and limited space below it
37+
- **When**: the menu opens
38+
- **Then**: the menu height is capped to the available space (`maxHeight <= availableHeight`),
39+
the option list scrolls internally, and the menu bottom stays within the viewport
40+
(respecting the 8px padding)
41+
42+
- Menu width matches the input width (unit + e2e)
43+
- **Given**: a Combobox of a known input-container width, menu open
44+
- **When**: floating-ui's `size` middleware applies
45+
- **Then**: the menu wrapper width equals the reference (input container) width
46+
47+
- Menu header and footer remain visible when the menu shrinks (e2e)
48+
- **Given**: a Combobox configured with `menuHeaderContent` and `menuFooterContent`, in a
49+
tight space so the menu shrinks
50+
- **When**: the menu opens
51+
- **Then**: header and footer are both fully visible; only the option list scrolls; footer
52+
is not clipped below the fold
53+
54+
- Menu does not flash before it is positioned (e2e)
55+
- **Given**: a Combobox opened
56+
- **When**: the first render occurs before floating-ui reports `isPositioned`
57+
- **Then**: the menu is `visibility: hidden` until positioned (no visible jump-from-origin)
58+
59+
### Regression Tests
60+
61+
- alwaysOpen (`keepMenuOpen`) renders inline and does not use floating positioning (unit)
62+
- **Given**: `SingleSelection` and `MultiSelection` rendered with `keepMenuOpen`
63+
- **When**: the menu renders
64+
- **Then**: the wrapper style is `position: relative` (inline block), it does NOT carry the
65+
floating ref/`floatingStyles`, and `useFloatingMenu` is called with `open = false`
66+
67+
- MultiSelection wires floating identically to SingleSelection (unit)
68+
- **Given**: `MultiSelection` rendered open (not alwaysOpen)
69+
- **When**: it mounts
70+
- **Then**: reference ref is on the input container, floating ref + styles are on the menu
71+
wrapper — same contract as `SingleSelection`
72+
73+
- Lazy-loading scroll still works (unit)
74+
- **Given**: a Combobox with `lazyLoading` enabled and `hasMore` options, menu open
75+
- **When**: the option list is scrolled
76+
- **Then**: the `onScroll` handler fires and the `widget-combobox-menu-lazy-scroll` class is
77+
applied to the list — unchanged from current behavior
78+
79+
- Existing Single/Multi/Static selection specs pass with the new mock (unit)
80+
- **Given**: the existing `SingleSelection.spec`, `MultiSelection.spec`, `StaticSelection.spec`
81+
- **When**: run against the floating-ui implementation with `useFloatingMenu` mocked
82+
- **Then**: all previously-passing assertions still pass (menu render, item selection,
83+
open/close, a11y attributes)
84+
85+
## Notes
86+
87+
<!-- Track unexpected behaviors, additional edge cases found, test failures and resolutions. -->
88+
89+
- Real flip/shrink math is not unit-testable in jsdom (no layout engine). The no-jump and
90+
shrink outcomes are the reporter's core complaint, so they are covered by real Playwright
91+
e2e in `e2e/Combobox.spec.js` (stable `top` near the bottom edge, capped `maxHeight`, width
92+
== input) plus the manual showcase for Takuma / Ana.
93+
- Transformed/`contain`/`filter` ancestor + `position: fixed` mis-anchoring is a known
94+
limitation (no portal this pass) — not covered by a test, documented in proposal.
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
## Why
2+
3+
The Combobox dropdown menu jumps around while open (WC-3406). Opening the menu near the
4+
bottom of the viewport, or changing the number of visible options (e.g. by typing to
5+
filter), makes the menu flicker between rendering above and below the input instead of
6+
settling in one place.
7+
8+
Reported by Roman Vyakhirev with a screen recording. The reporter also built a
9+
proof-of-concept branch (`feat/combobox-better-menu`) migrating the menu to floating-ui,
10+
which this change ports and hardens.
11+
12+
Expected: once open, the menu picks a stable placement (below the input by default, above
13+
only when there genuinely isn't room) and does not oscillate when its content height
14+
changes.
15+
16+
## Root Cause
17+
18+
Menu positioning is hand-rolled in `hooks/useMenuStyle.ts`. `getMenuPosition()` chooses
19+
top-vs-bottom placement from the _measured_ menu height, and that same `menuHeight` is a
20+
dependency of the positioning `useEffect`. So: place menu → height changes → effect
21+
re-runs → placement recomputed from the new height → height changes again. The debounced
22+
`setStyle` (32ms) only smears the oscillation over time; it does not remove the feedback
23+
loop. There is also no re-anchoring on scroll/resize.
24+
25+
## What Changes
26+
27+
Replace the hand-rolled positioning with floating-ui (`@floating-ui/react`), following the
28+
reporter's approach but resolving the edge cases his branch left open.
29+
30+
- **New dependency**: `@floating-ui/react` (`^0.26.27`) added to `combobox-web`.
31+
- **New hook** `hooks/useFloatingMenu.ts`: `useFloating` with `strategy: "fixed"`,
32+
`placement: "bottom-start"`, `whileElementsMounted: autoUpdate`, and middleware
33+
`offset(4)``flip({ crossAxis: false, fallbackStrategy: "bestFit", padding: 8 })`
34+
`size({ padding: 8, apply })`. `apply` sets the floating width to the reference width and
35+
caps `maxHeight` at `min(availableHeight, 320)`. Menu stays hidden until
36+
`isPositioned` to avoid a first-frame flash.
37+
- **Delete** `hooks/useMenuStyle.ts` (and stop importing `usePositionObserver` / `debounce`
38+
for this purpose).
39+
- **Wire floating refs** through `SingleSelection` / `MultiSelection`
40+
`Single/MultiSelectionMenu``ComboboxMenuWrapper`: reference ref on the input
41+
container, floating ref + `floatingStyles` on the menu wrapper.
42+
- **Height model (SCSS)**: `.widget-combobox-menu` becomes `display: flex; flex-direction:
43+
column` and is the element floating-ui height-caps. `.widget-combobox-menu-list` changes
44+
from a hard `max-height: 320px` to `max-height: 100%; flex: 1; min-height: 0` so the list
45+
fills the wrapper and scrolls, while header/footer share the capped height. This is what
46+
makes the "shrink when space is tight" behavior work with header/footer present.
47+
- **Strip stale wrapper CSS**: remove `position: absolute`, `display: inline`,
48+
`margin: 4px 0`, `width: 100%`, `left: unset` from `.widget-combobox-menu` — these fight
49+
the inline styles floating-ui writes (the `margin` in particular caused a residual
50+
offset). The 4px gap is preserved via `offset(4)`.
51+
- **alwaysOpen (`keepMenuOpen`) unchanged in behavior**: this mode renders inline with
52+
`position: relative` and must NOT use floating positioning. Both selections call
53+
`useFloatingMenu(alwaysOpen ? false : isOpen)` and the wrapper attaches no floating ref /
54+
styles in the relative branch. (Fixes an inconsistency in the POC where Single passed
55+
`isOpen || keepMenuOpen` and Multi passed `isOpen`.)
56+
57+
## Impact
58+
59+
- **Widget**: `combobox-web` only. No public prop/XML changes — purely internal positioning
60+
and styling. Not breaking for app developers.
61+
- **Behavior change (intended)**: near a viewport edge the menu now shrinks and scrolls
62+
rather than overflowing; default placement is below the input. This shrink behavior is
63+
the part the reporter flagged for design review — this branch is meant to be **shown to
64+
Takuma / Ana before merge**, not merged blind.
65+
- **Must NOT break**:
66+
- `keepMenuOpen` / alwaysOpen inline rendering.
67+
- Menu width still matches the input width.
68+
- Lazy-loading scroll (`widget-combobox-menu-lazy-scroll`) and its scroll handler.
69+
- Menu header/footer content still visible and not clipped when the menu shrinks.
70+
- Existing Single/Multi/Static selection unit tests (updated to mock `useFloatingMenu`).
71+
- **Known limitations (documented, out of scope)**:
72+
- No React portal — with `position: fixed`, a transformed/filtered/`contain` ancestor can
73+
still mis-anchor the menu. Same vulnerability as the old code; not a regression.
74+
- No `shift` middleware (width is pinned to the anchor, so horizontal overflow can't
75+
occur in practice).
76+
- RTL menu _placement_ is handled for free by `bottom-start`; pre-existing RTL item
77+
styling (`margin-right`) is left untouched.
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
## 1. Test Setup
2+
3+
<!-- RED: Write failing tests / add the mock first -->
4+
5+
- [x] 1.1 Add `@floating-ui/react` (`^0.26.27`) to `combobox-web` package.json dependencies
6+
- [x] 1.2 Add `src/hooks/__mocks__/useFloatingMenu.ts` returning stable `refs.setReference` /
7+
`refs.setFloating`, a marker `floatingStyles`, and `isPositioned: true`
8+
- [x] 1.3 Write failing unit test: `SingleSelection` open → menu wrapper gets the floating ref +
9+
`floatingStyles`; `useMenuStyle` no longer imported
10+
- [x] 1.4 Write failing unit test: `MultiSelection` wires floating identically to Single
11+
- [x] 1.5 Test alwaysOpen bypass: wrapper `position: relative`, floating ref NOT attached, floating
12+
styles ignored — via `ComboboxMenuWrapper.spec.tsx` (keepMenuOpen isn't wired to runtime
13+
`Combobox` props, so the presentational wrapper is the correct public interface to test)
14+
- [x] 1.6 Regression: lazy-loading `onScroll` + `widget-combobox-menu-lazy-scroll` covered by the
15+
existing (still-passing) lazy-loading specs
16+
17+
## 2. Implementation
18+
19+
<!-- GREEN: Make tests pass with minimal code -->
20+
21+
- [x] 2.1 Create `src/hooks/useFloatingMenu.ts`: `useFloating` with `strategy: "fixed"`,
22+
`placement: "bottom-start"`, `whileElementsMounted: autoUpdate`, middleware
23+
`offset(4)``flip({ crossAxis: false, fallbackStrategy: "bestFit", padding: 8 })`
24+
`size({ padding: 8, apply })`; `apply` sets width = reference width and
25+
`maxHeight = min(availableHeight, 320)`; hide until `isPositioned`
26+
- [x] 2.2 `ComboboxMenuWrapper`: accept `floatingRef` + `floatingStyles`; apply on the
27+
`.widget-combobox-menu` div; alwaysOpen branch keeps `position: relative`, no floating ref
28+
- [x] 2.3 Thread props through `SingleSelectionMenu` / `MultiSelectionMenu`
29+
- [x] 2.4 `SingleSelection` + `MultiSelection`: call `useFloatingMenu(alwaysOpen ? false : isOpen)`,
30+
set `refs.setReference` on `ComboboxWrapper`, pass `refs.setFloating` + `floatingStyles`
31+
to the menu (consistent across both — fixes the POC Single/Multi inconsistency)
32+
- [x] 2.5 `ComboboxWrapper`: already `forwardRef` — confirm reference ref lands on the
33+
input container div
34+
35+
## 3. Refactoring / Cleanup
36+
37+
<!-- REFACTOR: Clean up while keeping tests green -->
38+
39+
- [x] 3.1 Delete `src/hooks/useMenuStyle.ts`; remove now-dead `usePositionObserver` / `debounce`
40+
usage for menu positioning
41+
- [x] 3.2 SCSS `.widget-combobox-menu`: remove `position: absolute`, `display: inline`,
42+
`margin: 4px 0`, `width: 100%`, `left: unset`; add `display: flex; flex-direction: column`
43+
- [x] 3.3 SCSS `.widget-combobox-menu-list`: `max-height: 320px` → `max-height: 100%; flex: 1;
44+
min-height: 0` so the list fills + scrolls within the (shrunk) wrapper
45+
- [ ] 3.4 Verify header/footer share the capped height and are not clipped when shrunk
46+
(needs live app — part of manual verification / 4.5 showcase)
47+
48+
## 4. Verification
49+
50+
- [x] 4.1 All new + updated unit tests pass (`SingleSelection`, `MultiSelection`,
51+
`StaticSelection` specs green with the mock) — 28 passed, 5 snapshots regenerated
52+
- [x] 4.2 Full `combobox-web` test suite passes; lint clean (touched files: no issues);
53+
snapshots updated
54+
- [x] 4.3 Add Playwright e2e in `e2e/Combobox.spec.js`: menu `top` stable while open,
55+
height capped + within viewport when space is tight, menu width == input width
56+
(run against the live test project — not runnable in this env)
57+
- [x] 4.4 Add CHANGELOG.md entry under `## [Unreleased] > ### Fixed` + `### Changed`
58+
- [ ] 4.5 Showcase branch to Takuma / Ana for the shrink-behavior sign-off before merge
59+
60+
## Notes
61+
62+
- Real flip/shrink math is not unit-testable in jsdom — covered by 4.3 (e2e/manual), not units.
63+
- Known limitation (no portal): `position: fixed` inside a transformed/`contain`/`filter`
64+
ancestor can mis-anchor. Same as old code; documented in proposal, not fixed here.
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
schema: tdd-refactor

0 commit comments

Comments
 (0)