OpenCloud
/
riot-web
mirror of https://github.com/vector-im/riot-web
1
0
Fork 0
Code Issues Releases Wiki Activity

Fix React 18 strict mode breaking spotlight search 

This code originally held an array of refs. But these refs were unset
just before sorting leading to errors.

For the fix, I've used a callback ref to add/remove the DOM elements
to/from the array in state. This way there's nothing that could possibly
mutate just before sort.
pull/28452/head
R Midhun Suresh 2024-11-13 00:28:32 +05:30
parent 2f8e98242c
commit 0faf298e05
No known key found for this signature in database
3 changed files with 151 additions and 133 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

160
src/accessibility/RovingTabIndex.tsx
View File

@ -10,7 +10,6 @@ import React, {
createContext, createContext,
useCallback, useCallback,
useContext, useContext,
useEffect,
useMemo, useMemo,
useRef, useRef,
useReducer, useReducer,
@ -22,7 +21,7 @@ import React, {
import { getKeyBindingsManager } from "../KeyBindingsManager"; import { getKeyBindingsManager } from "../KeyBindingsManager";
import { KeyBindingAction } from "./KeyboardShortcuts"; import { KeyBindingAction } from "./KeyboardShortcuts";
import { FocusHandler, Ref } from "./roving/types"; import { FocusHandler } from "./roving/types";
/** /**
* Module to simplify implementing the Roving TabIndex accessibility technique * Module to simplify implementing the Roving TabIndex accessibility technique
@ -49,8 +48,8 @@ export function checkInputableElement(el: HTMLElement): boolean {
} }
export interface IState { export interface IState {
activeRef?: Ref; activeNode?: HTMLElement;
refs: Ref[]; nodes: HTMLElement[];
} }
export interface IContext { export interface IContext {
@ -60,7 +59,7 @@ export interface IContext {
export const RovingTabIndexContext = createContext<IContext>({ export const RovingTabIndexContext = createContext<IContext>({
state: { state: {
refs: [], // list of refs in DOM order nodes: [], // list of nodes in DOM order
}, },
dispatch: () => {}, dispatch: () => {},
}); });
@ -76,7 +75,7 @@ export enum Type {
export interface IAction { export interface IAction {
type: Exclude<Type, Type.Update>; type: Exclude<Type, Type.Update>;
payload: { payload: {
ref: Ref; node: HTMLElement;
}; };
} }
@ -87,12 +86,12 @@ interface UpdateAction {
type Action = IAction | UpdateAction; type Action = IAction | UpdateAction;
const refSorter = (a: Ref, b: Ref): number => { const nodeSorter = (a: HTMLElement, b: HTMLElement): number => {
if (a === b) { if (a === b) {
return 0; return 0;
} }
const position = a.current!.compareDocumentPosition(b.current!); const position = a.compareDocumentPosition(b);
if (position & Node.DOCUMENT_POSITION_FOLLOWING || position & Node.DOCUMENT_POSITION_CONTAINED_BY) { if (position & Node.DOCUMENT_POSITION_FOLLOWING || position & Node.DOCUMENT_POSITION_CONTAINED_BY) {
return -1; return -1;
@ -106,54 +105,56 @@ const refSorter = (a: Ref, b: Ref): number => {
export const reducer: Reducer<IState, Action> = (state: IState, action: Action) => { export const reducer: Reducer<IState, Action> = (state: IState, action: Action) => {
switch (action.type) { switch (action.type) {
case Type.Register: { case Type.Register: {
if (!state.activeRef) { if (!state.activeNode) {
// Our list of refs was empty, set activeRef to this first item // Our list of nodes was empty, set activeNode to this first item
state.activeRef = action.payload.ref; state.activeNode = action.payload.node;
} }
if (state.nodes.includes(action.payload.node)) return state;
// Sadly due to the potential of DOM elements swapping order we can't do anything fancy like a binary insert // Sadly due to the potential of DOM elements swapping order we can't do anything fancy like a binary insert
state.refs.push(action.payload.ref); state.nodes.push(action.payload.node);
state.refs.sort(refSorter); state.nodes.sort(nodeSorter);
return { ...state }; return { ...state };
} }
case Type.Unregister: { case Type.Unregister: {
const oldIndex = state.refs.findIndex((r) => r === action.payload.ref); const oldIndex = state.nodes.findIndex((r) => r === action.payload.node);
if (oldIndex === -1) { if (oldIndex === -1) {
return state; // already removed, this should not happen return state; // already removed, this should not happen
} }
if (state.refs.splice(oldIndex, 1)[0] === state.activeRef) { if (state.nodes.splice(oldIndex, 1)[0] === state.activeNode) {
// we just removed the active ref, need to replace it // we just removed the active node, need to replace it
// pick the ref closest to the index the old ref was in // pick the node closest to the index the old node was in
if (oldIndex >= state.refs.length) { if (oldIndex >= state.nodes.length) {
state.activeRef = findSiblingElement(state.refs, state.refs.length - 1, true); state.activeNode = findSiblingElement(state.nodes, state.nodes.length - 1, true);
} else { } else {
state.activeRef = state.activeNode =
findSiblingElement(state.refs, oldIndex) || findSiblingElement(state.refs, oldIndex, true); findSiblingElement(state.nodes, oldIndex) || findSiblingElement(state.nodes, oldIndex, true);
} }
if (document.activeElement === document.body) { if (document.activeElement === document.body) {
// if the focus got reverted to the body then the user was likely focused on the unmounted element // if the focus got reverted to the body then the user was likely focused on the unmounted element
setTimeout(() => state.activeRef?.current?.focus(), 0); setTimeout(() => state.activeNode?.focus(), 0);
} }
} }
// update the refs list // update the nodes list
return { ...state }; return { ...state };
} }
case Type.SetFocus: { case Type.SetFocus: {
// if the ref doesn't change just return the same object reference to skip a re-render // if the node doesn't change just return the same object reference to skip a re-render
if (state.activeRef === action.payload.ref) return state; if (state.activeNode === action.payload.node) return state;
// update active ref // update active node
state.activeRef = action.payload.ref; state.activeNode = action.payload.node;
return { ...state }; return { ...state };
} }
case Type.Update: { case Type.Update: {
state.refs.sort(refSorter); state.nodes.sort(nodeSorter);
return { ...state }; return { ...state };
} }
@ -174,28 +175,28 @@ interface IProps {
} }
export const findSiblingElement = ( export const findSiblingElement = (
refs: RefObject<HTMLElement>[], nodes: HTMLElement[],
startIndex: number, startIndex: number,
backwards = false, backwards = false,
loop = false, loop = false,
): RefObject<HTMLElement> | undefined => { ): HTMLElement | undefined => {
if (backwards) { if (backwards) {
for (let i = startIndex; i < refs.length && i >= 0; i--) { for (let i = startIndex; i < nodes.length && i >= 0; i--) {
if (refs[i].current?.offsetParent !== null) { if (nodes[i]?.offsetParent !== null) {
return refs[i]; return nodes[i];
} }
} }
if (loop) { if (loop) {
return findSiblingElement(refs.slice(startIndex + 1), refs.length - 1, true, false); return findSiblingElement(nodes.slice(startIndex + 1), nodes.length - 1, true, false);
} }
} else { } else {
for (let i = startIndex; i < refs.length && i >= 0; i++) { for (let i = startIndex; i < nodes.length && i >= 0; i++) {
if (refs[i].current?.offsetParent !== null) { if (nodes[i]?.offsetParent !== null) {
return refs[i]; return nodes[i];
} }
} }
if (loop) { if (loop) {
return findSiblingElement(refs.slice(0, startIndex), 0, false, false); return findSiblingElement(nodes.slice(0, startIndex), 0, false, false);
} }
} }
}; };
@ -211,7 +212,7 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
onKeyDown, onKeyDown,
}) => { }) => {
const [state, dispatch] = useReducer<Reducer<IState, Action>>(reducer, { const [state, dispatch] = useReducer<Reducer<IState, Action>>(reducer, {
refs: [], nodes: [],
}); });
const context = useMemo<IContext>(() => ({ state, dispatch }), [state]); const context = useMemo<IContext>(() => ({ state, dispatch }), [state]);
@ -227,17 +228,17 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
let handled = false; let handled = false;
const action = getKeyBindingsManager().getAccessibilityAction(ev); const action = getKeyBindingsManager().getAccessibilityAction(ev);
let focusRef: RefObject<HTMLElement> | undefined; let focusNode: HTMLElement | undefined;
// Don't interfere with input default keydown behaviour // Don't interfere with input default keydown behaviour
// but allow people to move focus from it with Tab. // but allow people to move focus from it with Tab.
if (!handleInputFields && checkInputableElement(ev.target as HTMLElement)) { if (!handleInputFields && checkInputableElement(ev.target as HTMLElement)) {
switch (action) { switch (action) {
case KeyBindingAction.Tab: case KeyBindingAction.Tab:
handled = true; handled = true;
if (context.state.refs.length > 0) { if (context.state.nodes.length > 0) {
const idx = context.state.refs.indexOf(context.state.activeRef!); const idx = context.state.nodes.indexOf(context.state.activeNode!);
focusRef = findSiblingElement( focusNode = findSiblingElement(
context.state.refs, context.state.nodes,
idx + (ev.shiftKey ? -1 : 1), idx + (ev.shiftKey ? -1 : 1),
ev.shiftKey, ev.shiftKey,
); );
@ -251,7 +252,7 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
if (handleHomeEnd) { if (handleHomeEnd) {
handled = true; handled = true;
// move focus to first (visible) item // move focus to first (visible) item
focusRef = findSiblingElement(context.state.refs, 0); focusNode = findSiblingElement(context.state.nodes, 0);
} }
break; break;
@ -259,7 +260,7 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
if (handleHomeEnd) { if (handleHomeEnd) {
handled = true; handled = true;
// move focus to last (visible) item // move focus to last (visible) item
focusRef = findSiblingElement(context.state.refs, context.state.refs.length - 1, true); focusNode = findSiblingElement(context.state.nodes, context.state.nodes.length - 1, true);
} }
break; break;
@ -270,9 +271,9 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
(action === KeyBindingAction.ArrowRight && handleLeftRight) (action === KeyBindingAction.ArrowRight && handleLeftRight)
) { ) {
handled = true; handled = true;
if (context.state.refs.length > 0) { if (context.state.nodes.length > 0) {
const idx = context.state.refs.indexOf(context.state.activeRef!); const idx = context.state.nodes.indexOf(context.state.activeNode!);
focusRef = findSiblingElement(context.state.refs, idx + 1, false, handleLoop); focusNode = findSiblingElement(context.state.nodes, idx + 1, false, handleLoop);
} }
} }
break; break;
@ -284,9 +285,9 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
(action === KeyBindingAction.ArrowLeft && handleLeftRight) (action === KeyBindingAction.ArrowLeft && handleLeftRight)
) { ) {
handled = true; handled = true;
if (context.state.refs.length > 0) { if (context.state.nodes.length > 0) {
const idx = context.state.refs.indexOf(context.state.activeRef!); const idx = context.state.nodes.indexOf(context.state.activeNode!);
focusRef = findSiblingElement(context.state.refs, idx - 1, true, handleLoop); focusNode = findSiblingElement(context.state.nodes, idx - 1, true, handleLoop);
} }
} }
break; break;
@ -298,17 +299,17 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
ev.stopPropagation(); ev.stopPropagation();
} }
if (focusRef) { if (focusNode) {
focusRef.current?.focus(); focusNode?.focus();
// programmatic focus doesn't fire the onFocus handler, so we must do the do ourselves // programmatic focus doesn't fire the onFocus handler, so we must do the do ourselves
dispatch({ dispatch({
type: Type.SetFocus, type: Type.SetFocus,
payload: { payload: {
ref: focusRef, node: focusNode,
}, },
}); });
if (scrollIntoView) { if (scrollIntoView) {
focusRef.current?.scrollIntoView(scrollIntoView); focusNode?.scrollIntoView(scrollIntoView);
} }
} }
}, },
@ -337,46 +338,57 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
); );
}; };
// Hook to register a roving tab index /**
// inputRef parameter specifies the ref to use * Hook to register a roving tab index.
// onFocus should be called when the index gained focus in any manner *
// isActive should be used to set tabIndex in a manner such as `tabIndex={isActive ? 0 : -1}` * inputRef is an optional argument; when passed this ref points to the DOM element
// ref should be passed to a DOM node which will be used for DOM compareDocumentPosition * to which the callback ref is attached.
*
* Returns:
* onFocus should be called when the index gained focus in any manner.
* isActive should be used to set tabIndex in a manner such as `tabIndex={isActive ? 0 : -1}`.
* ref is a callback ref that should be passed to a DOM node which will be used for DOM compareDocumentPosition.
* nodeRef is a ref that points to the DOM element to which the ref mentioned above is attached.
*
* nodeRef = inputRef when inputRef argument is provided.
*/
export const useRovingTabIndex = <T extends HTMLElement>( export const useRovingTabIndex = <T extends HTMLElement>(
inputRef?: RefObject<T>, inputRef?: RefObject<T>,
): [FocusHandler, boolean, RefObject<T>] => { ): [FocusHandler, boolean, (node: T | null) => void, RefObject<T | null>] => {
const context = useContext(RovingTabIndexContext); const context = useContext(RovingTabIndexContext);
let ref = useRef<T>(null);
let nodeRef = useRef<T | null>(null);
if (inputRef) { if (inputRef) {
// if we are given a ref, use it instead of ours // if we are given a ref, use it instead of ours
ref = inputRef; nodeRef = inputRef;
} }
// setup (after refs) const ref = useCallback((node: T | null) => {
useEffect(() => { if (node) {
nodeRef.current = node;
context.dispatch({ context.dispatch({
type: Type.Register, type: Type.Register,
payload: { ref }, payload: { node },
}); });
// teardown } else {
return () => {
context.dispatch({ context.dispatch({
type: Type.Unregister, type: Type.Unregister,
payload: { ref }, payload: { node: nodeRef.current! },
}); });
}; nodeRef.current = null;
}
}, []); // eslint-disable-line react-hooks/exhaustive-deps }, []); // eslint-disable-line react-hooks/exhaustive-deps
const onFocus = useCallback(() => { const onFocus = useCallback(() => {
context.dispatch({ context.dispatch({
type: Type.SetFocus, type: Type.SetFocus,
payload: { ref }, payload: { node: nodeRef.current } as { node: T },
}); });
}, []); // eslint-disable-line react-hooks/exhaustive-deps }, []); // eslint-disable-line react-hooks/exhaustive-deps
const isActive = context.state.activeRef === ref; const isActive = context.state.activeNode === nodeRef.current;
return [onFocus, isActive, ref]; return [onFocus, isActive, ref, nodeRef];
}; };
// re-export the semantic helper components for simplicity // re-export the semantic helper components for simplicity

6
src/accessibility/roving/RovingTabIndexWrapper.tsx
View File

@ -13,7 +13,11 @@ import { FocusHandler, Ref } from "./types";
interface IProps { interface IProps {
inputRef?: Ref; inputRef?: Ref;
children(renderProps: { onFocus: FocusHandler; isActive: boolean; ref: Ref }): ReactElement<any, any>; children(renderProps: {
onFocus: FocusHandler;
isActive: boolean;
ref: (node: HTMLElement | null) => void;
}): ReactElement<any, any>;
} }
// Wrapper to allow use of useRovingTabIndex outside of React Functional Components. // Wrapper to allow use of useRovingTabIndex outside of React Functional Components.

112
test/unit-tests/accessibility/RovingTabIndex-test.tsx
View File

@ -28,6 +28,12 @@ const checkTabIndexes = (buttons: NodeListOf<HTMLElement>, expectations: number[
expect([...buttons].map((b) => b.tabIndex)).toStrictEqual(expectations); expect([...buttons].map((b) => b.tabIndex)).toStrictEqual(expectations);
}; };
const createButtonElement = (text: string): HTMLButtonElement => {
const button = document.createElement("button");
button.textContent = text;
return button;
};
// give the buttons keys for the fibre reconciler to not treat them all as the same // give the buttons keys for the fibre reconciler to not treat them all as the same
const button1 = <Button key={1}>a</Button>; const button1 = <Button key={1}>a</Button>;
const button2 = <Button key={2}>b</Button>; const button2 = <Button key={2}>b</Button>;
@ -123,11 +129,7 @@ describe("RovingTabIndex", () => {
{button2} {button2}
<RovingTabIndexWrapper> <RovingTabIndexWrapper>
{({ onFocus, isActive, ref }) => ( {({ onFocus, isActive, ref }) => (
<button <button onFocus={onFocus} tabIndex={isActive ? 0 : -1} ref={ref}>
onFocus={onFocus}
tabIndex={isActive ? 0 : -1}
ref={ref as React.RefObject<HTMLButtonElement>}
>
. .
</button> </button>
)} )}
@ -147,75 +149,75 @@ describe("RovingTabIndex", () => {
describe("reducer functions as expected", () => { describe("reducer functions as expected", () => {
it("SetFocus works as expected", () => { it("SetFocus works as expected", () => {
const ref1 = React.createRef<HTMLElement>(); const node1 = createButtonElement("Button 1");
const ref2 = React.createRef<HTMLElement>(); const node2 = createButtonElement("Button 2");
expect( expect(
reducer( reducer(
{ {
activeRef: ref1, activeNode: node1,
refs: [ref1, ref2], nodes: [node1, node2],
}, },
{ {
type: Type.SetFocus, type: Type.SetFocus,
payload: { payload: {
ref: ref2, node: node2,
}, },
}, },
), ),
).toStrictEqual({ ).toStrictEqual({
activeRef: ref2, activeNode: node2,
refs: [ref1, ref2], nodes: [node1, node2],
}); });
}); });
it("Unregister works as expected", () => { it("Unregister works as expected", () => {
const ref1 = React.createRef<HTMLElement>(); const button1 = createButtonElement("Button 1");
const ref2 = React.createRef<HTMLElement>(); const button2 = createButtonElement("Button 2");
const ref3 = React.createRef<HTMLElement>(); const button3 = createButtonElement("Button 3");
const ref4 = React.createRef<HTMLElement>(); const button4 = createButtonElement("Button 4");
let state: IState = { let state: IState = {
refs: [ref1, ref2, ref3, ref4], nodes: [button1, button2, button3, button4],
}; };
state = reducer(state, { state = reducer(state, {
type: Type.Unregister, type: Type.Unregister,
payload: { payload: {
ref: ref2, node: button2,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
refs: [ref1, ref3, ref4], nodes: [button1, button3, button4],
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Unregister, type: Type.Unregister,
payload: { payload: {
ref: ref3, node: button3,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
refs: [ref1, ref4], nodes: [button1, button4],
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Unregister, type: Type.Unregister,
payload: { payload: {
ref: ref4, node: button4,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
refs: [ref1], nodes: [button1],
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Unregister, type: Type.Unregister,
payload: { payload: {
ref: ref1, node: button1,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
refs: [], nodes: [],
}); });
}); });
@ -235,122 +237,122 @@ describe("RovingTabIndex", () => {
); );
let state: IState = { let state: IState = {
refs: [], nodes: [],
}; };
state = reducer(state, { state = reducer(state, {
type: Type.Register, type: Type.Register,
payload: { payload: {
ref: ref1, node: ref1.current!,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
activeRef: ref1, activeNode: ref1.current,
refs: [ref1], nodes: [ref1.current],
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Register, type: Type.Register,
payload: { payload: {
ref: ref2, node: ref2.current!,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
activeRef: ref1, activeNode: ref1.current,
refs: [ref1, ref2], nodes: [ref1.current, ref2.current],
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Register, type: Type.Register,
payload: { payload: {
ref: ref3, node: ref3.current!,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
activeRef: ref1, activeNode: ref1.current,
refs: [ref1, ref2, ref3], nodes: [ref1.current, ref2.current, ref3.current],
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Register, type: Type.Register,
payload: { payload: {
ref: ref4, node: ref4.current!,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
activeRef: ref1, activeNode: ref1.current,
refs: [ref1, ref2, ref3, ref4], nodes: [ref1.current, ref2.current, ref3.current, ref4.current],
}); });
// test that the automatic focus switch works for unmounting // test that the automatic focus switch works for unmounting
state = reducer(state, { state = reducer(state, {
type: Type.SetFocus, type: Type.SetFocus,
payload: { payload: {
ref: ref2, node: ref2.current!,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
activeRef: ref2, activeNode: ref2.current,
refs: [ref1, ref2, ref3, ref4], nodes: [ref1.current, ref2.current, ref3.current, ref4.current],
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Unregister, type: Type.Unregister,
payload: { payload: {
ref: ref2, node: ref2.current!,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
activeRef: ref3, activeNode: ref3.current,
refs: [ref1, ref3, ref4], nodes: [ref1.current, ref3.current, ref4.current],
}); });
// test that the insert into the middle works as expected // test that the insert into the middle works as expected
state = reducer(state, { state = reducer(state, {
type: Type.Register, type: Type.Register,
payload: { payload: {
ref: ref2, node: ref2.current!,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
activeRef: ref3, activeNode: ref3.current,
refs: [ref1, ref2, ref3, ref4], nodes: [ref1.current, ref2.current, ref3.current, ref4.current],
}); });
// test that insertion at the edges works // test that insertion at the edges works
state = reducer(state, { state = reducer(state, {
type: Type.Unregister, type: Type.Unregister,
payload: { payload: {
ref: ref1, node: ref1.current!,
}, },
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Unregister, type: Type.Unregister,
payload: { payload: {
ref: ref4, node: ref4.current!,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
activeRef: ref3, activeNode: ref3.current,
refs: [ref2, ref3], nodes: [ref2.current, ref3.current],
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Register, type: Type.Register,
payload: { payload: {
ref: ref1, node: ref1.current!,
}, },
}); });
state = reducer(state, { state = reducer(state, {
type: Type.Register, type: Type.Register,
payload: { payload: {
ref: ref4, node: ref4.current!,
}, },
}); });
expect(state).toStrictEqual({ expect(state).toStrictEqual({
activeRef: ref3, activeNode: ref3.current,
refs: [ref1, ref2, ref3, ref4], nodes: [ref1.current, ref2.current, ref3.current, ref4.current],
}); });
}); });
}); });