Fix accessibility regressions (#7336)

t3chguy · web-flow · commit a667677c57f7 · 2021-12-14T14:27:35.000Z
* Fix room list roving treeview

New TooltipTarget &amp; TextWithTooltip were not roving-accessible

* Fix programmatic focus management in roving tab index not triggering onFocus handler

* Fix toolbar no longer handling left &amp; right arrows

* Fix roving tab index focus tracking on interactive element like context menu trigger

* Fix thread list context menu roving

* add comment

* fix comment

* Fix handling vertical arrows in the wrong direction

* iterate PR

* delint

* tidy up
diff --git a/src/accessibility/RovingTabIndex.tsx b/src/accessibility/RovingTabIndex.tsx
@@ -131,6 +131,8 @@ export const reducer = (state: IState, action: IAction) => {
         }
 
         case Type.SetFocus: {
+            // if the ref doesn't change just return the same object reference to skip a re-render
+            if (state.activeRef === action.payload.ref) return state;
             // update active ref
             state.activeRef = action.payload.ref;
             return { ...state };
@@ -194,6 +196,7 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
         }
 
         let handled = false;
+        let focusRef: RefObject<HTMLElement>;
         // Don't interfere with input default keydown behaviour
         if (ev.target.tagName !== "INPUT" && ev.target.tagName !== "TEXTAREA") {
             // check if we actually have any items
@@ -202,36 +205,38 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
                     if (handleHomeEnd) {
                         handled = true;
                         // move focus to first (visible) item
-                        findSiblingElement(context.state.refs, 0)?.current?.focus();
+                        focusRef = findSiblingElement(context.state.refs, 0);
                     }
                     break;
 
                 case Key.END:
                     if (handleHomeEnd) {
                         handled = true;
                         // move focus to last (visible) item
-                        findSiblingElement(context.state.refs, context.state.refs.length - 1, true)?.current?.focus();
+                        focusRef = findSiblingElement(context.state.refs, context.state.refs.length - 1, true);
                     }
                     break;
 
-                case Key.ARROW_UP:
+                case Key.ARROW_DOWN:
                 case Key.ARROW_RIGHT:
-                    if ((ev.key === Key.ARROW_UP && handleUpDown) || (ev.key === Key.ARROW_RIGHT && handleLeftRight)) {
+                    if ((ev.key === Key.ARROW_DOWN && handleUpDown) ||
+                        (ev.key === Key.ARROW_RIGHT && handleLeftRight)
+                    ) {
                         handled = true;
                         if (context.state.refs.length > 0) {
                             const idx = context.state.refs.indexOf(context.state.activeRef);
-                            findSiblingElement(context.state.refs, idx - 1)?.current?.focus();
+                            focusRef = findSiblingElement(context.state.refs, idx + 1);
                         }
                     }
                     break;
 
-                case Key.ARROW_DOWN:
+                case Key.ARROW_UP:
                 case Key.ARROW_LEFT:
-                    if ((ev.key === Key.ARROW_DOWN && handleUpDown) || (ev.key === Key.ARROW_LEFT && handleLeftRight)) {
+                    if ((ev.key === Key.ARROW_UP && handleUpDown) || (ev.key === Key.ARROW_LEFT && handleLeftRight)) {
                         handled = true;
                         if (context.state.refs.length > 0) {
                             const idx = context.state.refs.indexOf(context.state.activeRef);
-                            findSiblingElement(context.state.refs, idx + 1, true)?.current?.focus();
+                            focusRef = findSiblingElement(context.state.refs, idx - 1, true);
                         }
                     }
                     break;
@@ -242,7 +247,18 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
             ev.preventDefault();
             ev.stopPropagation();
         }
-    }, [context.state, onKeyDown, handleHomeEnd, handleUpDown, handleLeftRight]);
+
+        if (focusRef) {
+            focusRef.current?.focus();
+            // programmatic focus doesn't fire the onFocus handler, so we must do the do ourselves
+            dispatch({
+                type: Type.SetFocus,
+                payload: {
+                    ref: focusRef,
+                },
+            });
+        }
+    }, [context, onKeyDown, handleHomeEnd, handleUpDown, handleLeftRight]);
 
     return <RovingTabIndexContext.Provider value={context}>
         { children({ onKeyDownHandler }) }
@@ -283,7 +299,7 @@ export const useRovingTabIndex = (inputRef?: Ref): [FocusHandler, boolean, Ref]
             type: Type.SetFocus,
             payload: { ref },
         });
-    }, [ref, context]);
+    }, []); // eslint-disable-line react-hooks/exhaustive-deps
 
     const isActive = context.state.activeRef === ref;
     return [onFocus, isActive, ref];
