Fixing a bug where the focus trap may not have been set before it is …

[cgood92]

Fixing a bug where the focus trap may not have been set before it is unmounted.

I found this when running our unit tests. Imagine this scenario.

Imagine this scenario:

const Example = () => {
  const [isOpen, setIsOpen] = useState(false)
  return  (<FocusTrap
      active={isOpen}
      containerElements={[ref1.current, ref2.current]}>
    {isOpen && (
      <>
        <div ref={ref1}/>
        <div ref={ref2}/>
        ....blah blah blah
      </>
    )}
  </FocusTrap>
  )
}

In this scenario, let's say that ref1.current and ref2.current don't get initialized before this component is unmounting. Because of that, it will try to deactivate a focusTrap that was never instantiated in the first place. See this line: https://github.com/focus-trap/focus-trap-react/blob/master/src/focus-trap-react.js#L67

This will fix an error that looks like this:

TypeError: Cannot read property 'deactivate' of undefined

      at FocusTrap.componentWillUnmount (node_modules/focus-trap-react/dist/focus-trap-react.js:171:24)

Sorry that I didn't have this in the original PR. I wasn't expecting that. I tested out the UI, but never ran the unit tests in our consuming code.

[changeset-bot]

🦋 Changeset detected

Latest commit: 8a60b16

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
focus-trap-react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[stefcameron]

@cgood92 Thanks for the fix. Would the code scenario you mentioned work well anyway? It seems like just containerElements={[ref1.current, ref2.current]} would risk resulting in [null, null] in some (other?) corner cases. Seems like you should always have an effect that sets containerElements if/when those refs get filled (or re-filled with new DOM elements if React changes the elements from under you in some render pass):

const [containerElements, setContainerElements] = useState([]);

...

useEffect(() => {
  const containers = [];

  if (ref1.current) {
    containers.push(ref1.current);
  }

  if (ref2.current) {
    containers.push(ref2.current);
  }

  setContainerElements(containers);
});

...

  return  (<FocusTrap
      active={isOpen}
      containerElements={containerElements}>
      ...
}

?

Regardless, your fix is still good to take in, though, thank you!

[stefcameron]

Just wondering about the scenario in case there's something more to this; maybe not.

[cgood92]

@stefcameron you are right, but this has already been taken into account here: https://github.com/focus-trap/focus-trap-react/blob/master/src/focus-trap-react.js#L66.

Because I thought it would be a common case that ref's were waiting to be initialized, I had us not set up a trap until at least one of the containers is there. Then, https://github.com/focus-trap/focus-trap/blob/master/index.js#L73, we filter out any falsey stuff.

So, I think these cases are already covered.

[stefcameron]

@stefcameron you are right, but this has already been taken into account here: https://github.com/focus-trap/focus-trap-react/blob/master/src/focus-trap-react.js#L66.

Because I thought it would be a common case that ref's were waiting to be initialized, I had us not set up a trap until at least one of the containers is there. Then, https://github.com/focus-trap/focus-trap/blob/master/index.js#L73, we filter out any falsey stuff.

So, I think these cases are already covered.

Got it, thanks for explaining that!