From 5ffb7607cc68e3542e65d6539c4fe723328cc88b Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Fri, 27 Jun 2025 14:01:53 -0400 Subject: [PATCH] MM-64669 Fix keyboard navigation of settings sidebar (#32098) * MM-64669 Fix keyboard navigation of settings sidebar and add Playwright test * MM-64669 Revert changes to Cypress test which masked keyboard bug The changes that were previously made caused Cypress to refocus the sidebar repeatedly which stopped the test from failing without fixing the bug. * Ensure focus highlight is always visible on sidebar tabs This may not have been broken by the changes that caused MM-64669, but I noticed it while I was in there and wanted to fix it. * Fix settings modal scrolling while changing sections using arrow keys * Remove accidentally-added field --- .../accessibility_account_settings_spec.js | 14 ++-- .../channels/account_settings_sidebar.spec.ts | 68 +++++++++++++++++++ server/public/model/channel.go | 1 - .../settings_sidebar/settings_sidebar.tsx | 49 ++++--------- .../channels/src/sass/routes/_settings.scss | 6 ++ 5 files changed, 91 insertions(+), 47 deletions(-) create mode 100644 e2e-tests/playwright/specs/accessibility/channels/account_settings_sidebar.spec.ts diff --git a/e2e-tests/cypress/tests/integration/channels/accessibility/accessibility_account_settings_spec.js b/e2e-tests/cypress/tests/integration/channels/accessibility/accessibility_account_settings_spec.js index 8c0c701c5a..580cd7a6de 100644 --- a/e2e-tests/cypress/tests/integration/channels/accessibility/accessibility_account_settings_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/accessibility/accessibility_account_settings_spec.js @@ -89,14 +89,12 @@ describe('Verify Accessibility Support in different sections in Settings and Pro }); it('MM-T1465_1 Verify Label & Tab behavior in section links', () => { - // * Verify tab selection and keyboard navigation in Account settings modal + // * Verify aria-label and tab support in section of Account settings modal cy.uiOpenProfileModal('Profile Settings'); cy.findByRole('tab', {name: 'profile settings'}).should('be.visible').focus().should('be.focused'); ['profile settings', 'security'].forEach((text) => { - // * Verify each tab is correctly selected and supports navigating to the next tab with arrow keys - cy.findByRole('tab', {name: text}). - should('have.attr', 'aria-selected', 'true'). - type('{downarrow}'); + // * Verify aria-label on each tab and it supports navigating to the next tab with arrow keys + cy.focused().should('have.attr', 'aria-label', text).type('{downarrow}'); }); cy.uiClose(); @@ -104,10 +102,8 @@ describe('Verify Accessibility Support in different sections in Settings and Pro cy.uiOpenSettingsModal(); cy.findByRole('tab', {name: 'notifications'}).should('be.visible').focus().should('be.focused'); ['notifications', 'display', 'sidebar', 'advanced'].forEach((text) => { - // * Verify each tab is correctly selected and supports navigating to the next tab with arrow keys - cy.findByRole('tab', {name: text}). - should('have.attr', 'aria-selected', 'true'). - type('{downarrow}'); + // * Verify aria-label on each tab and it supports navigating to the next tab with arrow keys + cy.focused().should('have.attr', 'aria-label', text).type('{downarrow}'); }); }); diff --git a/e2e-tests/playwright/specs/accessibility/channels/account_settings_sidebar.spec.ts b/e2e-tests/playwright/specs/accessibility/channels/account_settings_sidebar.spec.ts new file mode 100644 index 0000000000..c630d0bde8 --- /dev/null +++ b/e2e-tests/playwright/specs/accessibility/channels/account_settings_sidebar.spec.ts @@ -0,0 +1,68 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {expect, test} from '@mattermost/playwright-lib'; + +test('Settings sidebar should be keyboard accessible', async ({axe, pw}) => { + const {user} = await pw.initSetup(); + + // # Log in as a user in new browser context + const {page, channelsPage} = await pw.testBrowser.login(user); + + // # Initialize Axe + const ab = axe.builder(page).disableRules([ + 'color-contrast', + + // Known issue: These fail due to the way we've grouped plugin setting tabs together in the LHS + 'aria-required-children', + 'aria-required-parent', + ]); + + // # Visit default channel page + await channelsPage.goto(); + await channelsPage.toBeVisible(); + + // # Open settings modal + const settingsModal = await channelsPage.globalHeader.openSettings(); + + // * The settings modal should have no accessibility violations + const accessibilityScanResults = await ab.analyze(); + expect(accessibilityScanResults.violations).toHaveLength(0); + + // # Focus the sidebar + await settingsModal.container.focus(); + await page.keyboard.press('Tab'); + await page.keyboard.press('Tab'); + + // * The Notifications tab should start focused + await expect(page.getByRole('tab', {name: 'Notifications'})).toBeFocused(); + await expect(page.getByText('Desktop and mobile notifications')).toBeVisible(); + + // * Pressing the down arrow should focus and show the Display tab + await page.keyboard.press('ArrowDown'); + await expect(page.getByRole('tab', {name: 'Display'})).toBeFocused(); + await expect(page.getByText('Theme', {exact: true})).toBeVisible(); + + // * Pressing the down arrow should focus and show the Sidebar tab + await page.keyboard.press('ArrowDown'); + await expect(page.getByRole('tab', {name: 'Sidebar'})).toBeFocused(); + await expect(page.getByText('Group unread channels separately')).toBeVisible(); + + // * Pressing the down arrow should focus and show the Advanced tab + await page.keyboard.press('ArrowDown'); + await expect(page.getByRole('tab', {name: 'Advanced'})).toBeFocused(); + await expect(page.getByText('Enable Post Formatting')).toBeVisible(); + + // * Pressing the up arrow should go back through the tabs + await page.keyboard.press('ArrowUp'); + await expect(page.getByRole('tab', {name: 'Sidebar'})).toBeFocused(); + await expect(page.getByText('Group unread channels separately')).toBeVisible(); + + await page.keyboard.press('ArrowUp'); + await expect(page.getByRole('tab', {name: 'Display'})).toBeFocused(); + await expect(page.getByText('Theme', {exact: true})).toBeVisible(); + + await page.keyboard.press('ArrowUp'); + await expect(page.getByRole('tab', {name: 'Notifications'})).toBeFocused(); + await expect(page.getByText('Desktop and mobile notifications')).toBeVisible(); +}); diff --git a/server/public/model/channel.go b/server/public/model/channel.go index 919fd7a53f..615bbc75cf 100644 --- a/server/public/model/channel.go +++ b/server/public/model/channel.go @@ -147,7 +147,6 @@ type ChannelPatch struct { Header *string `json:"header"` Purpose *string `json:"purpose"` GroupConstrained *bool `json:"group_constrained"` - Type ChannelType `json:"type"` BannerInfo *ChannelBannerInfo `json:"banner_info"` } diff --git a/webapp/channels/src/components/settings_sidebar/settings_sidebar.tsx b/webapp/channels/src/components/settings_sidebar/settings_sidebar.tsx index eb8c9f01f4..8f29a55ded 100644 --- a/webapp/channels/src/components/settings_sidebar/settings_sidebar.tsx +++ b/webapp/channels/src/components/settings_sidebar/settings_sidebar.tsx @@ -3,7 +3,6 @@ import classNames from 'classnames'; import React from 'react'; -import type {RefObject} from 'react'; import {FormattedMessage} from 'react-intl'; import Constants from 'utils/constants'; @@ -27,42 +26,12 @@ export type Props = { }; export default class SettingsSidebar extends React.PureComponent { - buttonRefs: Map>; + buttonRefs: Map; constructor(props: Props) { super(props); - // Initialize an empty Map for button refs this.buttonRefs = new Map(); - - // Initialize refs for all tabs - this.initializeButtonRefs(props.tabs, props.pluginTabs); - } - - // Initialize or update button refs for all tabs - private initializeButtonRefs(tabs: Tab[], pluginTabs?: Tab[]) { - // Clear existing refs if reinitializing - this.buttonRefs.clear(); - - // Create refs for all tabs, regardless of display status - tabs.forEach((tab) => { - this.buttonRefs.set(tab.name, React.createRef()); - }); - - // Create refs for plugin tabs if they exist - if (pluginTabs?.length) { - pluginTabs.forEach((tab) => { - this.buttonRefs.set(tab.name, React.createRef()); - }); - } - } - - // Update refs when props change - componentDidUpdate(prevProps: Props) { - // Check if tabs or pluginTabs have changed - if (prevProps.tabs !== this.props.tabs || prevProps.pluginTabs !== this.props.pluginTabs) { - this.initializeButtonRefs(this.props.tabs, this.props.pluginTabs); - } } // Get all visible tabs in the correct order @@ -78,13 +47,13 @@ export default class SettingsSidebar extends React.PureComponent { (e.target as Element).closest('.settings-modal')?.classList.add('display--content'); }; - public handleKeyUp = (tab: Tab, e: React.KeyboardEvent) => { + public handleKeyDown = (tab: Tab, e: React.KeyboardEvent) => { // Only handle UP and DOWN arrow keys if (!isKeyPressed(e, Constants.KeyCodes.UP) && !isKeyPressed(e, Constants.KeyCodes.DOWN)) { return; } - // Prevent default behavior + // Prevent scrolling e.preventDefault(); // Get all visible tabs @@ -121,7 +90,7 @@ export default class SettingsSidebar extends React.PureComponent { this.props.updateTab(targetTab.name); // Focus the target tab button directly - const targetButton = this.buttonRefs.get(targetTab.name)?.current; + const targetButton = this.buttonRefs.get(targetTab.name); if (targetButton) { // Use direct focus instead of a11yFocus to ensure Cypress tests can detect the focus change targetButton.focus(); @@ -155,11 +124,17 @@ export default class SettingsSidebar extends React.PureComponent { {tab.newGroup &&
}