Repository navigation
fix(core): a mouse slide across NavHeadingMenu or TabMenu items selects no text - #8802
Conversation
…text NavHeadingMenu, TopNavMenu and TabMenu take userSelect none on their menu, as DropdownMenu's and BreadcrumbItem's menus do, so a held pointer sliding across the items (family:action-groups) starts no text selection; in an expanded SideNavHeading, which stays open after a pick, the selection used to stay on the item the press began on. Follow-up to #8791.
|
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.
|
github-actions
Bot
added
the
needs:design-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. Visual RegressionStatus: No visual change across 6 compared shot(s). 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.
Changes requested: the fix stops a mouse slide from selecting the rows' text in all three menus, but it needs two adjustments first.
userSelect: 'none'on NavHeadingMenu's root also makes caller content inside the menu unselectable, whichcomponent:NavHeadingMenuFR8 leaves to the browser; putting it onNavHeadingMenuItem's row keeps the fix and the caller's text.- The new test only checks that the style is declared, so please add real-browser slide tests (like
MenuPress.a11y.chromium.spec.ts) for NavHeadingMenu, TopNavMenu and TabMenu that assert no selection remains, asarchitecture:component-test-sufficiencyINV3 asks.
[Automated review]
| const styles = stylex.create({ | ||
| root: { | ||
| // A held pointer slides across the items; it selects no text. | ||
| userSelect: 'none', |
There was a problem hiding this comment.
This root also holds caller content that is not an item, which component:NavHeadingMenu FR8 leaves to "that content's or the browser's own". user-select: none here resolves onto that content, so text a caller puts in the menu can no longer be selected or copied. Moving the declaration to NavHeadingMenuItem's root style keeps the slide free of selection (a press has to start on an item to be tracked) and leaves caller content alone.
| <NavHeadingMenuItem label="Dashboard" onClick={() => {}} /> | ||
| </NavHeadingMenu>, | ||
| ); | ||
| expect(screen.getByRole('menu')).toHaveStyle({userSelect: 'none'}); |
There was a problem hiding this comment.
This asserts the new StyleX declaration rather than the outcome; architecture:component-test-sufficiency lists that kind of test as not establishing sufficiency, and INV3 asks for evidence at a layer that reproduces the selection. A real-Chromium slide test (as in MenuPress.a11y.chromium.spec.ts) that asserts window.getSelection().isCollapsed after a mouse slide, for NavHeadingMenu, TopNavMenu and TabMenu, would fail on main and pass here.
… real-Chromium slide tests - userSelect none moves from NavHeadingMenu's root to NavHeadingMenuItem, so caller content beside the items stays selectable (NavHeadingMenu FR8). - TopNavMenu's change is dropped: its slide already selects no text. - The jsdom style assertion is replaced by a real-Chromium spec: a mouse slide from the first item's text to the second selects nothing while held and after the lift, in TopNavHeading's and an expanded SideNavHeading's NavHeadingMenu, TopNavMenu and TabMenu. On main three of the four fail (the held selection, which an expanded SideNavHeading keeps after the lift); all pass here.
cixzhang
changed the title
There was a problem hiding this comment.
This is ready to approve: putting user-select: none on NavHeadingMenuItem's row and TabMenu's dropdown stops a mouse slide from selecting item text while caller content beside NavHeadingMenu's items stays selectable, and the new Chromium spec fails on main and passes here. One optional follow-up: TopNavMenu items with onClick and no href still select text during a slide, which the spec's TopNavMenu case cannot catch because its story only has link rows, so consider restoring TopNavMenu's declaration with an action-row case or narrowing that case's claim to link rows.
[Automated review]
| }); | ||
| }); | ||
|
|
||
| test('TopNavMenu', async ({page}) => { |
There was a problem hiding this comment.
This case passes on main as well, so it cannot catch the selection bug in TopNavMenu: core-topnavmenu--default has only link rows, where Chromium starts no selection, and first.x + 12 lands on TopNavMenu's 40 px icon cell rather than the label. TopNavMenu rows with onClick and no href still select text during a slide on this head. Consider restoring TopNavMenu's declaration with an action-row case, or naming this case (and the header's claim) as link rows only.
User impact
Someone sliding a mouse from one NavHeadingMenu or TabMenu item to the next, which these menus track as a held pointer since #8791, selected the item text while holding. In an expanded SideNavHeading, which stays open after a pick, the selection stayed on the item the press began on. With this change no item text is selected. Caller content beside NavHeadingMenu's items stays selectable.
Change
userSelect: 'none'on NavHeadingMenuItem's row and on TabMenu's dropdown. DropdownMenu's and BreadcrumbItem's menus already do this. NavHeadingMenu's root is left alone, so caller content keeps the browser's selection (component:NavHeadingMenuFR8).NavMenu/__tests__/NavMenusSlideSelection.a11y.chromium.spec.ts. A mouse presses the first item's text, slides to the second and lifts. The spec asserts that nothing is selected while held or after the lift, in TopNavHeading's and an expanded SideNavHeading's NavHeadingMenu, in TopNavMenu, and in TabMenu.Evidence
The spec runs in Chrome 156 against the built Storybook at main (820769d) and at this head:
Authority
family:action-groupsFR1–FR2 (the slide is the gesture),component:NavHeadingMenuFR8,component:TabMenuFR7, and the DropdownMenu precedent. CodeBunny noted this on #8791 at a515c5f.Testing
The new Chromium spec fails 3 of 4 on main and passes 4 of 4 here. NavMenu, TopNav and TabList unit tests: 288 pass. tsc, lint and the changeset check are clean.