Skip to content

Menu: Fix flaky submenu focus test #77430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mirka merged 5 commits into from
Apr 22, 2026
Merged

Menu: Fix flaky submenu focus test #77430

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1447ca3
Fix flaky Menu submenu focus test
mirka Apr 16, 2026
aa01b67
Simplify
mirka Apr 16, 2026
a288f6a
Stabilize Menu focus tests
mirka Apr 16, 2026
8bcfb44
Address review feedback on Menu focus tests
mirka Apr 22, 2026
c88fb89
Trim Menu focus test waits
mirka Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 47 additions & 94 deletions packages/components/src/menu/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,19 @@ const delay = ( delayInMs: number ) => {
return new Promise( ( resolve ) => setTimeout( resolve, delayInMs ) );
};

const waitForFocusedMenu = () =>
waitFor( () => expect( screen.getByRole( 'menu' ) ).toHaveFocus() );

const waitForFocusedMenuItem = ( name: string ) =>
waitFor( () =>
expect( screen.getByRole( 'menuitem', { name } ) ).toHaveFocus()
);

const waitForFocusedButton = ( name: string ) =>
waitFor( () =>
expect( screen.getByRole( 'button', { name } ) ).toHaveFocus()
);

// Open dropdown => open menu
// Submenu trigger item => open submenu

Expand Down Expand Up @@ -114,7 +127,7 @@ describe( 'Menu', () => {
await click( toggleButton );

// Menu open, focus is on the menu wrapper
expect( screen.getByRole( 'menu' ) ).toHaveFocus();
await waitForFocusedMenu();
} );

