Repository navigation
Conversation
… frame did not focus The case added with the menu-row hover focus fix (meta#8221) expected one focus({preventScroll: true, focusVisible: false}) call on the Design row after a pointer open, and sometimes saw none in CI. BreadcrumbItem opens its menu and focuses the first row, Design, in the next animation frame; when that frame landed before the test's pointerMove, Design already had focus, and focusMenuItemOnHover rightly moves nothing onto an element that has it. The case now waits for that frame's focus on Design, then hovers Engineering, so the hover moves focus in either order. It still guards the same rule: with the crumb's useInteractionModalityTracking() removed it fails. With requestAnimationFrame forced to fire before the hover, the previous case fails every time with the CI message (no call) and this one passes; it also passes with the frame forced late and over 200 repeats, idle and under load. Test only.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Recommending approval. The hover-focus case now waits for the menu's first-row focus and then hovers Engineering, so it passes whether the animation frame lands before or after the hover, and its focusVisible: false assertion still fails if the Breadcrumbs menu stops activating modality tracking. No changes are needed.
[Automated review]
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
|
Closing: #8824 fixed the same race the same way (it waits for the first row's focus, then hovers Engineering), so this one is no longer needed. |
Maintainer impact
What led to it: our merged #8221 added a Breadcrumbs test that turned out flaky in CI; another pull request's runs hit it about once in 15.
Why: the case "after a pointer open, a row the mouse rests on takes focus without :focus-visible" sometimes sees no
focuscall and fails a green pull request.Intended invariant
The case guards that focus following the mouse onto a row after a pointer open carries
focusVisible: false. That assertion is unchanged.Change
The menu focuses its first row, Design, in the next animation frame. When that frame lands before the test's hover onto Design, Design already has focus, so hovering it moves nothing and the spy sees no call. The case now waits for that first focus, then hovers the next row, Engineering, so it passes in either order.
Evidence
With the frame forced to land early, the old case fails every time with CI's message and the new case passes; it also passes with the frame forced late and over 200 natural repeats. Without the crumb's modality tracking, the new case fails, so it still guards the rule. Test only.
Scope
Testing
Run
pnpm vitest run packages/core/src/Breadcrumbs: the suite passes, and the changed case passes whether the menu's first focus lands before or after the hover.