[Select] Fix focus visible always set on menu item#47912
[Select] Fix focus visible always set on menu item#47912silviuaavram merged 8 commits intomui:masterfrom
Conversation
Netlify deploy previewhttps://deploy-preview-47912--material-ui.netlify.app/ Bundle size report
|
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent Select from always applying “focus-visible” styling to the initially focused menu item by tracking whether the menu was opened via keyboard vs pointer, and threading that information into MenuItem autofocus behavior.
Changes:
- Extracted
SelectInputhelpers (areEqualValues,isEmpty) intopackages/mui-material/src/Select/utils/*and addedgetOpenInteractionType. - Added
openInteractionTypestate inSelectInputand started passing a focus-visible hint throughautoFocus. - Updated
MenuListto not override a child’sautoFocus, and expandedMenuItem’sautoFocusprop typing/docs to support an object payload.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mui-material/src/Select/utils/isEmpty.ts | New helper extracted from SelectInput. |
| packages/mui-material/src/Select/utils/areEqualValues.ts | New helper extracted from SelectInput. |
| packages/mui-material/src/Select/utils/getOpenInteractionType.ts | New helper to classify open interaction as keyboard/pointer. |
| packages/mui-material/src/Select/utils/index.ts | Barrel export for the new Select utils. |
| packages/mui-material/src/Select/SelectInput.js | Tracks open interaction type; injects autoFocus payload into items. |
| packages/mui-material/src/MenuList/MenuList.js | Avoids overriding autoFocus if a child already provides it. |
| packages/mui-material/src/MenuItem/MenuItem.js | Allows autoFocus to be an object and forwards it to .focus(...). |
| packages/mui-material/src/MenuItem/MenuItem.d.ts | Updates TS type for autoFocus union. |
| docs/pages/material-ui/api/menu-item.json | Updates API docs to reflect new autoFocus type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
175aee5 to
b7bb4b2
Compare
|
@copilot review again |
5709bf8 to
1b64525
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4983cdd to
c75ee0a
Compare
8a915d5 to
1b82580
Compare
b0ee6a6 to
e3d8b6f
Compare
| } | ||
|
|
||
| try { | ||
| element.focus(autoFocus); |
There was a problem hiding this comment.
even with the try catch guard, this still fails the tests, so had to avoid calling it during tests. unfortunately it will also not get called on local dev.
There was a problem hiding this comment.
Is it possible to adjust process.env.NODE_ENV !== 'production' to at least have a test with playwright?
a388479 to
f37c544
Compare
Proposed e2e testThe e2e setup builds with Fixture import * as React from 'react';
import Select from '@mui/material/Select';
import MenuItem from '@mui/material/MenuItem';
export default function SelectFocusVisible() {
return (
<Select defaultValue={10} data-testid="select">
<MenuItem value={10}>Ten</MenuItem>
<MenuItem value={20}>Twenty</MenuItem>
<MenuItem value={30}>Thirty</MenuItem>
</Select>
);
}Tests in describe('<Select />', () => {
it('should not show focus-visible on menu item when opened by mouse', async () => {
await renderFixture('Select/SelectFocusVisible');
const trigger = page.getByRole('combobox');
await trigger.click();
await page.waitForSelector('[role="listbox"]');
const selectedItem = page.locator('[role="option"][aria-selected="true"]');
await expect(selectedItem).toBeFocused();
const hasVisible = await selectedItem.evaluate(
(el) => el.classList.contains('Mui-focusVisible'),
);
expect(hasVisible).toEqual(false);
});
it('should show focus-visible on menu item when opened by keyboard', async () => {
await renderFixture('Select/SelectFocusVisible');
await page.keyboard.press('Tab');
const trigger = page.getByRole('combobox');
await expect(trigger).toBeFocused();
await page.keyboard.press('Enter');
await page.waitForSelector('[role="listbox"]');
const selectedItem = page.locator('[role="option"][aria-selected="true"]');
await expect(selectedItem).toBeFocused();
const hasVisible = await selectedItem.evaluate(
(el) => el.classList.contains('Mui-focusVisible'),
);
expect(hasVisible).toEqual(true);
});
});Both modalities (mouse vs keyboard) are covered, directly validating the user-facing behavior this PR fixes — no |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e96f53e to
6530fba
Compare
3a476a9 to
404bc7e
Compare
c92d99b to
2ae4b44
Compare
|
Cherry-pick PRs will be created targeting branches: v7.x |
Fixes #23747
Leveraged a good deal from #47898.