diff --git a/src/accessibility/Toolbar.tsx b/src/accessibility/Toolbar.tsx
@@ -52,7 +52,7 @@ const Toolbar: React.FC<IProps> = ({ children, ...props }) => {
         }
     };
 
-    return <RovingTabIndexProvider handleHomeEnd={true} onKeyDown={onKeyDown}>
+    return <RovingTabIndexProvider handleHomeEnd handleLeftRight onKeyDown={onKeyDown}>
         { ({ onKeyDownHandler }) => <div {...props} onKeyDown={onKeyDownHandler} role="toolbar">
             { children }
         </div> }
diff --git a/src/components/views/avatars/DecoratedRoomAvatar.tsx b/src/components/views/avatars/DecoratedRoomAvatar.tsx
@@ -31,6 +31,7 @@ import TextWithTooltip from "../elements/TextWithTooltip";
 import DMRoomMap from "../../../utils/DMRoomMap";
 import { replaceableComponent } from "../../../utils/replaceableComponent";
 import { IOOBData } from "../../../stores/ThreepidInviteStore";
+import TooltipTarget from "../elements/TooltipTarget";
 
 interface IProps {
     room: Room;
@@ -39,6 +40,7 @@ interface IProps {
     forceCount?: boolean;
     oobData?: IOOBData;
     viewAvatarOnClick?: boolean;
+    tooltipProps?: Omit<React.ComponentProps<typeof TooltipTarget>, "label" | "tooltipClassName" | "className">;
 }
 
 interface IState {
@@ -189,6 +191,7 @@ export default class DecoratedRoomAvatar extends React.PureComponent<IProps, ISt
         if (this.state.icon !== Icon.None) {
             icon = <TextWithTooltip
                 tooltip={tooltipText(this.state.icon)}
+                tooltipProps={this.props.tooltipProps}
                 class={`mx_DecoratedRoomAvatar_icon mx_DecoratedRoomAvatar_icon_${this.state.icon.toLowerCase()}`}
             />;
         }
diff --git a/src/components/views/context_menus/ThreadListContextMenu.tsx b/src/components/views/context_menus/ThreadListContextMenu.tsx
@@ -14,26 +14,34 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React, { useCallback, useEffect, useState } from "react";
+import React, { RefObject, useCallback, useEffect } from "react";
 import { MatrixEvent } from "matrix-js-sdk/src";
 
 import { ButtonEvent } from "../elements/AccessibleButton";
 import dis from '../../../dispatcher/dispatcher';
 import { Action } from "../../../dispatcher/actions";
 import { RoomPermalinkCreator } from "../../../utils/permalinks/Permalinks";
 import { copyPlaintext } from "../../../utils/strings";
-import { ChevronFace, ContextMenuTooltipButton } from "../../structures/ContextMenu";
+import { ChevronFace, ContextMenuTooltipButton, useContextMenu } from "../../structures/ContextMenu";
 import { _t } from "../../../languageHandler";
 import IconizedContextMenu, { IconizedContextMenuOption, IconizedContextMenuOptionList } from "./IconizedContextMenu";
 import { WidgetLayoutStore } from "../../../stores/widgets/WidgetLayoutStore";
 import { MatrixClientPeg } from "../../../MatrixClientPeg";
+import { useRovingTabIndex } from "../../../accessibility/RovingTabIndex";
 
 interface IProps {
     mxEvent: MatrixEvent;
     permalinkCreator: RoomPermalinkCreator;
     onMenuToggle?: (open: boolean) => void;
 }
 
+interface IExtendedProps extends IProps {
+    // Props for making the button into a roving one
+    tabIndex?: number;
+    inputRef?: RefObject<HTMLElement>;
+    onFocus?(): void;
+}
+
 const contextMenuBelow = (elementRect: DOMRect) => {
     // align the context menu's icons with the icon which opened the context menu
     const left = elementRect.left + window.pageXOffset + elementRect.width;
@@ -42,11 +50,27 @@ const contextMenuBelow = (elementRect: DOMRect) => {
     return { left, top, chevronFace };
 };
 
-const ThreadListContextMenu: React.FC<IProps> = ({ mxEvent, permalinkCreator, onMenuToggle }) => {
-    const [optionsPosition, setOptionsPosition] = useState(null);
-    const closeThreadOptions = useCallback(() => {
-        setOptionsPosition(null);
-    }, []);
+export const RovingThreadListContextMenu: React.FC<IProps> = (props) => {
+    const [onFocus, isActive, ref] = useRovingTabIndex();
+
+    return <ThreadListContextMenu
+        {...props}
+        onFocus={onFocus}
+        tabIndex={isActive ? 0 : -1}
+        inputRef={ref}
+    />;
+};
+
+const ThreadListContextMenu: React.FC<IExtendedProps> = ({
+    mxEvent,
+    permalinkCreator,
+    onMenuToggle,
+    onFocus,
+    inputRef,
+    ...props
+}) => {
+    const [menuDisplayed, _ref, openMenu, closeThreadOptions] = useContextMenu();
+    const button = inputRef ?? _ref; // prefer the ref we receive via props in case we are being controlled
 
     const viewInRoom = useCallback((evt: ButtonEvent): void => {
         evt.preventDefault();
@@ -68,37 +92,31 @@ const ThreadListContextMenu: React.FC<IProps> = ({ mxEvent, permalinkCreator, on
         closeThreadOptions();
     }, [mxEvent, closeThreadOptions, permalinkCreator]);
 
-    const toggleOptionsMenu = useCallback((ev: ButtonEvent): void => {
-        if (!!optionsPosition) {
-            closeThreadOptions();
-        } else {
-            const position = ev.currentTarget.getBoundingClientRect();
-            setOptionsPosition(position);
-        }
-    }, [closeThreadOptions, optionsPosition]);
-
     useEffect(() => {
         if (onMenuToggle) {
-            onMenuToggle(!!optionsPosition);
+            onMenuToggle(menuDisplayed);
         }
-    }, [optionsPosition, onMenuToggle]);
+        onFocus?.();
+    }, [menuDisplayed, onMenuToggle, onFocus]);
 
     const isMainSplitTimelineShown = !WidgetLayoutStore.instance.hasMaximisedWidget(
         MatrixClientPeg.get().getRoom(mxEvent.getRoomId()),
     );
     return <React.Fragment>
         <ContextMenuTooltipButton
+            {...props}
             className="mx_MessageActionBar_maskButton mx_MessageActionBar_optionsButton"
-            onClick={toggleOptionsMenu}
+            onClick={openMenu}
             title={_t("Thread options")}
-            isExpanded={!!optionsPosition}
+            isExpanded={menuDisplayed}
+            inputRef={button}
         />
-        { !!optionsPosition && (<IconizedContextMenu
+        { menuDisplayed && (<IconizedContextMenu
             onFinished={closeThreadOptions}
             className="mx_RoomTile_contextMenu"
             compact
             rightAligned
-            {...contextMenuBelow(optionsPosition)}
+            {...contextMenuBelow(button.current.getBoundingClientRect())}
         >
             <IconizedContextMenuOptionList>
                 { isMainSplitTimelineShown &&
diff --git a/src/components/views/elements/TextWithTooltip.tsx b/src/components/views/elements/TextWithTooltip.tsx
@@ -24,7 +24,7 @@ interface IProps {
     class?: string;
     tooltipClass?: string;
     tooltip: React.ReactNode;
-    tooltipProps?: {};
+    tooltipProps?: Omit<React.ComponentProps<typeof TooltipTarget>, "label" | "tooltipClassName" | "className">;
     onClick?: (ev?: React.MouseEvent) => void;
 }
 
diff --git a/src/components/views/messages/MessageActionBar.tsx b/src/components/views/messages/MessageActionBar.tsx
@@ -91,7 +91,13 @@ const OptionsButton: React.FC<IOptionsButtonProps> = ({
         <ContextMenuTooltipButton
             className="mx_MessageActionBar_maskButton mx_MessageActionBar_optionsButton"
             title={_t("Options")}
-            onClick={openMenu}
+            onClick={() => {
+                openMenu();
+                // when the context menu is opened directly, e.g. via mouse click, the onFocus handler which tracks
+                // the element that is currently focused is skipped. So we want to call onFocus manually to keep the
+                // position in the page even when someone is clicking around.
+                onFocus();
+            }}
             isExpanded={menuDisplayed}
             inputRef={ref}
             onFocus={onFocus}
@@ -127,7 +133,13 @@ const ReactButton: React.FC<IReactButtonProps> = ({ mxEvent, reactions, onFocusC
         <ContextMenuTooltipButton
             className="mx_MessageActionBar_maskButton mx_MessageActionBar_reactButton"
             title={_t("React")}
-            onClick={openMenu}
+            onClick={() => {
+                openMenu();
+                // when the context menu is opened directly, e.g. via mouse click, the onFocus handler which tracks
+                // the element that is currently focused is skipped. So we want to call onFocus manually to keep the
+                // position in the page even when someone is clicking around.
+                onFocus();
+            }}
             isExpanded={menuDisplayed}
             inputRef={ref}
             onFocus={onFocus}
@@ -196,10 +208,7 @@ export default class MessageActionBar extends React.PureComponent<IMessageAction
     };
 
     private onFocusChange = (focused: boolean): void => {
-        if (!this.props.onFocusChange) {
-            return;
-        }
-        this.props.onFocusChange(focused);
+        this.props.onFocusChange?.(focused);
     };
 
     private onReplyClick = (ev: React.MouseEvent): void => {
diff --git a/src/components/views/rooms/EventTile.tsx b/src/components/views/rooms/EventTile.tsx
@@ -67,7 +67,7 @@ import { MediaEventHelper } from "../../../utils/MediaEventHelper";
 import Toolbar from '../../../accessibility/Toolbar';
 import { POLL_START_EVENT_TYPE } from '../../../polls/consts';
 import { RovingAccessibleTooltipButton } from '../../../accessibility/roving/RovingAccessibleTooltipButton';
-import ThreadListContextMenu from '../context_menus/ThreadListContextMenu';
+import { RovingThreadListContextMenu } from '../context_menus/ThreadListContextMenu';
 import { ThreadNotificationState } from '../../../stores/notifications/ThreadNotificationState';
 import { RoomNotificationStateStore } from '../../../stores/notifications/RoomNotificationStateStore';
 import { NotificationStateEvents } from '../../../stores/notifications/NotificationState';
@@ -1432,7 +1432,7 @@ export default class EventTile extends React.Component<IProps, IState> {
                                 onClick={() => dispatchShowThreadEvent(this.props.mxEvent)}
                                 key="thread"
                             />
-                            <ThreadListContextMenu
+                            <RovingThreadListContextMenu
                                 mxEvent={this.props.mxEvent}
                                 permalinkCreator={this.props.permalinkCreator}
                                 onMenuToggle={this.onActionBarFocusChange}
diff --git a/src/components/views/rooms/RoomTile.tsx b/src/components/views/rooms/RoomTile.tsx
@@ -566,13 +566,6 @@ export default class RoomTile extends React.PureComponent<IProps, IState> {
         if (typeof name !== 'string') name = '';
         name = name.replace(":", ":\u200b"); // add a zero-width space to allow linewrapping after the colon
 
-        const roomAvatar = <DecoratedRoomAvatar
-            room={this.props.room}
-            avatarSize={32}
-            displayBadge={this.props.isMinimized}
-            oobData={({ avatarUrl: roomProfile.avatarMxc })}
-        />;
-
         let badge: React.ReactNode;
         if (!this.props.isMinimized && this.notificationState) {
             // aria-hidden because we summarise the unread count/highlight status in a manual aria-label below
@@ -663,7 +656,13 @@ export default class RoomTile extends React.PureComponent<IProps, IState> {
                             aria-selected={this.state.selected}
                             aria-describedby={ariaDescribedBy}
                         >
-                            { roomAvatar }
+                            <DecoratedRoomAvatar
+                                room={this.props.room}
+                                avatarSize={32}
+                                displayBadge={this.props.isMinimized}
+                                oobData={({ avatarUrl: roomProfile.avatarMxc })}
+                                tooltipProps={{ tabIndex: isActive ? 0 : -1 }}
+                            />
                             { nameContainer }
                             { badge }
                             { this.renderGeneralMenu() }

Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ const Toolbar: React.FC<IProps> = ({ children, ...props }) => {`
`52`	`52`	`}`
`53`	`53`	`};`
`54`	`54`
`55`		`- return <RovingTabIndexProvider handleHomeEnd={true} onKeyDown={onKeyDown}>`
	`55`	`+ return <RovingTabIndexProvider handleHomeEnd handleLeftRight onKeyDown={onKeyDown}>`
`56`	`56`	`{ ({ onKeyDownHandler }) => <div {...props} onKeyDown={onKeyDownHandler} role="toolbar">`
`57`	`57`	`{ children }`
`58`	`58`	`</div> }`
Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ interface IProps {`
`24`	`24`	`class?: string;`
`25`	`25`	`tooltipClass?: string;`
`26`	`26`	`tooltip: React.ReactNode;`
`27`		`- tooltipProps?: {};`
	`27`	`+ tooltipProps?: Omit<React.ComponentProps<typeof TooltipTarget>, "label" \| "tooltipClassName" \| "className">;`
`28`	`28`	`onClick?: (ev?: React.MouseEvent) => void;`
`29`	`29`	`}`
`30`	`30`