useEffect is running a function an additional time instead of once per if statement

Question

I have a modal. This modal opens after a user uploads a CSV. They can then make changes to the entries or delete them. The user can Confirm & Continue (go to next page, modal closes) or Upload New File (modal closes, remain on same page). However, if they make any changes, a new button will appear after these two: Download Updated CSV

Currently on my modal I am implementing Focus Trapping for accessibility purposes. My focus trap will prevent user from tabbing outside of the modal - if they hit tab on the last element, it focuses the first, and shift tab on the first will focus the last. Here is where my problem was first noticed.

In my functional component for the modal, I have a useEffect. This useEffect will detect when changes are made and cause a re-render (using a custom Hook) and then will grab a ref of all interactable elements. This is then passed to a function called focusTrap.

Custom hook useIsMount (used for checking if DOM is on first or second render, in case elements are not ready to be referenced):

export const useIsMount = () => {
    const isMountRef = useRef(true);
    useEffect(() => {
        isMountRef.current = false;
    }, []);
    return isMountRef.current;
};

useEffect that (might be) causing the issue:

const isMount = useIsMount();

useEffect(() => {
    if (!isMount) {
        const buttonsElement = buttonsRef?.current;
        buttonsElement && focusTrap(buttonsElement, onClose);
    }
});

!isMount is used here to make sure all content has been rendered before grabbing elements. I'm not using a dependency array so that it will always run the useEffect on re-render.

My focusTrap function that hasn't failed me until now:

export const focusTrap = (modalElement, onClose) => {
    const focusableElements = modalElement.querySelectorAll(
        'button, [href], input, checkbox, select, textarea, [tabindex]:not([tabindex="-1"])'
    );
    const firstElement = focusableElements[0];
    const lastElement = focusableElements[focusableElements.length - 1];

    const handleTabKeyPress = (event) => {
        if (event.key === "Tab") {
            if (event.shiftKey && document.activeElement === firstElement) {
                event.preventDefault();
                lastElement.focus();
            } else if (!event.shiftKey && document.activeElement === lastElement) {
                event.preventDefault();
                firstElement.focus();
            }
        }
    };

    const handleEscapeKeyPress = (event) => {
        if (event.key === "Escape") {
            onClose();
        }
    };

    modalElement.addEventListener("keydown", handleTabKeyPress, true);
    modalElement.addEventListener("keydown", handleEscapeKeyPress);

    return () => {
        modalElement.removeEventListener("keydown", handleTabKeyPress, true);
        modalElement.removeEventListener("keydown", handleEscapeKeyPress);
    };
};

When the modal renders, all buttons are found just fine and no issues. However when the user makes a change, the useEffect runs again, and the focusTrap function is called again. Each additional change will cause focusTrap to run an additional instance, but with a different list of focusableElements each instance. When the first edit is made, the Download Updated CSV button is rendered conditionally. It shows in the console.logs for focusableElements but only in the second run of focusTrap. This causes the first run to take priority, ignoring the new button and focusing to the first again.

I've tried:

Adding empty dependency array (doesn't catch content change re-renders, doesn't detect Download button)
Adding dependency array for isMount and totalEdits/totalDeletes (keep track of edit/delete amounts to render Download button) but I still call focusTrap as I need an if statement to determine if there's changes
```
if (totalEdits > 0 || totalDeletes > 0) {
    buttonsElement && focusTrap(buttonsElement, onClose);
}
```
A bunch of other things that I honestly can't even recall at this point. Either the focus trap would fail and escape the modal (but detect the new button!) or the button still isn't focusable

It may be possible that a new addEventListener is being added each time focusTrap runs, because whenever I hit Tab the handleTabKeyPress will run one additional time for each change made (i.e. 3 changes/deletions made to CSV table - handleTabKeyPress runs 4 times at once and I log something to the console)

Would it be somehow possible to kill a function that is running before it runs again?

Lastly if I conditionally render this button somewhere that it isn't the last element, it's focused fine (like before the Confirm button). However the focusTrap still runs multiple times and that's not good.

Drew Reese · Accepted Answer · 2025-10-29 18:59:06Z

The potential issues I see in your code are:

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.
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);

Excellent! This is just what I needed. My issues were completely resolved with the first example, but I went with the final since it looks a bit more clean. Only needed to change two things: moving const modalElement before handleKeyPress as it throws an undefined error otherwise and the event.preventDefault() outside of the if/else statements was causing the Tab key to be ignored, so I just left it inside the if/else statements. Thank you!

Collectives™ on Stack Overflow

useEffect is running a function an additional time instead of once per if statement

1 Answer 1

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related