Skip to content

[Select] Fix focus visible always set on menu item#47912

Merged
silviuaavram merged 8 commits intomui:masterfrom
silviuaavram:fix/select-item-focus-visible
Mar 18, 2026
Merged

[Select] Fix focus visible always set on menu item#47912
silviuaavram merged 8 commits intomui:masterfrom
silviuaavram:fix/select-item-focus-visible

Conversation

@silviuaavram
Copy link
Copy Markdown
Member

@silviuaavram silviuaavram commented Mar 5, 2026

Fixes #23747

Leveraged a good deal from #47898.

  • pass the pointer source via context from SelectInput.
  • MenuItem uses the pointer source (if available) in order to perform focus with focusVisible.
  • mock the focusVisible with parameter function when tests run in firefox, otherwise all focus visible related browser unit tests fail on firefox
  • do not add autoFocus prop on MenuItem from MenuList if it's already passed to the component

Copilot AI review requested due to automatic review settings March 5, 2026 07:59
@silviuaavram silviuaavram marked this pull request as draft March 5, 2026 07:59
@mui-bot
Copy link
Copy Markdown

mui-bot commented Mar 5, 2026

Netlify deploy preview

https://deploy-preview-47912--material-ui.netlify.app/

Bundle size report

Bundle Parsed size Gzip size
@mui/material 🔺+575B(+0.11%) 🔺+241B(+0.16%)
@mui/lab 0B(0.00%) 0B(0.00%)
@mui/system 0B(0.00%) 0B(0.00%)
@mui/utils 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 2ae4b44

@silviuaavram silviuaavram added accessibility a11y breaking change Introduces changes that are not backward compatible. scope: select Changes related to the select. v9.x labels Mar 5, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SelectInput helpers (areEqualValues, isEmpty) into packages/mui-material/src/Select/utils/* and added getOpenInteractionType.
  • Added openInteractionType state in SelectInput and started passing a focus-visible hint through autoFocus.
  • Updated MenuList to not override a child’s autoFocus, and expanded MenuItem’s autoFocus prop 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.

@silviuaavram silviuaavram force-pushed the fix/select-item-focus-visible branch from 175aee5 to b7bb4b2 Compare March 5, 2026 08:28
@silviuaavram silviuaavram marked this pull request as ready for review March 5, 2026 09:05
@silviuaavram
Copy link
Copy Markdown
Member Author

silviuaavram commented Mar 5, 2026

@copilot review again

@silviuaavram silviuaavram force-pushed the fix/select-item-focus-visible branch 2 times, most recently from 5709bf8 to 1b64525 Compare March 5, 2026 13:53
@silviuaavram silviuaavram self-assigned this Mar 6, 2026
@silviuaavram silviuaavram requested a review from Copilot March 6, 2026 08:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@silviuaavram silviuaavram marked this pull request as draft March 6, 2026 13:25
@silviuaavram silviuaavram force-pushed the fix/select-item-focus-visible branch from 4983cdd to c75ee0a Compare March 6, 2026 13:43
@silviuaavram silviuaavram marked this pull request as ready for review March 6, 2026 13:45
@silviuaavram silviuaavram force-pushed the fix/select-item-focus-visible branch from 8a915d5 to 1b82580 Compare March 9, 2026 07:38
@silviuaavram silviuaavram reopened this Mar 9, 2026
@silviuaavram silviuaavram force-pushed the fix/select-item-focus-visible branch from b0ee6a6 to e3d8b6f Compare March 9, 2026 11:49
}

try {
element.focus(autoFocus);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to adjust process.env.NODE_ENV !== 'production' to at least have a test with playwright?

@silviuaavram silviuaavram force-pushed the fix/select-item-focus-visible branch 2 times, most recently from a388479 to f37c544 Compare March 10, 2026 07:42
@siriwatknp
Copy link
Copy Markdown
Member

siriwatknp commented Mar 10, 2026

Proposed e2e test

The e2e setup builds with NODE_ENV=production and runs in real Chromium — focus({ focusVisible }) is natively supported there.

Fixture test/e2e/fixtures/Select/SelectFocusVisible.tsx:

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 test/e2e/index.test.ts:

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 NODE_ENV hacks needed.

Copy link
Copy Markdown
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Awesome

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@silviuaavram silviuaavram force-pushed the fix/select-item-focus-visible branch from e96f53e to 6530fba Compare March 11, 2026 09:06
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 12, 2026
@silviuaavram silviuaavram force-pushed the fix/select-item-focus-visible branch from 3a476a9 to 404bc7e Compare March 12, 2026 11:46
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Mar 12, 2026
@silviuaavram silviuaavram added v7.x needs cherry-pick The PR should be cherry-picked to master after merge. and removed breaking change Introduces changes that are not backward compatible. v9.x labels Mar 12, 2026
@silviuaavram silviuaavram force-pushed the fix/select-item-focus-visible branch from c92d99b to 2ae4b44 Compare March 18, 2026 11:15
@silviuaavram silviuaavram merged commit 89ef991 into mui:master Mar 18, 2026
19 checks passed
@silviuaavram silviuaavram deleted the fix/select-item-focus-visible branch March 18, 2026 11:38
@github-actions
Copy link
Copy Markdown

Cherry-pick PRs will be created targeting branches: v7.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility a11y needs cherry-pick The PR should be cherry-picked to master after merge. scope: select Changes related to the select. v7.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Select] - First options receives .Mui-focusVisible always

5 participants