Question Details

No question body available.

Tags

javascript reactjs react-hooks focus

Answers (1)

Accepted Answer Available
Accepted Answer
October 29, 2025 Score: 4 Rep: 208,026 Quality: Expert Completeness: 80%

The potential issues I see in your code are:

  1. focusTrap grabs all the focusable elements, specifically the first and last elements, outside any event handlers, so they will easily likely be stale by the time any keydown events are handled.
  2. Event listeners are not ever cleaned up, so each time focusTrap runs, two more keydown event handlers are added, and never removed.

I suggest simplifying the logic a bit by moving querying the DOM for interactive elements inside an event handler, using a single event handler for checking both the tab and escape keys, and utilizing the returned event handler cleanup function.

Example:

const query = "button, [href], input, checkbox, select, textarea, [tabindex]:not([tabindex="-1"])";

export const focusTrap = (modalElement, onClose) => { const handleKeyPress = (event) => { switch(event.key) { case "Tab": event.preventDefault();

const focusableElements = modalElement.querySelectorAll(query); const firstElement = focusableElements[0]; const lastElement = focusableElements[focusableElements.length - 1];

if (event.shiftKey && document.activeElement === firstElement) { lastElement.focus(); } else if (!event.shiftKey && document.activeElement === lastElement) { firstElement.focus(); } break;

case "Escape": onClose(); break;

default: // ignore/do nothing } };

modalElement.addEventListener("keydown", handleKeyPress, true);

return () => { modalElement.removeEventListener("keydown", handleKeyPress, true); }; };
const isMount = useIsMount();

useEffect(() => { const buttonsElement = buttonsRef?.current;

if (!isMount && buttonsElement) { const cleanup = focusTrap(buttonsElement, onClose); return () => { cleanup(); };

// or more succinctly // return focusTrap(buttonsElement, onClose); } });

Since you are working with React refs, I think you could modify the above a bit so the effect can truly only run once when the parent component mounts by moving the conditional checks into the event handler callback and using an empty dependency array for the useEffect hook to set everything up. You'll instead pass the button/modal ref itself to focusTrap so it can be passed to and checked in the event handler. This also should eliminate the need for the isMounted ref (which is now considered a React anti-pattern anyway).

Example:

const query = "button, [href], input, checkbox, select, textarea, [tabindex]:not([tabindex="-1"])";

export const focusTrap = (modalElementRef, onClose) => { const handleKeyPress = (event) => { const modalElement = modalElementRef.current;

// If no ref value yet don't do anything if (!modalElement) { return; }

switch(event.key) { case "Tab": event.preventDefault();

const focusableElements = modalElement.querySelectorAll(query); const firstElement = focusableElements[0]; const lastElement = focusableElements[focusableElements.length - 1];

if (event.shiftKey && document.activeElement === firstElement) { lastElement.focus(); } else if (!event.shiftKey && document.activeElement === lastElement) { firstElement.focus(); } break;

case "Escape": onClose(); break;

default: // ignore/do nothing } };

modalElement.addEventListener("keydown", handleKeyPress, true);

return () => { modalElement.removeEventListener("keydown", handleKeyPress, true); }; };
useEffect(() => {
  const cleanup = focusTrap(buttonsRef, onClose);
  return () => {
    cleanup();
  };

// or more succinctly // return focusTrap(buttonsRef, onClose); }, []);

This could be taken a step further by converting focusTrap into a custom hook. Basically rename focusTrap to useFocusTrap and move the useEffect hook call into it and move all the logic into the effect callback.

Example:

const query = "button, [href], input, checkbox, select, textarea, [tabindex]:not([tabindex="-1"])";

export const useFocusTrap = (modalElementRef, onClose) => { useEffect(() => { const handleKeyPress = (event) => { const modalElement = modalElementRef.current;

// If no ref value yet don't do anything if (!modalElement) { return; }

switch(event.key) { case "Tab": event.preventDefault();

const focusableElements = modalElement.querySelectorAll(query); const firstElement = focusableElements[0]; const lastElement = focusableElements[focusableElements.length - 1];

if (event.shiftKey && document.activeElement === firstElement) { lastElement.focus(); } else if (!event.shiftKey && document.activeElement === lastElement) { firstElement.focus(); } break;

case "Escape": onClose(); break;

default: // ignore/do nothing } };

modalElement.addEventListener("keydown", handleKeyPress, true);

return () => { modalElement.removeEventListener("keydown", handleKeyPress, true); }; }, [onClose]); };
const onCloseStable = useCallback(onClose, []);

useFocusTrap(buttonsRef, onCloseStable);