From a667677c57f75946d414d0df1221770e9d747fc5 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 14 Dec 2021 14:27:35 +0000 Subject: [PATCH] Fix accessibility regressions (#7336) * Fix room list roving treeview New TooltipTarget & TextWithTooltip were not roving-accessible * Fix programmatic focus management in roving tab index not triggering onFocus handler * Fix toolbar no longer handling left & 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 --- src/accessibility/RovingTabIndex.tsx | 52 ++++++++++------ src/accessibility/Toolbar.tsx | 2 +- .../views/avatars/DecoratedRoomAvatar.tsx | 3 + .../context_menus/ThreadListContextMenu.tsx | 62 ++++++++++++------- .../views/elements/TextWithTooltip.tsx | 2 +- .../views/messages/MessageActionBar.tsx | 21 +++++-- src/components/views/rooms/EventTile.tsx | 4 +- src/components/views/rooms/RoomTile.tsx | 15 +++-- 8 files changed, 103 insertions(+), 58 deletions(-) diff --git a/src/accessibility/RovingTabIndex.tsx b/src/accessibility/RovingTabIndex.tsx index cdd937bba3..65494a210d 100644 --- 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 = ({ } let handled = false; + let focusRef: RefObject; // 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,7 +205,7 @@ export const RovingTabIndexProvider: React.FC = ({ if (handleHomeEnd) { handled = true; // move focus to first (visible) item - findSiblingElement(context.state.refs, 0)?.current?.focus(); + focusRef = findSiblingElement(context.state.refs, 0); } break; @@ -210,28 +213,30 @@ export const RovingTabIndexProvider: React.FC = ({ if (handleHomeEnd) { handled = true; // move focus to last (visible) item - findSiblingElement(context.state.refs, context.state.refs.length - 1, true)?.current?.focus(); - } - break; - - case Key.ARROW_UP: - case Key.ARROW_RIGHT: - if ((ev.key === Key.ARROW_UP && 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, context.state.refs.length - 1, true); } break; case Key.ARROW_DOWN: - case Key.ARROW_LEFT: - if ((ev.key === Key.ARROW_DOWN && handleUpDown) || (ev.key === Key.ARROW_LEFT && handleLeftRight)) { + case Key.ARROW_RIGHT: + 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, true)?.current?.focus(); + focusRef = findSiblingElement(context.state.refs, idx + 1); + } + } + break; + + case Key.ARROW_UP: + case Key.ARROW_LEFT: + 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); + focusRef = findSiblingElement(context.state.refs, idx - 1, true); } } break; @@ -242,7 +247,18 @@ export const RovingTabIndexProvider: React.FC = ({ 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 { 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 index 6e99c7f1fa..c0f2b56748 100644 --- a/src/accessibility/Toolbar.tsx +++ b/src/accessibility/Toolbar.tsx @@ -52,7 +52,7 @@ const Toolbar: React.FC = ({ children, ...props }) => { } }; - return + return { ({ onKeyDownHandler }) =>
{ children }
} diff --git a/src/components/views/avatars/DecoratedRoomAvatar.tsx b/src/components/views/avatars/DecoratedRoomAvatar.tsx index 99f2b70efc..6ba507c0cc 100644 --- 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, "label" | "tooltipClassName" | "className">; } interface IState { @@ -189,6 +191,7 @@ export default class DecoratedRoomAvatar extends React.PureComponent; } diff --git a/src/components/views/context_menus/ThreadListContextMenu.tsx b/src/components/views/context_menus/ThreadListContextMenu.tsx index f9aa7a4b9f..012b2dbae4 100644 --- a/src/components/views/context_menus/ThreadListContextMenu.tsx +++ b/src/components/views/context_menus/ThreadListContextMenu.tsx @@ -14,7 +14,7 @@ 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"; @@ -22,11 +22,12 @@ 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; @@ -34,6 +35,13 @@ interface IProps { onMenuToggle?: (open: boolean) => void; } +interface IExtendedProps extends IProps { + // Props for making the button into a roving one + tabIndex?: number; + inputRef?: RefObject; + 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 = ({ mxEvent, permalinkCreator, onMenuToggle }) => { - const [optionsPosition, setOptionsPosition] = useState(null); - const closeThreadOptions = useCallback(() => { - setOptionsPosition(null); - }, []); +export const RovingThreadListContextMenu: React.FC = (props) => { + const [onFocus, isActive, ref] = useRovingTabIndex(); + + return ; +}; + +const ThreadListContextMenu: React.FC = ({ + 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 = ({ 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 - { !!optionsPosition && ( { isMainSplitTimelineShown && diff --git a/src/components/views/elements/TextWithTooltip.tsx b/src/components/views/elements/TextWithTooltip.tsx index d5a37e16e7..2b5926f3d7 100644 --- 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, "label" | "tooltipClassName" | "className">; onClick?: (ev?: React.MouseEvent) => void; } diff --git a/src/components/views/messages/MessageActionBar.tsx b/src/components/views/messages/MessageActionBar.tsx index abaf78797e..87433fb4de 100644 --- a/src/components/views/messages/MessageActionBar.tsx +++ b/src/components/views/messages/MessageActionBar.tsx @@ -91,7 +91,13 @@ const OptionsButton: React.FC = ({ { + 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 = ({ mxEvent, reactions, onFocusC { + 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 { - 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 index 0d0df6d519..80d5e3bbba 100644 --- 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 { onClick={() => dispatchShowThreadEvent(this.props.mxEvent)} key="thread" /> - { if (typeof name !== 'string') name = ''; name = name.replace(":", ":\u200b"); // add a zero-width space to allow linewrapping after the colon - const roomAvatar = ; - 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 { aria-selected={this.state.selected} aria-describedby={ariaDescribedBy} > - { roomAvatar } + { nameContainer } { badge } { this.renderGeneralMenu() }