Fix multiple issues surrounding trap deactivation (#659)

- Consistently call onDeactivate, onPostDeactivate, checkCanReturnFocus

These options were not being called consistently, particularly because
of the way focus-trap-react tries to participate in the trap's
deactivation process in order to control how focus is returned
(if the trap is configured to return focus after deactivation).

- Ensure an outside click permitted to deactivate the trap with the
  `clickOutsideDeactivates=true` option is actually permitted to so
  _and_ focus remains on the outside node clicked rather than returning
  to the node that had focus prior to the trap being activated (which
  is the default behavior).

- Required bump to focus-trap v6.9.0 for bug fixes and new features
  to help with above fixes.

.changeset/fifty-houses-occur.md

@@ -0,0 +1,5 @@
+---
+'focus-trap-react': minor
+---
+
+Bump focus-trap to v6.9.0 to get bug fixes and new features to help fix some bugs.

.changeset/good-islands-enjoy.md

@@ -0,0 +1,5 @@
+---
+'focus-trap-react': patch
+---
+
+Fix onDeactivate, onPostDeactivate, and checkCanReturnFocus options not being called consistently on deactivation.

.changeset/honest-mice-call.md

@@ -0,0 +1,5 @@
+---
+'focus-trap-react': patch
+---
+
+Fix focus not being allowed to remain on outside node post-deactivation when `clickOutsideDeactivates` is true or returns true.

.gitignore

@@ -70,3 +70,4 @@ dist/
 .Thumbs
 
 cypress/videos
+cypress/screenshots

demo/js/demo-setReturnFocus.js

@@ -1,17 +1,21 @@
-const { useState } = require('react');
+const { useState, useMemo } = require('react');
 const React = require('react');
 const { createRoot } = require('react-dom/client');
 const FocusTrap = require('../../dist/focus-trap-react');
 
 const container = document.getElementById('demo-setReturnFocus');
 
-const focusTrapOptions = {
-  setReturnFocus: '#AlternateReturnFocusElement',
-};
-
 const DemoSetReturnFocusDialog = () => {
   const [isTrapActive, setIsTrapActive] = useState(false);
 
+  const focusTrapOptions = useMemo(
+    () => ({
+      setReturnFocus: '#AlternateReturnFocusElement',
+      onDeactivate: () => setIsTrapActive(false),
+    }),
+    []
+  );
+
   return (
     <>
       <div>

demo/js/demo-special-element.js

@@ -51,7 +51,7 @@ class DemoSpecialElement extends React.Component {
         <FocusTrap
           active={this.state.activeTrap}
           focusTrapOptions={{
-            onDeactivate: this.unmountTrap,
+            onPostDeactivate: this.unmountTrap,
             clickOutsideDeactivates: true,
             returnFocusOnDeactivate: true,
           }}

package.json

@@ -26,6 +26,7 @@
     "test:coverage": "jest --coverage",
     "test:cypress": "start-server-and-test start 9966 'cypress open'",
     "test:cypress:ci": "start-server-and-test start 9966 'cypress run --browser $CYPRESS_BROWSER --headless'",
+    "test:chrome": "CYPRESS_BROWSER=chrome yarn test:cypress:ci",
     "test": "yarn format:check && yarn lint && yarn test:unit && yarn test:types && CYPRESS_BROWSER=chrome yarn test:cypress:ci",
     "prepare": "yarn build",
     "prepublishOnly": "yarn test && yarn build",
@@ -92,7 +93,7 @@
     "typescript": "^4.6.3"
   },
   "dependencies": {
-    "focus-trap": "^6.8.1"
+    "focus-trap": "^6.9.0"
   },
   "peerDependencies": {
     "prop-types": "^15.8.1",

src/focus-trap-react.js

@@ -2,6 +2,7 @@ const React = require('react');
 const ReactDOM = require('react-dom');
 const PropTypes = require('prop-types');
 const { createFocusTrap } = require('focus-trap');
+const { isFocusable } = require('tabbable');
 
 // TODO: These issues are related to older React features which we'll likely need
 //  to fix in order to move the code forward to the next major version of React.
@@ -12,38 +13,76 @@ class FocusTrap extends React.Component {
   constructor(props) {
     super(props);
 
-    // We need to hijack the returnFocusOnDeactivate option,
-    // because React can move focus into the element before we arrived at
-    // this lifecycle hook (e.g. with autoFocus inputs). So the component
-    // captures the previouslyFocusedElement in componentWillMount,
-    // then (optionally) returns focus to it in componentWillUnmount.
-    this.tailoredFocusTrapOptions = {
+    this.handleDeactivate = this.handleDeactivate.bind(this);
+    this.handlePostDeactivate = this.handlePostDeactivate.bind(this);
+    this.handleClickOutsideDeactivates =
+      this.handleClickOutsideDeactivates.bind(this);
+
+    // focus-trap options used internally when creating the trap
+    this.internalOptions = {
+      // We need to hijack the returnFocusOnDeactivate option,
+      // because React can move focus into the element before we arrived at
+      // this lifecycle hook (e.g. with autoFocus inputs). So the component
+      // captures the previouslyFocusedElement in componentWillMount,
+      // then (optionally) returns focus to it in componentWillUnmount.
       returnFocusOnDeactivate: false,
+
+      // the rest of these are also related to deactivation of the trap, and we
+      //  need to use them and control them as well
+      checkCanReturnFocus: null,
+      onDeactivate: this.handleDeactivate,
+      onPostDeactivate: this.handlePostDeactivate,
+
+      // we need to special-case this setting as well so that we can know if we should
+      //  NOT return focus if the trap gets auto-deactivated as the result of an
+      //  outside click (otherwise, we'll always think we should return focus because
+      //  of how we manage that flag internally here)
+      clickOutsideDeactivates: this.handleClickOutsideDeactivates,
     };
 
-    // because of the above, we maintain our own flag for this option, and
-    //  default it to `true` because that's focus-trap's default
-    this.returnFocusOnDeactivate = true;
+    // original options provided by the consumer
+    this.originalOptions = {
+      // because of the above `tailoredFocusTrapOptions`, we maintain our own flag for
+      //  this option, and default it to `true` because that's focus-trap's default
+      returnFocusOnDeactivate: true,
+
+      // because of the above `tailoredFocusTrapOptions`, we keep these separate since
+      //  they're part of the deactivation process which we configure (internally) to
+      //  be shared between focus-trap and focus-trap-react
+      onDeactivate: null,
+      onPostDeactivate: null,
+      checkCanReturnFocus: null,
+
+      // the user's setting, defaulted to false since focus-trap defaults this to false
+      clickOutsideDeactivates: false,
+    };
 
     const { focusTrapOptions } = props;
     for (const optionName in focusTrapOptions) {
       if (!Object.prototype.hasOwnProperty.call(focusTrapOptions, optionName)) {
         continue;
       }
 
-      if (optionName === 'returnFocusOnDeactivate') {
-        this.returnFocusOnDeactivate = !!focusTrapOptions[optionName];
-        continue;
-      }
-
-      if (optionName === 'onPostDeactivate') {
-        this.onPostDeactivate = focusTrapOptions[optionName];
-        continue;
+      if (
+        optionName === 'returnFocusOnDeactivate' ||
+        optionName === 'onDeactivate' ||
+        optionName === 'onPostDeactivate' ||
+        optionName === 'checkCanReturnFocus' ||
+        optionName === 'clickOutsideDeactivates'
+      ) {
+        this.originalOptions[optionName] = focusTrapOptions[optionName];
+        continue; // exclude from tailoredFocusTrapOptions
       }
 
-      this.tailoredFocusTrapOptions[optionName] = focusTrapOptions[optionName];
+      this.internalOptions[optionName] = focusTrapOptions[optionName];
     }
 
+    // if set, `{ target: Node, allowDeactivation: boolean }` where `target` is the outside
+    //  node that was clicked, and `allowDeactivation` is the result of the consumer's
+    //  option (stored in `this.originalOptions.clickOutsideDeactivates`, which may be a
+    //  function) whether to allow or deny auto-deactivation on click on this outside node
+    this.outsideClick = null;
+
     // elements from which to create the focus trap on mount; if a child is used
     //  instead of the `containerElements` prop, we'll get the child's related
     //  element when the trap renders and then is declared 'mounted'
@@ -69,7 +108,7 @@ class FocusTrap extends React.Component {
 
   // TODO: Need more test coverage for this function
   getNodeForOption(optionName) {
-    const optionValue = this.tailoredFocusTrapOptions[optionName];
+    const optionValue = this.internalOptions[optionName];
     if (!optionValue) {
       return null;
     }
@@ -108,36 +147,102 @@ class FocusTrap extends React.Component {
   }
 
   deactivateTrap() {
-    const { checkCanReturnFocus, preventScroll = false } =
-      this.tailoredFocusTrapOptions;
+    // NOTE: it's possible the focus trap has already been deactivated without our knowing it,
+    //  especially if the user set the `clickOutsideDeactivates: true` option on the trap,
+    //  and the mouse was clicked on some element outside the trap; at that point, focus-trap
+    //  will initiate its auto-deactivation process, which will call our own
+    //  handleDeactivate(), which will call into this method
+    if (!this.focusTrap || !this.focusTrap.active) {
+      return;
+    }
 
-    if (this.focusTrap) {
+    this.focusTrap.deactivate({
       // NOTE: we never let the trap return the focus since we do that ourselves
-      this.focusTrap.deactivate({ returnFocus: false });
+      returnFocus: false,
+      // we'll call this in our own post deactivate handler so make sure the trap doesn't
+      //  do it prematurely
+      checkCanReturnFocus: null,
+      // let it call the user's original deactivate handler, if any, instead of
+      //  our own which calls back into this function
+      onDeactivate: this.originalOptions.onDeactivate,
+      // NOTE: for post deactivate, don't specify anything so that it calls the
+      //  onPostDeactivate handler specified on `this.internalOptions`
+      //  which will always be our own `handlePostDeactivate()` handler, which
+      //  will finish things off by calling the user's provided onPostDeactivate
+      //  handler, if any, at the right time
+      // onPostDeactivate: NOTHING
+    });
+  }
+
+  handleClickOutsideDeactivates(event) {
+    // use consumer's option (or call their handler) as the permission or denial
+    const allowDeactivation =
+      typeof this.originalOptions.clickOutsideDeactivates === 'function'
+        ? this.originalOptions.clickOutsideDeactivates.call(null, event) // call out of context
+        : this.originalOptions.clickOutsideDeactivates; // boolean
+
+    if (allowDeactivation) {
+      // capture the outside target that was clicked so we can use it in the deactivation
+      //  process since the consumer allowed it to cause auto-deactivation
+      this.outsideClick = {
+        target: event.target,
+        allowDeactivation,
+      };
+    }
+
+    return allowDeactivation;
+  }
+
+  handleDeactivate() {
+    if (this.originalOptions.onDeactivate) {
+      this.originalOptions.onDeactivate.call(null); // call user's handler out of context
     }
+    this.deactivateTrap();
+  }
 
+  handlePostDeactivate() {
     const finishDeactivation = () => {
       const returnFocusNode = this.getReturnFocusNode();
-      const canReturnFocus =
-        returnFocusNode?.focus && this.returnFocusOnDeactivate;
+      const canReturnFocus = !!(
+        // did the consumer allow it?
+        (
+          this.originalOptions.returnFocusOnDeactivate &&
+          // can we actually focus the node?
+          returnFocusNode?.focus &&
+          // was there an outside click that allowed deactivation?
+          (!this.outsideClick ||
+            // did the consumer allow deactivation when the outside node was clicked?
+            (this.outsideClick.allowDeactivation &&
+              // is the outside node NOT focusable (implying that it did NOT receive focus
+              //  as a result of the click-through) -- in which case do NOT restore focus
+              //  to `returnFocusNode` because focus should remain on the outside node
+              !isFocusable(
+                this.outsideClick.target,
+                this.internalOptions.tabbableOptions
+              )))
+        )
+        // if no, the restore focus to `returnFocusNode` at this point
+      );
+      const { preventScroll = false } = this.internalOptions;
 
       if (canReturnFocus) {
-        /** Returns focus to the element that had focus when the trap was activated. */
+        // return focus to the element that had focus when the trap was activated
         returnFocusNode.focus({
           preventScroll,
         });
       }
 
-      if (this.onPostDeactivate) {
-        this.onPostDeactivate.call(null); // don't call it in context of "this"
+      if (this.originalOptions.onPostDeactivate) {
+        this.originalOptions.onPostDeactivate.call(null); // don't call it in context of "this"
       }
+
+      this.outsideClick = null; // reset: no longer needed
     };
 
-    if (checkCanReturnFocus) {
-      checkCanReturnFocus(this.getReturnFocusNode()).then(
-        finishDeactivation,
-        finishDeactivation
-      );
+    if (this.originalOptions.checkCanReturnFocus) {
+      this.originalOptions.checkCanReturnFocus
+        .call(null, this.getReturnFocusNode()) // call out of context
+        .then(finishDeactivation, finishDeactivation);
     } else {
       finishDeactivation();
     }
@@ -157,7 +262,7 @@ class FocusTrap extends React.Component {
         // eslint-disable-next-line react/prop-types -- _createFocusTrap is an internal prop
         this.focusTrap = this.props._createFocusTrap(
           focusTrapElementDOMNodes,
-          this.tailoredFocusTrapOptions
+          this.internalOptions
         );
 
         if (this.props.active) {

test/focus-trap-react.test.js

@@ -43,8 +43,11 @@ const FocusTrapExample = ({ focusTrapOptions, ...otherProps }) => {
   const unmountTrap = () => setTrapIsActive(false);
 
   const options = getTestFocusTrapOptions({
-    onDeactivate: unmountTrap,
     ...focusTrapOptions,
+    onDeactivate: () => {
+      focusTrapOptions?.onDeactivate?.();
+      unmountTrap();
+    },
   });
 
   const trap = (
@@ -482,8 +485,10 @@ describe('FocusTrap', () => {
       // Deactivate the focus trap
       fireEvent.click(screen.getByText('deactivate trap'));
 
-      expect(onDeactivate).toHaveBeenCalled();
-      expect(onPostDeactivate).toHaveBeenCalled();
+      await waitFor(() => {
+        expect(onDeactivate).toHaveBeenCalled();
+        expect(onPostDeactivate).toHaveBeenCalled();
+      });
     });
 
     it('Will call onPostDeactivate() even if returnFocusOnDeactivate is false', async () => {
@@ -516,8 +521,10 @@ describe('FocusTrap', () => {
       // Deactivate the focus trap
       fireEvent.click(screen.getByText('deactivate trap'));
 
-      expect(onDeactivate).toHaveBeenCalled();
-      expect(onPostDeactivate).toHaveBeenCalled();
+      await waitFor(() => {
+        expect(onDeactivate).toHaveBeenCalled();
+        expect(onPostDeactivate).toHaveBeenCalled();
+      });
     });
 
     ['string', 'element', 'function'].forEach((elementSelectionMethod) => {

yarn.lock

@@ -4329,10 +4329,10 @@ flatted@^3.1.0:
   resolved "https://registry.yarnpkg.com/flatted/-/flatted-3.1.0.tgz#a5d06b4a8b01e3a63771daa5cb7a1903e2e57067"
   integrity sha512-tW+UkmtNg/jv9CSofAKvgVcO7c2URjhTdW1ZTkcAritblu8tajiYy7YisnIflEwtKssCtOxpnBRoCB7iap0/TA==
 
-focus-trap@^6.8.1:
-  version "6.8.1"
-  resolved "https://registry.yarnpkg.com/focus-trap/-/focus-trap-6.8.1.tgz#0c9e4e44db8f7242f3d4b1056a518747d9c97125"
-  integrity sha512-sdz/jAPiP/9cyElo31+X3/estGPi6wgHutg+R/3MFmJtMM5AeeBlFGplejQyy89Ouyds/9xW+qPEH3jFlOAuKg==
+focus-trap@^6.9.0:
+  version "6.9.0"
+  resolved "https://registry.yarnpkg.com/focus-trap/-/focus-trap-6.9.0.tgz#d72a1ba17ac1b500bd857c6b01f072b8cfd97f6e"
+  integrity sha512-Yv3ieSeAPbfjzjU6xIuF1yAGw0kIKO5EkEJL9o/8MYfBcr99cV7dE6rJM4slk1itDHHeEhoNorQVzvEIT1rNsw==
   dependencies:
     tabbable "^5.3.1"