You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
121 lines
8.8 KiB
121 lines
8.8 KiB
|
3 months ago
|
# Audit: dropdown-menu
|
||
|
|
audit-version: 1
|
||
|
|
audited-at: 2026-06-26
|
||
|
|
scope: ['soma', 'sema', 'eidos'] (eidos IS declared — no scope-drift, unlike select/dialog/popover/combobox)
|
||
|
|
files:
|
||
|
|
- provider: src/uix/soma/components/dropdown-menu/dropdown-menu-provider.svelte.ts (present)
|
||
|
|
- morfo: src/uix/morfo/components/dropdown-menu.ts (present)
|
||
|
|
- recipe: src/uix/eidos/components/dropdown-menu/dropdown-menu.css + lib/recipes/base.ts §dropdown-menu (present)
|
||
|
|
- demo: web/routes/uix/components/dropdown-menu (present, not inspected this pass)
|
||
|
|
- test: src/uix/soma/components/dropdown-menu/dropdown-menu-provider.svelte.test.ts (present, jsdom, 6 tests)
|
||
|
|
- readme: present (not inspected this pass)
|
||
|
|
|
||
|
|
## Summary
|
||
|
|
The most disciplined provider of Batch 1. Consistent roving DOM focus (no virtual/real mix — the morfo declares
|
||
|
|
NO `aria-activedescendant`, the provider uses real `dom.focus`, A17-correct), uniform `renderProps()` usage
|
||
|
|
(no double-writes), a clean eidos→soma frontier, correct `data-disabled`/`accessibleWhenDisabled` handling, and
|
||
|
|
fully self-managed cleanup — including `SafePolygon`, which registers its document listeners inside an internal
|
||
|
|
`$effect` via `dom.listen` (verified: no A6 leak). It is the only Batch-1 overlay that correctly declares
|
||
|
|
`eidos` in its morfo scope. No HIGH findings. The remaining issues are the systemic directional-nav duplication,
|
||
|
|
magic theming literals, a checkbox/radio toggle that emits no semantic event, and a navigation-untested gap.
|
||
|
|
|
||
|
|
Counts: CRITICAL 0 · HIGH 0 · MEDIUM 4 · LOW 1.
|
||
|
|
|
||
|
|
## Findings
|
||
|
|
|
||
|
|
### MEDIUM: Directional-nav index math duplicated (Content + SubContent, and across Select/Combobox) — incl. the loop off-by <!-- id: dropdown-menu-001 -->
|
||
|
|
- dimension: G, B
|
||
|
|
- rule: dimension-G redundancy + the loop-boundary edge case
|
||
|
|
- location: dropdown-menu-provider.svelte.ts:384-422 (Content.onkeydown) and :1165-1209 (SubContent.onkeydown)
|
||
|
|
- evidence: both handlers re-implement the identical next/prev/Home/End + typeahead index math
|
||
|
|
(`(i+1)%len` / `Math.min` / `Math.max` / `(i-1+len)%len`). It's also the same math as Select content route
|
||
|
|
(select-003) and Combobox content route. The `currentIndex === -1` + prev + loop path computes
|
||
|
|
`(-1-1+n)%n = n-2` (lands second-to-last, not last) — the same off-by as select-003; reachable when keydown
|
||
|
|
fires with focus on the container rather than an item.
|
||
|
|
- impact: four copies of one algorithm (one bug fixed in one place misses the others); the loop off-by is a
|
||
|
|
latent edge-case bug.
|
||
|
|
- proposed-fix: extract a pure `nextIndex(curr, key, len, {loop, dir, orientation})` helper, unit-test the
|
||
|
|
boundaries once, and consume it in all menu/select/combobox keyboard routes. (See SHARED_EXTRACTION
|
||
|
|
DIRECTIONAL-NAV.)
|
||
|
|
- fix-status: open
|
||
|
|
|
||
|
|
### MEDIUM: Checkbox/Radio item activation mutates state but emits no semantic event (unlike plain Item) <!-- id: dropdown-menu-002 -->
|
||
|
|
- dimension: A, B
|
||
|
|
- rule: M-3.7 (every state-mutating keyboard action needs a semantic event) + the morfo only declares
|
||
|
|
`commit-select` on `item`
|
||
|
|
- location: provider `MenuItemProvider.onclick/onkeydown` fire `runtime.trigger('commit-select', …)` (:510, :521),
|
||
|
|
but `MenuCheckboxItemProvider.activate` (:735-745) and `MenuRadioItemProvider.activate` (:860-865) mutate
|
||
|
|
`checked`/group value and call only `onSelect()` + `handleClose()` — no `runtime.trigger`.
|
||
|
|
- evidence: toggling a checkbox-item or selecting a radio-item changes state with no perceptual emit; the morfo
|
||
|
|
declares no event for `checkbox-item`/`radio-item`.
|
||
|
|
- impact: with `closeOnSelect: false` (the common case for checkbox menus) there is zero sema feedback on
|
||
|
|
toggle, while a plain Item select gets `commit.select + affirm`. Inconsistent perceptual contract; a
|
||
|
|
checkbox toggle is canonically `commit.toggle`.
|
||
|
|
- repro: open a menu with a `closeOnSelect={false}` CheckboxItem, toggle it — no `data-event` is stamped.
|
||
|
|
- proposed-fix: declare `commit-toggle` (checkbox) / `commit-select` (radio) events on those parts in the
|
||
|
|
morfo and fire them from `activate()`, or document the silence as deliberate in the README.
|
||
|
|
- fix-status: open
|
||
|
|
|
||
|
|
### MEDIUM: Magic theming literals — `content-z: '80'`, `item-disabled-opacity: '0.55'`, `item-height: '2rem'` <!-- id: dropdown-menu-003 -->
|
||
|
|
- dimension: E-bis
|
||
|
|
- rule: THEMING §35 (z-index/opacity magic → tokens) + §5 (size from `--list-*`/`--control-height-*`/`--space-*`) + §4
|
||
|
|
- location: lib/recipes/base.ts:4279 (`'content-z': '80'`), :4292 (`'item-disabled-opacity': '0.55'`),
|
||
|
|
:4286 (`'item-height': '2rem'`)
|
||
|
|
- evidence: `content-z: '80'` (5th overlay with a hardcoded content-z — systemic z-ladder). `0.55` should be
|
||
|
|
`var(--opacity-disabled)` (the canonical disabled opacity Dialog uses). `2rem` item-height is a raw rem
|
||
|
|
where the list-surface size layer (`--list-*`, per project memory 2026-06-21) should drive it.
|
||
|
|
- impact: opacity/size/z values that won't track theme/density/scale changes; the disabled opacity diverges
|
||
|
|
from the canonical `--opacity-disabled` used elsewhere.
|
||
|
|
- proposed-fix: `content-z` → canonical overlay z token; `item-disabled-opacity` → `var(--opacity-disabled)`;
|
||
|
|
confirm `item-height` should bridge `--list-item-height-*`.
|
||
|
|
- fix-status: open
|
||
|
|
|
||
|
|
### MEDIUM: Arrow-key NAVIGATION index math is untested (activation/groups/submenu ARE tested); jsdom-only <!-- id: dropdown-menu-004 -->
|
||
|
|
- dimension: F
|
||
|
|
- rule: dimension-F honesty check
|
||
|
|
- location: dropdown-menu-provider.svelte.test.ts (6 tests)
|
||
|
|
- evidence: the suite is the strongest of Batch 1 — it covers trigger toggle, item scoping + disabled policy,
|
||
|
|
click/keyboard activation + close, checkbox/radio groups, submenu open (hover/click/ArrowRight), and
|
||
|
|
group/separator a11y. But no test calls `Content.onkeydown`/`SubContent.onkeydown` with ArrowDown/ArrowUp/
|
||
|
|
Home/End to exercise the navigation index math (dropdown-menu-001) or the loop boundary. `@vitest-environment
|
||
|
|
jsdom` (:1) — `dom.focus` moves aren't faithful.
|
||
|
|
- impact: the duplicated nav math + loop off-by ship unverified.
|
||
|
|
- proposed-fix: add Content/SubContent navigation tests asserting the focused index after each arrow key incl.
|
||
|
|
the loop wrap; add a client/Playwright test for real focus movement.
|
||
|
|
- fix-status: open
|
||
|
|
|
||
|
|
### LOW: `open`/`close`/`commit-select` set state imperatively alongside a non-awaited `void trigger` (sequence not runtime-sequenced) <!-- id: dropdown-menu-005 -->
|
||
|
|
- dimension: B
|
||
|
|
- rule: two-moments doctrine + the Dialog pattern (events wired into the runtime so `trigger` sequences emit→handler)
|
||
|
|
- location: runtime created with `{}` (no events map, :93); `handleOpen`/`handleClose` do
|
||
|
|
`void this.runtime.trigger(...)` then set `open` synchronously (:116-140).
|
||
|
|
- evidence: unlike Dialog (which wires `events: { open, close }` so `runtime.trigger` awaits emit before the
|
||
|
|
handler), dropdown-menu sets `open` imperatively next to a fire-and-forget emit, so the morfo's
|
||
|
|
`sequence: 'pre'` is not enforced by the runtime — it relies on Presence keeping content mounted during exit.
|
||
|
|
- impact: works in practice (Presence + the 'pre' content still exists), and the non-await is likely a
|
||
|
|
deliberate snappiness choice for a high-frequency surface; but it diverges from the canonical sequencing and
|
||
|
|
makes `sequence: 'pre'` advisory here.
|
||
|
|
- proposed-fix: either wire the open/close handlers into the runtime `events` map (Dialog parity) or document
|
||
|
|
that menus intentionally don't await the emit. Decide once and apply to all menu-family components.
|
||
|
|
- fix-status: open
|
||
|
|
|
||
|
|
## No-findings dimensions
|
||
|
|
- **A17 (focus strategy):** consistent roving DOM focus — morfo declares no `aria-activedescendant`, provider
|
||
|
|
uses real `dom.focus`. No mixing (contrast Combobox). Clean.
|
||
|
|
- **C (selectors):** `getItems` composes attribute selectors with `.closest()` scoping (A10), no user-value
|
||
|
|
interpolation. Clean.
|
||
|
|
- **D (frontier, severe half):** provider imports only soma layers + morfo — zero eidos imports.
|
||
|
|
- **A6 (cleanup):** typeahead `destroy` (:111), SubTrigger open timer (:1007), and `SafePolygon` (self-manages
|
||
|
|
via an internal `$effect` + `dom.listen` disposers, verified safe-polygon.ts:209-215) all clean up. No leak.
|
||
|
|
- **A (renderProps discipline):** every part spreads `renderProps()` and adds only soma-owned extras
|
||
|
|
(data-state/tabindex/handlers) — no syncAttrs+manual double-write (contrast Dialog/Popover).
|
||
|
|
- **A30:** SubTrigger ref mirror via `onRefChange`, not `$effect` (:988).
|
||
|
|
- **SCOPE:** morfo scope correctly includes `eidos` — the only Batch-1 overlay without the scope-drift.
|
||
|
|
|
||
|
|
## Style observations (opinion, non-blocking)
|
||
|
|
- Use dropdown-menu as the positive reference for: roving-focus consistency, uniform `renderProps()` (no
|
||
|
|
double-write), self-managed layer cleanup, and correct `eidos`-in-scope declaration. Its main debts are
|
||
|
|
shared, systemic ones (nav duplication, magic literals), not local mistakes.
|
||
|
|
- The checkbox/radio silent-toggle (dropdown-menu-002) is the one genuinely local contract gap worth an
|
||
|
|
explicit decision.
|