Repository navigation
test(DropdownMenuItem): the audit opens its menu and captures a row's highlight under load - #9495
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| ContextMenuItem | 50.2 kB (0) · 9.9 kB CSS (0) | — | — · — · — |
| DropdownMenu | 70.5 kB (0) · 15.4 kB CSS (0) | — | ✓ · ✓ · ✓ |
| DropdownMenuCheckboxItem | 18.2 kB (0) · 5.9 kB CSS (0) | — | ✓ · ✓ · ✓ |
6 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DropdownMenuDivider | 5.2 kB (0) · 548 B CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuGroup | 6.2 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuItem | 17.4 kB (0) · 5.5 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuRadioGroup | 6.1 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuRadioItem | 18.2 kB (0) · 5.9 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuSubMenu | 55.7 kB (0) · 9.9 kB CSS (0) | — | ✓ · ✓ · ✓ |
Package size: no published package changed; no new dependencies.
API changes: none in this pull request
Committed API report behind this build: core 5 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested, for the description only: the settle-path fix itself looks sound. The Maintainer impact paragraph says every case's screenshot could also catch the menu's entrance fade. But each screenshot already follows holdMotionStill and passes animations: 'disabled', which finishes finite animations and transitions before capture. Please drop that clause, or say that the rest cases now also wait for any running animation before their receipt is read.
[Automated review]
| await expect | ||
| .poll( | ||
| async () => highlighted.evaluate(paintsHighlight, HIGHLIGHT_TOKEN), | ||
| { | ||
| message: `the interacted row paints ${HIGHLIGHT_TOKEN}`, | ||
| }, | ||
| ) | ||
| .toBe(true); |
There was a problem hiding this comment.
Non-blocking: this poll throws before the receipt and screenshot are written (lines 397–402). A focus or hover case that never paints --color-overlay-hover now fails with the poll's message and expected true, received false, and leaves no receipt. That receipt is what diagnosed this flake. Letting the wait time out without throwing would keep the wait and let interactionPaintPass decide, with the receipt still written.
|
Description corrected: Maintainer impact no longer says the screenshots could catch the menu's entrance fade (each one passes On the poll: agreed. A wait that ends without throwing, so a row that never paints still writes its receipt and screenshot and fails on |
vjeux
changed the title
e06b099 to
a01158f
Compare
|
Done in a01158f: the highlight wait no longer throws, so a row that never paints still writes its receipt and screenshot and fails on |
There was a problem hiding this comment.
Ready to approve: both load fixes match their stated causes, and the audit's checks are unchanged. One optional touch-up: the comment on HIGHLIGHT_TOKEN cites component:DropdownMenuItem, but that record names no token; --color-overlay-hover comes from the row's :focus style in DropdownMenuItem.tsx.
[Automated review]
|
|
||
| const OUTPUT = path.resolve('dropdown-menu-item-audit-evidence'); | ||
| const STORY_ID = 'core-dropdownmenuitem--item-states'; | ||
| /** The overlay a focused or hovered row paints (`component:DropdownMenuItem`). */ |
There was a problem hiding this comment.
Optional: component:DropdownMenuItem names no token here (FR8 says only "hover background"). --color-overlay-hover comes from the row's :focus style in DropdownMenuItem.tsx, so citing that source would keep this comment accurate.
|
Agreed: the comment should cite the row's |
a01158f to
01c006f
Compare
… highlight under load Two load causes in the item-states audit. The capture. The focus and hover cases read the interacted row's background right after the interaction, but the row transitions background-color (Item's interactive rows, --duration-fast-min): a focus draws the outline at once and starts the background's transition, and the spec stopped motion only after the interaction, when a running transition keeps its duration. With starved frames the capture read the background before the transition started. Motion now stops before the interaction, and instead of sleeping 40 ms the capture waits for fonts, every finite animation and transition to finish, and an interacted row's background to equal --color-overlay-hover resolved in its theme and color scheme, then two rendered frames. That wait doesn't fail by itself: a row that never paints still writes its receipt and screenshot, and the receipt's check fails. The opening. The story's play opened the menu with a click that carries no coordinates, so its release landed at the page's corner. DropdownMenu lets the release of the press that opened it act once the press has lasted 300 ms, and on a loaded machine the open's own render takes that long; a mouse release outside the trigger and the menu then closes the menu it opened. The play now presses and releases at the trigger's center, where a release never closes it.
01c006f to
900141e
Compare
cixzhang
added
the
ci:run
Maintainer impact
Under load, the DropdownMenuItem item-states audit failed in two ways. Its
focus-lightreceipt recorded the focused row with its focus outline drawn and a transparent background, so the check that the focused row's background differs from a resting row's failed;focus-dark,hover-lightandhover-darkread the row through the same capture. And at eight workers on two cores, about one case in fifty failed at its first step, because the menu the story opens was closed. The file passes when run alone.Intended invariant
Each case records the state it names, in the menu the story opened: an interacted row painting the menu's highlight, and nothing mid-transition. The checks themselves are unchanged.
Change
The capture. Cause: DropdownMenuItem renders through Item's interactive row, which transitions
background-color(--duration-fast-min, 95 ms in the audit's theme). A focus draws the outline at once and starts the background's transition. The spec stopped motion only after the interaction, and a transition already running keeps its duration whatever a later style sets; then it waited 40 ms. When frames are starved, the capture reads the background before the transition has started. A per-frame trace shows it: the first frame after the focus has the outline drawn and the background still transparent, and the overlay arrives over the next 95 ms.holdMotionStillmoves ahead of the focus or hover, so no background transition starts.--color-overlay-hoverresolved in that row's theme and color scheme (a token that doesn't resolve fails). Two frames then render before the receipt is read.interactionPaintPassfails it.focus-dark, and both hover cases (hovering a row focuses it, so it paints the same highlight). The rest cases also wait for any running animation before their receipt is read.The opening. Cause: the story's play opened the menu with
userEvent.click, whose events carry no coordinates, so the press model hit-tests its release at the page's corner. DropdownMenu lets the release of the press that opened it act once the press has lasted 300 ms, and a mouse release outside the trigger and the menu closes the menu. On a loaded machine the open's own synchronous render inside the pointerdown takes that long: in a traced failure the press lasted 318 ms and its release closed the menu.userEvent.pointerwithcoords), where a release never closes the menu. It is still a mouse press, so the hover cases keep the pointer's look (no focus ring on the hovered row).Date.now()in its handlers, so a slow open render counts as press time; that is theuseMenuPresstiming class fix(core): a long press held through a busy main thread still reports, on the touch events' clock #9455 names.Evidence
focus-lightin 4 of 4 runs. The focus style landing 400 ms after the focus: a focus case fails in 4 of 4. The failing receipt matches the reported one: the focused row's backgroundrgba(0, 0, 0, 0), its outline drawn.interactionPaintPass. Without seams: 13 of 13 alone, 156 of 156 on the pin (three ×4 rounds), and 104 of 104 at eight workers on two cores. The receipts match the old play's in all 12 cases.Scope
Testing
pnpm exec playwright test --config playwright.config.ts packages/core/src/DropdownMenu/__tests__/DropdownMenuItemAudit.a11y.chromium.spec.ts --project chromium, alone and astaskset -c 2,3 … --workers=4 --repeat-each 4, against a Storybook build stamped with the checkout's revision as CI stamps it; main's side of each seam ran against a Storybook built from main's story. The seams were injected into local copies of the spec and are not committed.pnpm review:pre(only the public-boundary item applies),pnpm test:node(16,062 tests) andpnpm test:ui(18,860 tests) pass, and Prettier and strict ESLint pass on both files. With a Storybook built from that tree and stamped as CI stamps it, every DropdownMenu accessibility contract and the menu's cross-browser focus-return spec pass in Chromium, 165 of 165. The browser results above ran at a01158f, whose code differs from this head only in comments: the spec passed 13 of 13 alone, 156 of 156 on 4 workers pinned to two cores (three ×4 rounds), and 104 of 104 at 8 workers on the same two cores.