[Bug]: Invalid hook call on pressing Tab in ComposedModal with focus-wrap-without-sentinels

[fukudasjp]

Package

@carbon/react

Browser

No response

Package version

v1.82.1

React version

No response

Description

After pressing Tab key on Composed modal with focus-wrap-without-sentinels, an invalid hook call error Uncaught Error: Minified React error #321; visit https://react.dev/errors/321 for the full message or use the non-minified dev environment for full errors and additional helpful warnings. logged in console. Keyboard focus moves outside of the modal by pressing Tab or shift-Tab multiple times.

Log in console

      detail: Error: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:
      1. You might have mismatching versions of React and the renderer (such as React DOM)
      2. You might be breaking the Rules of Hooks
      3. You might have more than one copy of React in the same app
      See https://reactjs.org/link/invalid-hook-call for tips about how to debug and fix this problem.
          at Object.throwInvalidHookError (...node_modules/react-dom/cjs/react-dom.development.js:15408:9)
          at Object.useEffect (...node_modules/react/cjs/react.development.js:1634:21)
          at Object.wrapFocusWithoutSentinels (...node_modules/@carbon/react/lib/internal/wrapFocus.js:103:9)
          at handleKeyDown (...node_modules/@carbon/react/lib/components/ComposedModal/ComposedModal.js:164:19)
          at HTMLUnknownElement.callCallback (...node_modules/react-dom/cjs/react-dom.development.js:4164:14)

Reproduction/example

https://react.carbondesignsystem.com/?path=/story/components-composedmodal-feature-flags--enable-experimental-focus-wrap-without-sentinels

Steps to reproduce

1. Open the storybook ComposedModal > Feature Flags > enable-experimental-focus-wrap-without-sentinels https://react.carbondesignsystem.com/?path=/story/components-composedmodal-feature-flags--enable-experimental-focus-wrap-without-sentinels
2. Open browser console
3. Type Tab key, and see the console

Uncaught Error: Minified React error #321; visit https://react.dev/errors/321 for the full message or use the non-minified dev environment for full errors and additional helpful warnings.
    at Object.throwInvalidHookError (3372.85349052.iframe.bundle.js:2:84838)
    at exports.useEffect (index.js:3517:1)
    at wrapFocusWithoutSentinels (3160.4b349720.iframe.bundle.js:1:158454)
    at handleKeyDown (5724.76c208bb.iframe.bundle.js:1:101903)
    at processDispatchQueue (3372.85349052.iframe.bundle.js:2:289344)

Suggested Severity

None

Application/PAL

No response

Code of Conduct

• I agree to follow this project's Code of Conduct
• I checked the current issues for duplicate problems

[wkeese]

I think the regression came from #18913.

The rules of hooks clearly state that:

You can only call Hooks while React is rendering a function component:

✅ Call them at the top level in the body of a function component.
✅ Call them at the top level in the body of a custom Hook.

wrapFocus.ts violates these rules because a plain function calls a hook:

export const wrapFocusWithoutSentinels = ({
  containerNode,
  currentActiveNode,
  event,
}: {
  /** The container node within which to keep focus. */
  containerNode: HTMLElement;
  /** The current active node (i.e., the one with focus). */
  currentActiveNode: HTMLElement;
  /** The event that triggered this function. */
  event: React.KeyboardEvent | KeyboardEvent;
}) => {
  if (!containerNode) return;

  useEffect(() => {
    if (
      ['blur', 'focusout', 'focusin', 'focus'].includes(event.type) &&
      process.env.NODE_ENV !== 'production'
    ) {
      throw new Error(
        `Error: wrapFocusWithoutSentinels(...) called in unsupported ${event.type} event.\n\nCall wrapFocusWithoutSentinels(...) from onKeyDown instead.`
      );
    }
  }, []);

  ...

wrapFocus.js (from before that PR) was also bad, so part of the blame goes to #15868. But at least it only called useEffect() when there was an error:

function wrapFocusWithoutSentinels({
  containerNode,
  currentActiveNode,
  event,
}) {
  if (
    ['blur', 'focusout', 'focusin', 'focus'].includes(event.type) &&
    process.env.NODE_ENV !== 'production'
  ) {
    // TODO: https://github.com/carbon-design-system/carbon/issues/19005
    /*
    // eslint-disable-next-line react-hooks/rules-of-hooks
    */
    useEffect(() => {
      throw new Error(
        `Error: wrapFocusWithoutSentinels(...) called in unsupported ${event.type} event.\n\nCall wrapFocusWithoutSentinels(...) from onKeyDown instead.`
      );
    });
  }

I don't understand why @tay1orjones wrapped the throw in a useEffect() but I suggest we just get rid of that, so we just do

    if (
      ['blur', 'focusout', 'focusin', 'focus'].includes(event.type) &&
      process.env.NODE_ENV !== 'production'
    ) {
      throw new Error(
        `Error: wrapFocusWithoutSentinels(...) called in unsupported ${event.type} event.\n\nCall wrapFocusWithoutSentinels(...) from onKeyDown instead.`
      );
    }

@adamalston and @tay1orjones, thoughts?

[adamalston]

Two straightforward options: convert wrapFocusWithoutSentinels to a hook that returns a callback, or remove the useEffect entirely. I favor simplification here. I'm running some changes through CI to see what breaks when I change things.

[wkeese]

Thanks @adamalston. I checked #19407, it looks good to me. I always favor simplification.

[adamalston]

Given that the hook was already being called conditionally before #18913, should this issue really be considered a regression? Or is it more accurate to say the change increased the frequency of an existing issue? It’s only after those changes (i.e., now) that we're planning to address the issue.

[wkeese]

Well, here's the code in Modal.tsx (i.e. the code that runs when the feature flag is turned on by @carbon/ibm-products' TearsheetShell etc.):

function handleKeyDown(evt: React.KeyboardEvent<HTMLDivElement>) {
  ...
      wrapFocusWithoutSentinels({
        containerNode: innerModal.current,
        currentActiveNode: target,
        event: evt,
      });
...
}

It used to work 100% of the time, but now it fails 100% of the time.

So, it seems like a regression to me.

PS: In other words, the regression is with Modal, or more accurately, with Tearsheet, but the way to fix it is by fixing that utility function.