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

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}');
         });
     });
 

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();
+});

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"`
 }
 

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<Props> {
-    buttonRefs: Map<string, RefObject<HTMLButtonElement>>;
+    buttonRefs: Map<string, HTMLButtonElement>;
 
     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<Props> {
         (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<Props> {
         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<Props> {
                 {tab.newGroup && <hr/>}
                 <button
                     data-testid={`${tab.name}-tab-button`}
-                    ref={this.buttonRefs.get(tab.name)}
+                    ref={(element: HTMLButtonElement) => {
+                        if (element) {
+                            this.buttonRefs.set(tab.name, element);
+                        } else {
+                            this.buttonRefs.delete(tab.name);
+                        }
+                    }}
                     id={`${tab.name}Button`}
                     className={classNames('cursor--pointer style--none nav-pills__tab', {active: isActive})}
                     onClick={this.handleClick.bind(null, tab)}
-                    onKeyUp={this.handleKeyUp.bind(null, tab)}
+                    onKeyDown={this.handleKeyDown.bind(null, tab)}
                     aria-label={tab.uiName.toLowerCase()}
                     role='tab'
                     aria-selected={isActive}

webapp/channels/src/sass/routes/_settings.scss

@@ -648,6 +648,12 @@
                     }
                 }
             }
+
+            // Remove this when MM-62778 is completed
+            &:focus-visible {
+                border-radius: 4px;
+                box-shadow: 0 0 1px 3px rgba(var(--link-color-rgb), 0.5), 0 0 0 1px var(--link-color);
+            }
         }
     }