Repository navigation
feat(core): the nav, breadcrumb and tab menus track a held pointer across their items - #8791
Conversation
… their items A press that slides across a nav heading menu's or a top nav menu's items acts on the item it lifts on, once, with the highlight following the pointer; a lift on a disabled item acts on nothing, and a mouse released outside closes the menu (family:action-groups FR1-FR3, FR5). Both reuse useMenuPress, as DropdownMenu does; keyboard and assistive technology are unchanged.
… its rows A press that slides across the flyout's rows acts on the row it lifts on, once, with the highlight following the pointer; a mouse released outside closes it (family:action-groups FR1-FR3, FR5). The flyout reuses useMenuPress over each row's own action (its link or button, marked so a row's toggle and extra actions are not targets); keyboard stays with the popover's focus trap.
…r across its rows" This reverts commit 6886880.
… items A press that slides across the heading menu's items acts on the item it lifts on, once, with the highlight following the pointer; a mouse released outside closes it (family:action-groups FR1-FR3, FR5). Only the role=menu region is tracked, in every heading layout, through useMenuPress; keyboard and assistive technology are unchanged.
…ack a held pointer across their items A press that slides across either menu's items acts on the item it lifts on, once, with the highlight following the pointer; a mouse released outside closes it (family:action-groups FR1-FR3, FR5). TopNavHeading tracks only its role=menu region in both layouts; BreadcrumbItem's menu reuses DropdownMenu's item selector. Both through useMenuPress; keyboard and assistive technology are unchanged.
A press that slides across a TabMenu's options picks the one it lifts on, once, with the highlight following the pointer; a mouse released outside closes it (family:action-groups FR1-FR3, FR5). It reuses useMenuPress over DropdownMenu's item selector; keyboard and assistive technology are unchanged.
# Conflicts: # packages/core/src/NavMenu/NavHeadingMenu.test.tsx
…d a mouse drag as presses Real-browser checks showed two gaps the jsdom tests could not: - Under a finger the browser took the slide as a pan and cancelled the press. Each menu now takes DropdownMenu's touch-action by whether it scrolls (pinch-zoom when its items fit, pan-y pinch-zoom when it scrolls), from one shared module that DropdownMenu now uses too. - Under a mouse, a press on a link item that slid to another started a link drag. Link items in NavHeadingMenu and TopNavMenu are undraggable, as DropdownMenu's link rows are. Stories add a TopNavHeading product menu and a SideNavHeading menu of NavHeadingMenu items, and a changeset names the change.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:design-review
|
/approve-spec 6e8134d |
|
AI review status for this pull request.
|
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.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/NavHeadingMenu | +4.1KB |
+64B |
| core/SideNavHeading | +3.8KB |
+61B |
| core/TabMenu | +3.6KB |
+63B |
| core/TopNavHeading | +3.8KB |
+63B |
| core/TopNavMenu | +4.0KB |
+57B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 35.1KB | 35.1KB | 0B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 504 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | 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.
Requesting changes: the six menus now act on the row a held pointer lifts on, as family:action-groups requires, but mounting useMenuPress over the heading menus also swallows pointer clicks on menu content that is not a role="menuitem" row. Please let a press that does not land on a matched row reach the browser, make an expanded SideNavHeading that holds NavHeadingMenu close on a mouse release outside, and add tests for both.
- In SideNavHeading's and TopNavHeading's
menu, a ghostButton(the documented slot element) and theListItemlinks in the existing SideNav stories no longer respond to a mouse click or a tap; only the keyboard still works (architecture:interaction-modalityINV4). - With NavHeadingMenu inside an expanded SideNavHeading, a mouse press released outside leaves the menu open, so the changeset's "a mouse released outside closes the menu" does not hold there (
module:DropdownMenu/useMenuPressFR3).
[Automated review]
| // FR5); a mouse released outside closes it. Only the role="menu" region is | ||
| // tracked, not the heading button above it. | ||
| const headingMenuRef = useRef<HTMLDivElement>(null); | ||
| const headingMenuPress = useMenuPress({ |
There was a problem hiding this comment.
This root also wraps caller content that is not a role="menuitem" row: the documented ghost Button slot element and the ListItem links in the SideNav stories. A press that starts and lifts on that content settles with stray: true, so the window-capture swallower cancels its click: onClick never runs and links never follow under a mouse, pen or finger (TopNavHeading's mount at line 372 does the same). Consider tracking only presses that land on an itemSelector match, and add a case with a Button and a ListItem link.
| const menuPress = useMenuPress({ | ||
| menuRef: listRef, | ||
| itemSelector: ENABLED_ITEM_SELECTOR, | ||
| onDismiss: closeMenu, |
There was a problem hiding this comment.
Inside an expanded SideNavHeading there is no NavHeadingCloseContext (SideNavHeading FR7), so closeMenu is undefined here. As the innermost marked root this instance owns the press, so a mouse released outside closes nothing, though useMenuPress FR3 and the changeset say it closes. The heading's dismissal needs to reach this press (or the heading's root should own it), with a lift-outside test.
…ts own clicks, and an expanded SideNavHeading's menu closes on a release outside - SideNavHeading's, TopNavHeading's, NavHeadingMenu's and BreadcrumbItem's menus track only presses that land on an item (pressOnItemsOnly), so a button or link a caller puts in the menu keeps its own click under every pointer (architecture:interaction-modality INV4). - In an expanded SideNavHeading (no close-on-select context, FR7), the heading gives its NavHeadingMenu an internal dismissal, so a mouse pressed on an item and released outside closes the menu (useMenuPress FR3); item selection still does not close it. Both with failing-first tests.
|
/approve-spec e4d514d |
There was a problem hiding this comment.
Requesting changes: the six menus now act on the row a held pointer lifts on, as family:action-groups requires, and both earlier findings are fixed (caller content in the heading menus keeps its clicks, and an expanded SideNavHeading closes on a mouse release outside). One regression remains: after a mouse click, slide or tap picks a row in TopNavHeading's product menu, focus returns to the chevron as :focus-visible, so the browser paints its keyboard focus ring where main shows none (architecture:interaction-modality INV1). Please restore focus to the chevron with focusForLastInput rather than a plain focus(), and add a real-browser case asserting the chevron does not match :focus-visible after a pointer pick. Separately, the six component records do not yet name module:DropdownMenu/useMenuPress, which useMenuPress DEC-7 asks of an adopting component.
[Automated review]
| // FR1-FR3, FR5); a mouse released outside closes it. Only the role="menu" | ||
| // region is tracked, not the heading above it. | ||
| const productMenuRef = useRef<HTMLDivElement>(null); | ||
| const productMenuPress = useMenuPress({ |
There was a problem hiding this comment.
A pick in this menu now reaches the row through this press model's dispatched activation, and the close it triggers (useMenuHover's hideAndRestoreFocus, a plain focus()) leaves the chevron matching :focus-visible. In real Chromium, a mouse click, a slide or a tap on Billing in the With Heading Menu story ends with the browser's keyboard ring on the chevron; on main the same click and tap leave none (architecture:interaction-modality INV1). Restoring focus with focusForLastInput (utils/focusReturn.ts) would keep a pointer pick ringless while a keyboard pick keeps its ring.
…board ring, and the six records name useMenuPress - useMenuHover returns focus to its trigger through focusForLastInput, so a mouse click, slide or tap on an item leaves no keyboard ring on the trigger while a keyboard pick keeps it (architecture:interaction-modality INV1). Failing-first test on TopNavHeading. - NavHeadingMenu, TopNavMenu, SideNavHeading, TopNavHeading, BreadcrumbItem and TabMenu records each add one requirement naming module:DropdownMenu/useMenuPress, the surface it governs and the claims (family:action-groups FR1-FR5), as useMenuPress DEC-7 asks of an adopter, with a verification row; each joins family:action-groups.
github-actions
Bot
added
the
needs:spec-owner-review
…mb menu's link item stays the browser's A mouse press with Ctrl, Cmd or Shift on a link item, and a middle press, no longer start the press model in NavHeadingMenu, TopNavMenu, SideNavHeading's and TopNavHeading's menus or BreadcrumbItem's menu: the browser's click reaches the page unprevented, so the link opens in a new tab or window as it did before, and the menu dispatches no activation of its own. One test per menu type (a shared helper) fails on the parent commit; the five records name the rule.
|
/approve-spec a515c5f |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
This is ready to merge: the six menus now act on the item a held pointer lifts on, as family:action-groups requires, and the earlier findings are fixed (caller content keeps its clicks, an expanded SideNavHeading closes on a mouse release outside, and a pointer pick no longer leaves a keyboard ring on TopNavHeading's chevron). One optional follow-up: a mouse slide across NavHeadingMenu rows still selects text, as before this change, and in an expanded SideNavHeading that selection stays on the row the press began on; user-select: none on NavHeadingMenu's root, as DropdownMenu's menu already has, would remove it.
[Automated review]
| themeProps('nav-heading-menu', {size}), | ||
| stylex.props(styles.root, sizeStyles[size], xstyle), | ||
| stylex.props( | ||
| styles.root, |
There was a problem hiding this comment.
Optional: a mouse slide across these rows still starts the browser's text selection, as before this change. In an expanded SideNavHeading, which stays open after a pick, the selection then stays on the row the press began on. Adding userSelect: 'none' to styles.root, as DropdownMenu's menu and BreadcrumbItem's menu already declare, would keep the slide clean.
…umb and tab menus take the menu half of the press model These six menus use only the press model's menuProps (meta#8791). On this branch the public useMenuPress also carries the trigger half, whose scroll-view wait brings the gesture module, so they now use the internal trigger-less entry, useMenuSurfacePress: the same options and menuProps, the pointer model they shipped with, and no gesture code in their bundles.
… land (#8493) Review only this PR's change: vjeux/astryx@2b0f9a1...bcac705 ## Why Two TabMenu keyboard tests in `TabList.test.tsx` fail now and then on loaded CI runners, which fails the UI test shard and the required `test` check of pull requests that never touch TabList (seen on #8045, whose diff is the interaction harness alone). The failure reads "Received element with focus: Reports". ## What led to it We traced it while keeping our own runs green. Opening a TabMenu moves focus to its first item a frame later (`requestAnimationFrame(() => focusFirst())`, the APG menu-button focus-on-open). The two tests press ArrowDown right after the click and expect it to reach the first item. That holds only while the frame has not fired: with nothing focused, ArrowDown focuses the first item; once focus is on it, ArrowDown moves on to the second. On a busy runner the frame fires first. ## Options and tradeoffs - Move focus on open synchronously in the component: changes product behavior, and the frame exists so the menu is laid out when focus moves. - Fake timers in these tests: hides the frame the product relies on, and the tests would stop checking what a user gets. - Wait in the tests for focus to land on the first item, then press arrows (this pull request): the tests check the menu's behavior, whatever the runner's speed. ## Why this choice Test-only, and it asserts what a user sees: the menu opens with its first item focused, then ArrowDown and ArrowUp move from there. The other TabMenu tests (Home/End, Enter, Tab, Escape) don't depend on when that frame fires. The file's newer pointer test (#8791) is unchanged. ## How to check it 1. `pnpm exec vitest run --project ui packages/core/src/TabList/TabList.test.tsx`: every test passes. 2. To see the race without a loaded machine, add `await new Promise(r => setTimeout(r, 50));` after the click in "moves focus between items with ArrowDown and ArrowUp" and in "moves the roving tab stop with arrow navigation": on main both fail every time, with focus on Reports; here both pass. 3. Under load, the two tests as written: 24 copies of the file at once with every core busy, five rounds. Main failed 21 of 240 runs; this branch failed 0 of 240. ## Evidence - The run where it first failed someone else's pull request: #8045
User impact
People using a finger, a pen or a mouse on six menus: NavHeadingMenu, TopNavMenu, SideNavHeading's menu, TopNavHeading's product menu, BreadcrumbItem's menu and TabMenu.
Problem and solution fit
useMenuPresswith its existing API (menuRef,itemSelector,onDismiss), as DropdownMenu does. There's no trigger handoff: hover and click opening stay as they are.touch-actionbased on whether it scrolls (pinch-zoomwhen its items fit,pan-y pinch-zoomwhen it scrolls). Without it the browser cancels the press, which only a real browser shows. The two styles move into one shared module (DropdownMenu/menuTouchAction.stylex.ts), and DropdownMenu uses it too.hooks/menuPressOnItems.ts). A button or link a caller puts in the menu keeps its own click under every pointer (architecture:interaction-modalityINV4).focusForLastInput. A mouse click, slide or tap on an item leaves no keyboard ring on the trigger, and a keyboard pick keeps its ring (architecture:interaction-modalityINV1). This affects every useMenuHover menu.module:DropdownMenu/useMenuPress, the surface it governs and the claims (family:action-groupsFR1–FR5), as useMenuPress DEC-7 asks of an adopter, with a verification row. Each record joinsfamily:action-groups.Expected behavior and authority
family:action-groupsFR1–FR5 (current, merged in #8622):#8622's adoption table lists these menus under useMenuPress in-menu tracking.
Evidence
Real Chrome 156 (headless) against the built Storybook at this head and at main (b393144). The same stories were added to both builds so the two compare like for like. The real-input probe presses the first item, slides to the second, then lifts. It checks the highlight while held and which item's click ran.
NavHeadingMenu is covered through the SideNavHeading and TopNavHeading stories, which render NavHeadingMenu items. While held, the second item carries the highlight (this PR):
The other captures and the raw results for both builds are in the evidence folder (
results-pr.jsonl,results-main.jsonl).Caller content: in the Header with Menu story (
ListItemlinks inside SideNavHeading's menu), a mouse click or tap on a link reaches the page on main, was swallowed at 6e8134d, and reaches the page again at e4d514d (caller-content.jsonl). The slide results above hold at e4d514d and at 9c5299a (results-pr-e4d514d5.jsonl).Focus ring: after a pick, focus returns to the trigger with
:focus-visiblefalse for mouse and touch at this head. The headless probe also read false at e4d514d, so it doesn't reproduce the ring reported there; a unit test proves the pointer path now focuses withfocusVisible: false.Testing
draggable="false".focusVisible: false. This fails on e4d514d.hooks/__tests__/menuLinkClicks.ts): a Ctrl-, Cmd- and Shift-click and a middle click on a link item each reach the page once, unprevented, with no activation of the menu's own. All five fail on 9c5299a.Scope