it( 'should open and focus the first item when pressing the arrow down key on the trigger', async () => {
Expand Down Expand Up @@ -145,9 +158,7 @@ describe( 'Menu', () => {

// Menu open, focus is on the first focusable item
// (disabled items are still focusable and accessible)
expect(
screen.getByRole( 'menuitem', { name: 'First item' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'First item' );
} );

it( 'should open and focus the first item when pressing the space key on the trigger', async () => {
Expand Down Expand Up @@ -178,9 +189,7 @@ describe( 'Menu', () => {

// Menu open, focus is on the first focusable item
// (disabled items are still focusable and accessible
expect(
screen.getByRole( 'menuitem', { name: 'First item' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'First item' );
} );

it( 'should close when pressing the escape key', async () => {
Expand All @@ -201,18 +210,14 @@ describe( 'Menu', () => {

// Focuses menu on mouse click, focuses first item on keyboard press
// Can be changed with a custom useEffect
expect( screen.getByRole( 'menu' ) ).toHaveFocus();
await waitForFocusedMenu();

// Pressing esc will close the menu and move focus to the toggle
await press.Escape();

expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument();

await waitFor( () =>
expect(
screen.getByRole( 'button', { name: 'Open dropdown' } )
).toHaveFocus()
);
await waitForFocusedButton( 'Open dropdown' );
} );

it( 'should close when clicking outside of the content', async () => {
Expand Down Expand Up @@ -349,97 +354,53 @@ describe( 'Menu', () => {
);

// The menu is focused automatically when `defaultOpen` is set.
await waitFor( () =>
expect( screen.getByRole( 'menu' ) ).toHaveFocus()
);
await waitForFocusedMenu();
Comment on lines -352 to +356

@aduth aduth Apr 20, 2026

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.

I wondered if we actually needed to be awaiting these, or if we can expect it to be synchronous once the menu is open. Your PR description mentions "Leaves the synchronous assertions in place for stable in-menu navigation where focus is already inside the same menu context". Is it not referring to this?

@mirka mirka Apr 22, 2026

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.

I tried trimming that one further by making the defaultOpen focus assertion synchronous, but it immediately failed locally on the same expectation, so I restored the waitFor.


// Arrow up/down selects menu items
// The selection wraps around from last to first and viceversa
await press.ArrowDown();
expect(
screen.getByRole( 'menuitem', { name: 'Menu item 1' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'Menu item 1' );

await press.ArrowDown();
expect(
screen.getByRole( 'menuitem', { name: 'Menu item 2' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'Menu item 2' );

await press.ArrowDown();
expect(
screen.getByRole( 'menuitem', { name: 'Submenu trigger item' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'Submenu trigger item' );

await press.ArrowDown();
expect(
screen.getByRole( 'menuitem', { name: 'Menu item 3' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'Menu item 3' );

await press.ArrowDown();
expect(
screen.getByRole( 'menuitem', { name: 'Menu item 1' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'Menu item 1' );

await press.ArrowUp();
expect(
screen.getByRole( 'menuitem', { name: 'Menu item 3' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'Menu item 3' );

await press.ArrowUp();
expect(
screen.getByRole( 'menuitem', { name: 'Submenu trigger item' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'Submenu trigger item' );

// Arrow right/left can be used to enter/leave submenus
await press.ArrowRight();
expect(
screen.getByRole( 'menuitem', {
name: 'Submenu item 1',
} )
).toHaveFocus();
await waitForFocusedMenuItem( 'Submenu item 1' );

await press.ArrowDown();
expect(
screen.getByRole( 'menuitem', {
name: 'Submenu item 2',
} )
).toHaveFocus();
await waitForFocusedMenuItem( 'Submenu item 2' );

await press.ArrowLeft();
expect(
screen.getByRole( 'menuitem', {
name: 'Submenu trigger item',
} )
).toHaveFocus();
await waitForFocusedMenuItem( 'Submenu trigger item' );

// Spacebar or enter key can also be used to enter a submenu
await press.Enter();
expect(
screen.getByRole( 'menuitem', {
name: 'Submenu item 1',
} )
).toHaveFocus();
await waitForFocusedMenuItem( 'Submenu item 1' );

await press.ArrowLeft();
expect(
screen.getByRole( 'menuitem', {
name: 'Submenu trigger item',
} )
).toHaveFocus();
await waitForFocusedMenuItem( 'Submenu trigger item' );

await press.Space();
expect(
screen.getByRole( 'menuitem', {
name: 'Submenu item 1',
} )
).toHaveFocus();
await waitForFocusedMenuItem( 'Submenu item 1' );

await press.ArrowLeft();
expect(
screen.getByRole( 'menuitem', {
name: 'Submenu trigger item',
} )
).toHaveFocus();
await waitForFocusedMenuItem( 'Submenu trigger item' );
} );

it( 'should check radio items and keep the menu open when clicking (controlled)', async () => {
Expand Down Expand Up @@ -886,7 +847,7 @@ describe( 'Menu', () => {
);

// Menu open, focus is on the menu wrapper
expect( screen.getByRole( 'menu' ) ).toHaveFocus();
await waitForFocusedMenu();

expect(
screen.queryByRole( 'button', {
Expand Down Expand Up @@ -916,7 +877,7 @@ describe( 'Menu', () => {
);

// Menu open, focus is on the menu wrapper
expect( screen.getByRole( 'menu' ) ).toHaveFocus();
await waitForFocusedMenu();

// Menu is not modal, therefore the outer button is part of the
// accessibility tree and can be found.
Expand All @@ -927,8 +888,10 @@ describe( 'Menu', () => {
// The outer button can be focused by pressing tab. Doing so will cause
// the Menu to close.
await press.Tab();
expect( outerButton ).toBeInTheDocument();
expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument();
await waitFor( () => {
expect( outerButton ).toHaveFocus();
expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument();
} );

@aduth aduth Apr 20, 2026

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.

Minor:

  • Do we need to be awaiting both of these things (i.e. is each independently flakey) or can we await only the focus change and then break out the remaining assertion.
  • Related point: I wonder if we can reuse the utility for detecting the outer button, so we'd have something like:
Suggested change
await waitFor( () => {
expect( outerButton ).toHaveFocus();
expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument();
} );
await waitForFocusedButton( 'Button outside of dropdown' );
expect( screen.queryByRole( 'menu' ) ).not.toBeInTheDocument();

@mirka mirka Apr 22, 2026

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.

Trimmed the changeset down further so it generally only addresses the historically-known failures (listed in the PR description). Passed on 10 test runs, so good enough as a first try. We can expand if needed.

} );
} );

Expand Down Expand Up @@ -1068,23 +1031,19 @@ describe( 'Menu', () => {
name: 'Open dropdown',
} )
);
expect( screen.getByRole( 'menu' ) ).toHaveFocus();
await waitForFocusedMenu();

// Type "tw", it should match and focus the item with content "Two"
await type( 'tw' );
expect(
screen.getByRole( 'menuitem', { name: 'Two' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'Two' );

// Wait for the typeahead timer to reset and interpret
// the next keystrokes as a new search
await delay( 500 );

// Type "on", it should match and focus the item with content "One"
await type( 'on' );
expect(
screen.getByRole( 'menuitem', { name: 'One' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'One' );
} );

it( 'should keep previous focus when no matches are found', async () => {
Expand All @@ -1104,41 +1063,35 @@ describe( 'Menu', () => {
name: 'Open dropdown',
} )
);
expect( screen.getByRole( 'menu' ) ).toHaveFocus();
await waitForFocusedMenu();

// Type a string that doesn't match any items. Focus shouldn't move.
await type( 'abc' );
expect( screen.getByRole( 'menu' ) ).toHaveFocus();
await waitForFocusedMenu();

// Wait for the typeahead timer to reset and interpret
// the next keystrokes as a new search
await delay( 500 );

// Type "on", it should match and focus the item with content "One"
await type( 'on' );
expect(
screen.getByRole( 'menuitem', { name: 'One' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'One' );

// Wait for the typeahead timer to reset and interpret
// the next keystrokes as a new search
await delay( 500 );

// Type a string that doesn't match any items. Focus shouldn't move.
await type( 'abc' );
expect(
screen.getByRole( 'menuitem', { name: 'One' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'One' );

// Wait for the typeahead timer to reset and interpret
// the next keystrokes as a new search
await delay( 500 );

// Type "tw", it should match and focus the item with content "Two"
await type( 'tw' );
expect(
screen.getByRole( 'menuitem', { name: 'Two' } )
).toHaveFocus();
await waitForFocusedMenuItem( 'Two' );
} );
} );
} );
Loading