Skip to content

fix: Keyboard activation doesn't toggle Display menu controls #41089

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

Open
juliajforesti wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

fix: Keyboard activation doesn't toggle Display menu controls #41089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94f573f
fix: add `onClick` to display menu items
juliajforesti Jun 26, 2026
33cbb1b
test: add display menu a11y tests
juliajforesti Jun 26, 2026
3d45f5b
chore: changeset
juliajforesti Jun 26, 2026
cb9f9fa
fix: set noop `onChange` on Display menu addons to prevent double sta…
juliajforesti Jun 26, 2026
ed193b7
test: change default `unread` reset
juliajforesti Jun 26, 2026
784df51
test: remove condition since avatar is corretly being reset to default
juliajforesti Jun 29, 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
5 changes: 5 additions & 0 deletions .changeset/olive-nails-fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Enables keyboard activation for Display menu radio buttons and checkboxes
9 changes: 6 additions & 3 deletions apps/meteor/client/navbar/NavBarPagesGroup/hooks/useGroupingListItems.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,22 @@ export const useGroupingListItems = (): GenericMenuItemProps[] => {
id: 'unread',
content: t('Unread'),
icon: 'flag',
addon: <CheckBox onChange={handleChangeShowUnread} checked={sidebarShowUnread} />,
onClick: handleChangeShowUnread,
addon: <CheckBox checked={sidebarShowUnread} onChange={() => undefined} />,
},
!secondSidebarEnabled && {
id: 'favorites',
content: t('Favorites'),
icon: 'star',
addon: <CheckBox onChange={handleChangeShoFavorite} checked={sidebarShowFavorites} />,
onClick: handleChangeShoFavorite,
addon: <CheckBox checked={sidebarShowFavorites} onChange={() => undefined} />,
},
{
id: 'types',
content: t('Types'),
icon: 'group-by-type',
addon: <CheckBox onChange={handleChangeGroupByType} checked={sidebarGroupByType} />,
onClick: handleChangeGroupByType,
addon: <CheckBox checked={sidebarGroupByType} onChange={() => undefined} />,
},
].filter(Boolean) as GenericMenuItemProps[];
};
6 changes: 4 additions & 2 deletions apps/meteor/client/navbar/NavBarPagesGroup/hooks/useSortModeItems.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,16 @@ export const useSortModeItems = (): GenericMenuItemProps[] => {
id: 'activity',
content: t('Activity'),
icon: 'clock',
addon: <RadioButton onChange={setToActivity} checked={sidebarSortBy === 'activity'} />,
onClick: setToActivity,
addon: <RadioButton checked={sidebarSortBy === 'activity'} onChange={() => undefined} />,
description: sidebarSortBy === 'activity' && isOmnichannelEnabled && <OmnichannelSortingDisclaimer />,
},
{
id: 'name',
content: t('Name'),
icon: 'sort-az',
addon: <RadioButton onChange={setToAlphabetical} checked={sidebarSortBy === 'alphabetical'} />,
onClick: setToAlphabetical,
addon: <RadioButton checked={sidebarSortBy === 'alphabetical'} onChange={() => undefined} />,
description: sidebarSortBy === 'alphabetical' && isOmnichannelEnabled && <OmnichannelSortingDisclaimer />,
},
];
Expand Down
12 changes: 8 additions & 4 deletions apps/meteor/client/navbar/NavBarPagesGroup/hooks/useViewModeItems.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,25 +29,29 @@ export const useViewModeItems = (): GenericMenuItemProps[] => {
id: 'extended',
content: t('Extended'),
icon: 'extended-view',
addon: <RadioButton onChange={setToExtended} checked={sidebarViewMode === 'extended'} />,
onClick: setToExtended,
addon: <RadioButton checked={sidebarViewMode === 'extended'} onChange={() => undefined} />,
},
{
id: 'medium',
content: t('Medium'),
icon: 'medium-view',
addon: <RadioButton onChange={setToMedium} checked={sidebarViewMode === 'medium'} />,
onClick: setToMedium,
addon: <RadioButton checked={sidebarViewMode === 'medium'} onChange={() => undefined} />,
},
{
id: 'condensed',
content: t('Condensed'),
icon: 'condensed-view',
addon: <RadioButton onChange={setToCondensed} checked={sidebarViewMode === 'condensed'} />,
onClick: setToCondensed,
addon: <RadioButton checked={sidebarViewMode === 'condensed'} onChange={() => undefined} />,
},
{
id: 'avatars',
content: t('Avatars'),
icon: 'user-rounded',
addon: <ToggleSwitch onChange={handleChangeSidebarDisplayAvatar} checked={sidebarDisplayAvatar} />,
onClick: handleChangeSidebarDisplayAvatar,
addon: <ToggleSwitch checked={sidebarDisplayAvatar} onChange={() => undefined} />,
},
];
};
24 changes: 24 additions & 0 deletions apps/meteor/tests/e2e/page-objects/fragments/navbar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,30 @@ export class Navbar {
return this.root.getByRole('menu', { name: 'Display' });
}

