[#893] Fix incorrect returnFocusOnDeactivate=true behavior

Fixes #893

If `clickOutsideDeactivates=true` along with the return focus option being
`true` and the outside click is on a focusable node, focus was returning to
that node instead of the node originally focused prior to activation.

Now the `returnFocusOnDeactivate` option does exactly what it says it will
do instead of the seemingly well-intentioned (based on the comment that was
removed) behavior.

.changeset/rich-melons-beam.md

@@ -0,0 +1,5 @@
+---
+'focus-trap': patch
+---
+
+Fix incorrect behavior of `returnFocusOnDeactivate` option when set to true (or defaulted to true) along with `clickOutsideDeactivates=true` and the outside click that deactivates is on a focusable node. Focus was remaining on that node instead of returning to the node focused just prior to activation. ([#893](https://github.com/focus-trap/focus-trap/issues/893))

README.md

@@ -112,6 +112,9 @@ Returns a new focus trap on `element` (one or more "containers" of tabbable node
   - 💡 When `clickOutsideDeactivates=true`, this option is **ignored** (i.e. if it's a function, it will not be called).
   - Use this option to control if (and even which) clicks are allowed outside the trap in conjunction with `clickOutsideDeactivates=false`.
 - **returnFocusOnDeactivate** `{boolean}`: Default: `true`. If `false`, when the trap is deactivated, focus will *not* return to the element that had focus before activation.
+  - 💬 When using this option in conjunction with `clickOutsideDeactivates=true`:
+    - If `returnFocusOnDeactivate=true` and the outside click causing deactivation is on a focusable element, focus will __not__ return to that element; instead, it will return to the node focused just before activation.
+    - If `returnFocusOnDeactivate=false` and the outside click is on a focusable node, focus will __remain__ on that node instead of the node focused just before activation. If the outside click is on a non-focusable node, then "nothing" will have focus post-deactivation.
 - **setReturnFocus** `{HTMLElement | SVGElement | string | (previousActiveElement: HTMLElement | SVGElement) => HTMLElement | SVGElement | string | false}`: By default, on **deactivation**, if `returnFocusOnDeactivate=true` (or if `returnFocus=true` in the [deactivation options](#trapdeactivatedeactivateoptions)), focus will be returned to the element that was focused just before activation. With this option, you can specify another element to programmatically receive focus after deactivation. It can be a DOM node, a selector string (which will be passed to `document.querySelector()` to find the DOM node **upon deactivation**), or a function that returns any of these to call **upon deactivation** (i.e. the selector and function options are only executed at the time the trap is deactivated). Can also be `false` (or return `false`) to leave focus where it is at the time of deactivation.
   - 💬 Using the selector or function options is a good way to return focus to a DOM node that may not exist at the time the trap is activated.
   - ⚠️ See warning below about **Shadow DOM** and selector strings.

cypress/e2e/focus-trap-demo.cy.js

@@ -506,6 +506,7 @@ describe('focus-trap', () => {
     });
   });
 
+  // initially-focused-container
   describe('demo: ifc', () => {
     beforeEach(() => {
       cy.get('#demo-ifc').as('testRoot');
@@ -515,6 +516,7 @@ describe('focus-trap', () => {
       // activate trap
       cy.get('@testRoot')
         .findByRole('button', { name: /^activate trap/ })
+        .as('lastlyFocusedElementBeforeTrapIsActivated')
         .click();
 
       // instead of next tab-order element being focused, initial focus element should be focused
@@ -537,19 +539,23 @@ describe('focus-trap', () => {
       cy.get('@firstTabbableElInTrap').should('be.focused');
 
       // click on outside element deactivates this trap
-      cy.findByRole('heading', { name: 'focus-trap demo' })
-        .as('outsideFocusedEl')
-        .click();
+      cy.findByRole('heading', { name: 'focus-trap demo' }).click();
       cy.get('@firstTabbableElInTrap').should('be.not.focused');
 
       // focus can be transitioned freely when trap is deactivated
-      verifyFocusIsNotTrapped(cy.get('@outsideFocusedEl'));
+      // NOTE: a heading is NOT focusable, and this demo uses the default option of
+      //  `returnFocusOnDeactivate=true`, so that means focus will return to the node
+      //  that had focus just before activation
+      verifyFocusIsNotTrapped(
+        cy.get('@lastlyFocusedElementBeforeTrapIsActivated')
+      );
     });
 
     it('specify element to be focused (even with attribute tabindex="-1") after focus trap activation, and use reverse tab order', () => {
       // activate trap
       cy.get('@testRoot')
         .findByRole('button', { name: /^activate trap/ })
+        .as('lastlyFocusedElementBeforeTrapIsActivated')
         .click();
 
       // instead of next tab-order element being focused, initial focus element should be focused
@@ -577,13 +583,16 @@ describe('focus-trap', () => {
       cy.get('@firstTabbableElInTrap').should('be.focused');
 
       // click on outside element deactivates this trap
-      cy.findByRole('heading', { name: 'focus-trap demo' })
-        .as('outsideFocusedEl')
-        .click();
+      cy.findByRole('heading', { name: 'focus-trap demo' }).click();
       cy.get('@firstTabbableElInTrap').should('be.not.focused');
 
       // focus can be transitioned freely when trap is deactivated
-      verifyFocusIsNotTrapped(cy.get('@outsideFocusedEl'));
+      // NOTE: a heading is NOT focusable, and this demo uses the default option of
+      //  `returnFocusOnDeactivate=true`, so that means focus will return to the node
+      //  that had focus just before activation
+      verifyFocusIsNotTrapped(
+        cy.get('@lastlyFocusedElementBeforeTrapIsActivated')
+      );
     });
   });
 
@@ -996,13 +1005,19 @@ describe('focus-trap', () => {
         cy.get('#checkbox-clickoutsidedeactivates').click();
         cy.get('#checkbox-clickoutsidedeactivates').should('be.checked');
 
-        // implies trap no longer active since checkbox is outside trap
-        cy.get('#checkbox-clickoutsidedeactivates').should('be.focused');
+        // implies trap no longer active since checkbox is outside trap, but note that since
+        //  returnFocusOnDeactivate=TRUE, focus will be on the element focused just before
+        //  activation, not the checkbox, even though the checkbox is focusable
+        cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should(
+          'be.focused'
+        );
 
         cy.get('@lastElementInTrap').should('not.be.focused');
 
         // focus can be transitioned freely when trap is deactivated
-        verifyFocusIsNotTrapped(cy.get('#checkbox-clickoutsidedeactivates'));
+        verifyFocusIsNotTrapped(
+          cy.get('@lastlyFocusedElementBeforeTrapIsActivated')
+        );
       });
 
       it('traps focus, deactivates on outside click on checkbox and checkbox focused, clickOutsideDeactivates=Function, returnFocusOnDeactivate=true', () => {
@@ -1025,13 +1040,19 @@ describe('focus-trap', () => {
         cy.get('#checkbox-clickoutsidedeactivates').click();
         cy.get('#checkbox-clickoutsidedeactivates').should('be.checked');
 
-        // implies trap no longer active since checkbox is outside trap
-        cy.get('#checkbox-clickoutsidedeactivates').should('be.focused');
+        // implies trap no longer active since checkbox is outside trap, but note that since
+        //  returnFocusOnDeactivate=TRUE, focus will be on the element focused just before
+        //  activation, not the checkbox, even though the checkbox is focusable
+        cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should(
+          'be.focused'
+        );
 
         cy.get('@lastElementInTrap').should('not.be.focused');
 
         // focus can be transitioned freely when trap is deactivated
-        verifyFocusIsNotTrapped(cy.get('#checkbox-clickoutsidedeactivates'));
+        verifyFocusIsNotTrapped(
+          cy.get('@lastlyFocusedElementBeforeTrapIsActivated')
+        );
       });
 
       it('traps focus, deactivates on outside click on document and "activate trap" button focused', () => {
@@ -1074,7 +1095,8 @@ describe('focus-trap', () => {
         cy.get('#checkbox-clickoutsidedeactivates').click();
         cy.get('#checkbox-clickoutsidedeactivates').should('be.checked');
 
-        // implies trap no longer active since checkbox is outside trap
+        // implies trap no longer active since checkbox is outside trap, and since
+        //  returnFocusOnDeactivate=FALSE, focus remains on the focusable checkbox
         cy.get('#checkbox-clickoutsidedeactivates').should('be.focused');
 
         cy.get('@lastElementInTrap').should('not.be.focused');