get groupDisplay(): Locator {
return this.menuDisplay.getByRole('group', { name: 'Display' });
}

getDisplayMenuItem(mode: 'Extended' | 'Medium' | 'Condensed' | 'Avatars'): Locator {
return this.groupDisplay.getByRole('menuitemcheckbox', { name: mode });
}

get groupSortBy(): Locator {
return this.menuDisplay.getByRole('group', { name: 'Sort by' });
}

getSortMenuItem(mode: 'Activity' | 'Name'): Locator {
return this.groupSortBy.getByRole('menuitemcheckbox', { name: mode });
}

get groupGroupBy(): Locator {
return this.menuDisplay.getByRole('group', { name: 'Group by' });
}

getGroupByMenuItem(mode: 'Unread' | 'Favorites' | 'Types'): Locator {
return this.groupGroupBy.getByRole('menuitemcheckbox', { name: mode });
}

get btnCreateNew(): Locator {
return this.pagesGroup.getByRole('button', { name: 'Create new' });
}
Expand Down
65 changes: 65 additions & 0 deletions apps/meteor/tests/e2e/sidebar.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,71 @@ test.describe.serial('Sidebar', () => {
});
});

test.describe('Display menu keyboard accessibility', () => {
test.afterEach(async ({ api }) => {

@cubic-dev-ai cubic-dev-ai Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: These tests only reset preferences in afterEach but never establish the required starting state via beforeEach. This makes the test suite order-dependent: the first test runs against unknown state, and if any test fails mid-execution, afterEach may not run, leaving stale state for subsequent tests. Move (or duplicate) this preference initialization into a beforeEach block to ensure each test starts from a known state.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/e2e/sidebar.spec.ts, line 78:

<comment>These tests only reset preferences in `afterEach` but never establish the required starting state via `beforeEach`. This makes the test suite order-dependent: the first test runs against unknown state, and if any test fails mid-execution, `afterEach` may not run, leaving stale state for subsequent tests. Move (or duplicate) this preference initialization into a `beforeEach` block to ensure each test starts from a known state.</comment>

<file context>
@@ -74,6 +74,71 @@ test.describe.serial('Sidebar', () => {
 	});
 
+	test.describe('Display menu keyboard accessibility', () => {
+		test.afterEach(async ({ api }) => {
+			await api.post('/users.setPreferences', {
+				data: {
</file context>
Suggested change
test.afterEach(async ({ api }) => {
test.beforeEach(async ({ api }) => {
await api.post('/users.setPreferences', {
data: {
sidebarViewMode: 'extended',
sidebarDisplayAvatar: true,
sidebarSortby: 'activity',
sidebarShowUnread: false,
},
});
});
test.afterEach(async ({ api }) => {

await api.post('/users.setPreferences', {
data: {
sidebarViewMode: 'extended',
sidebarDisplayAvatar: true,
sidebarSortby: 'activity',
sidebarShowUnread: false,
},
});
});
Comment on lines +78 to +87

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Seed these preferences before each test, not only after.

Lines 78-87 clean up persisted sidebar prefs, but they never establish the starting state for the test that is about to run. That makes this block order-dependent: e.g. should toggle Avatars using keyboard and should toggle grouping using keyboard only pass if a prior case already reset the admin user's prefs. Initialize the same values in beforeEach (and keep afterEach only as cleanup if needed).

Suggested fix 
 test.describe('Display menu keyboard accessibility', () => {
+	test.beforeEach(async ({ api }) => {
+		await api.post('/users.setPreferences', {
+			data: {
+				sidebarViewMode: 'extended',
+				sidebarDisplayAvatar: true,
+				sidebarSortby: 'activity',
+				sidebarShowUnread: false,
+			},
+		});
+	});
+
 	test.afterEach(async ({ api }) => {
 		await api.post('/users.setPreferences', {
 			data: {
 				sidebarViewMode: 'extended',
 				sidebarDisplayAvatar: true,
 				sidebarSortby: 'activity',
 				sidebarShowUnread: false,
 			},
 		});
 	});

As per coding guidelines, "Ensure clean state for each test execution in Playwright tests" and "Maintain test isolation between test cases in Playwright tests".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test.afterEach(async ({ api }) => {
await api.post('/users.setPreferences', {
data: {
sidebarViewMode: 'extended',
sidebarDisplayAvatar: true,
sidebarSortby: 'activity',
sidebarShowUnread: false,
},
});
});
test.beforeEach(async ({ api }) => {
await api.post('/users.setPreferences', {
data: {
sidebarViewMode: 'extended',
sidebarDisplayAvatar: true,
sidebarSortby: 'activity',
sidebarShowUnread: false,
},
});
});
test.afterEach(async ({ api }) => {
await api.post('/users.setPreferences', {
data: {
sidebarViewMode: 'extended',
sidebarDisplayAvatar: true,
sidebarSortby: 'activity',
sidebarShowUnread: false,
},
});
});
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/meteor/tests/e2e/sidebar.spec.ts` around lines 78 - 87, The sidebar
preference setup in sidebar.spec.ts is only running in test.afterEach, so the
starting state is not guaranteed and tests become order-dependent. Move the same
/users.setPreferences call into test.beforeEach in the sidebar tests so each
case starts with the expected sidebarViewMode, sidebarDisplayAvatar,
sidebarSortby, and sidebarShowUnread values, and keep test.afterEach only for
any necessary cleanup. Use the existing test hooks around the sidebar test suite
to ensure isolation between cases.

Source: Coding guidelines


test('should change view mode using keyboard', async ({ page }) => {
await poHomeChannel.navbar.btnDisplay.focus();
await page.keyboard.press('Enter');
await expect(poHomeChannel.navbar.menuDisplay).toBeVisible();

await poHomeChannel.navbar.getDisplayMenuItem('Medium').focus();

await page.keyboard.press('Space');

await expect(poHomeChannel.navbar.getDisplayMenuItem('Medium').getByRole('radio')).toBeChecked();
await page.keyboard.press('Escape');
});

test('should toggle Avatars using keyboard', async ({ page }) => {
await poHomeChannel.navbar.btnDisplay.click();
const avatarsItem = poHomeChannel.navbar.getDisplayMenuItem('Avatars');

await avatarsItem.focus();
await page.keyboard.press('Space');

const newAvatarsItem = poHomeChannel.navbar.getDisplayMenuItem('Avatars');
await expect(newAvatarsItem.getByRole('checkbox')).not.toBeChecked();
await page.keyboard.press('Escape');
});

test('should change sort mode using keyboard', async ({ page }) => {
await poHomeChannel.navbar.btnDisplay.focus();
await page.keyboard.press('Enter');
await expect(poHomeChannel.navbar.menuDisplay).toBeVisible();
await poHomeChannel.navbar.getSortMenuItem('Activity').focus();
await page.keyboard.press('ArrowDown');
await expect(poHomeChannel.navbar.getSortMenuItem('Name')).toBeFocused();
await page.keyboard.press('Space');

await expect(poHomeChannel.navbar.getSortMenuItem('Name').getByRole('radio')).toBeChecked();
await page.keyboard.press('Escape');
});

test('should toggle grouping using keyboard', async ({ page }) => {
await poHomeChannel.navbar.btnDisplay.focus();
await page.keyboard.press('Enter');

const unreadItem = poHomeChannel.navbar.getGroupByMenuItem('Unread');
await expect(unreadItem.getByRole('checkbox')).not.toBeChecked();

await unreadItem.focus();
await page.keyboard.press('Space');

await expect(poHomeChannel.navbar.getGroupByMenuItem('Unread').getByRole('checkbox')).toBeChecked();
await page.keyboard.press('Escape');
});
});

test.describe('sidebar', async () => {
test('should navigate on sidebar items using arrow keys and restore focus', async ({ page }) => {
// focus should be on the next item
Expand Down
Loading