diff --git a/.changeset/strong-emus-do.md b/.changeset/strong-emus-do.md new file mode 100644 index 00000000..4f46cfa9 --- /dev/null +++ b/.changeset/strong-emus-do.md @@ -0,0 +1,5 @@ +--- +'focus-trap-react': patch +--- + +Fix focus not always returning to correct node after setting `active` prop to `false`. #139 diff --git a/cypress/integration/focus-trap-demo.spec.js b/cypress/integration/focus-trap-demo.spec.js index e236b192..7eeedfd0 100644 --- a/cypress/integration/focus-trap-demo.spec.js +++ b/cypress/integration/focus-trap-demo.spec.js @@ -13,145 +13,232 @@ describe(' component', () => { cy.get(focusedElAlias).should('be.focused'); } - it('By default focus first element in its tab order and trap focus within its children', () => { - cy.get('#demo-defaults').as('testRoot'); - - // activate trap - cy.get('@testRoot') - .findByRole('button', { name: 'activate trap' }) - .as('lastlyFocusedElementBeforeTrapIsActivated') - .click(); - - // 1st element should be focused - cy.get('@testRoot') - .findByRole('link', { name: 'with' }) - .as('firstElementInTrap') - .should('be.focused'); - - // crucial focus-trap feature: mouse click is trapped - verifyCrucialFocusTrapOnClicking('@firstElementInTrap'); - - // trap is active(keep focus in trap by tabbing through the focus trap's tabbable elements) - cy.get('@firstElementInTrap') - .tab() - .should('have.text', 'some') - .should('be.focused') - .tab() - .should('have.text', 'focusable') - .should('be.focused') - .tab() - .as('lastElementInTrap') - .should('have.text', 'deactivate trap') - .should('be.focused') - .tab(); - - // trap is active(keep focus in trap by shift-tabbing through the focus trap's tabbable elements) - cy.get('@firstElementInTrap').should('be.focused').tab({ shift: true }); - cy.get('@lastElementInTrap').should('be.focused'); - - // trap can be deactivated and return focus to lastly focused element before trap is activated - cy.get('@testRoot') - .findByRole('button', { name: 'deactivate trap' }) - .click(); - cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should('have.focus'); - - // focus can be transitioned freely when trap is unmounted - let previousFocusedEl; - cy.get('@lastlyFocusedElementBeforeTrapIsActivated') - .then(([lastlyFocusedEl]) => (previousFocusedEl = lastlyFocusedEl)) - .tab(); - cy.focused().should(([nextFocusedEl]) => - expect(nextFocusedEl).not.equal(previousFocusedEl) - ); - }); - - it('On trap mounts and activates, focus on manually specified input element', () => { - cy.get('#demo-ffne').as('testRoot'); - - // activate trap - cy.get('@testRoot').findByRole('button', { name: 'activate trap' }).click(); - - // instead of next tab-order element being focused, element specified should be focused - cy.get('@testRoot') - .findByRole('textbox', { name: 'Initially focused input' }) - .as('focusedEl') - .should('be.focused'); - - // crucial focus-trap feature: mouse click is trapped - verifyCrucialFocusTrapOnClicking('@focusedEl'); - }); - - it('Escape key does not deactivate trap. Instead, click on "deactivate trap" to deactivate trap', () => { - cy.get('#demo-ffne').as('testRoot'); - - // activate trap - cy.get('@testRoot') - .findByRole('button', { name: 'activate trap' }) - .as('lastlyFocusedElementBeforeTrapIsActivated') - .click(); - - // trying deactivate trap by ESC - cy.get('@testRoot') - .findByRole('textbox', { name: 'Initially focused input' }) - .as('trapChild') - .focus() - .type('{esc}'); - - // ESC does not deactivate the trap - cy.get('@trapChild').should('exist').should('be.focused'); - - // crucial focus-trap feature: mouse click is trapped - verifyCrucialFocusTrapOnClicking('@trapChild'); - - // click on deactivate trap button to deactivate trap - cy.get('@testRoot') - .findByRole('button', { name: 'deactivate trap' }) - .click(); - cy.get('@trapChild').should('not.exist'); - cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should('be.focused'); + describe('demo: defaults', () => { + it('By default focus first element in its tab order and trap focus within its children', () => { + cy.get('#demo-defaults').as('testRoot'); + + // activate trap + cy.get('@testRoot') + .findByRole('button', { name: 'activate trap' }) + .as('lastlyFocusedElementBeforeTrapIsActivated') + .click(); + + // 1st element should be focused + cy.get('@testRoot') + .findByRole('link', { name: 'with' }) + .as('firstElementInTrap') + .should('be.focused'); + + // crucial focus-trap feature: mouse click is trapped + verifyCrucialFocusTrapOnClicking('@firstElementInTrap'); + + // trap is active(keep focus in trap by tabbing through the focus trap's tabbable elements) + cy.get('@firstElementInTrap') + .tab() + .should('have.text', 'some') + .should('be.focused') + .tab() + .should('have.text', 'focusable') + .should('be.focused') + .tab() + .as('lastElementInTrap') + .should('have.text', 'deactivate trap') + .should('be.focused') + .tab(); + + // trap is active(keep focus in trap by shift-tabbing through the focus trap's tabbable elements) + cy.get('@firstElementInTrap').should('be.focused').tab({ shift: true }); + cy.get('@lastElementInTrap').should('be.focused'); + + // trap can be deactivated and return focus to lastly focused element before trap is activated + cy.get('@testRoot') + .findByRole('button', { name: 'deactivate trap' }) + .click(); + cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should('have.focus'); + + // focus can be transitioned freely when trap is unmounted + let previousFocusedEl; + cy.get('@lastlyFocusedElementBeforeTrapIsActivated') + .then(([lastlyFocusedEl]) => (previousFocusedEl = lastlyFocusedEl)) + .tab(); + cy.focused().should(([nextFocusedEl]) => + expect(nextFocusedEl).not.equal(previousFocusedEl) + ); + }); }); - it('Can be deactivated while staying mounted ', () => { - cy.get('#demo-special-element').as('testRoot'); - - // activate trap - cy.get('@testRoot') - .findByRole('button', { name: 'activate trap' }) - .as('lastlyFocusedElementBeforeTrapIsActivated') - .click(); - - // stay mounted after deactivation - cy.get('@testRoot') - .findByRole('button', { name: 'deactivate trap' }) - .as('trapChild') - .click(); - cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should('be.focused'); - cy.get('@trapChild').should('exist'); + describe('demo: ffne', () => { + it('On trap mounts and activates, focus on manually specified input element', () => { + cy.get('#demo-ffne').as('testRoot'); + + // activate trap + cy.get('@testRoot') + .findByRole('button', { name: 'activate trap' }) + .click(); + + // instead of next tab-order element being focused, element specified should be focused + cy.get('@testRoot') + .findByRole('textbox', { name: 'Initially focused input' }) + .as('focusedEl') + .should('be.focused'); + + // crucial focus-trap feature: mouse click is trapped + verifyCrucialFocusTrapOnClicking('@focusedEl'); + }); + + it('Escape key does not deactivate trap. Instead, click on "deactivate trap" to deactivate trap', () => { + cy.get('#demo-ffne').as('testRoot'); + + // activate trap + cy.get('@testRoot') + .findByRole('button', { name: 'activate trap' }) + .as('lastlyFocusedElementBeforeTrapIsActivated') + .click(); + + // trying deactivate trap by ESC + cy.get('@testRoot') + .findByRole('textbox', { name: 'Initially focused input' }) + .as('trapChild') + .focus() + .type('{esc}'); + + // ESC does not deactivate the trap + cy.get('@trapChild').should('exist').should('be.focused'); + + // crucial focus-trap feature: mouse click is trapped + verifyCrucialFocusTrapOnClicking('@trapChild'); + + // click on deactivate trap button to deactivate trap + cy.get('@testRoot') + .findByRole('button', { name: 'deactivate trap' }) + .click(); + cy.get('@trapChild').should('not.exist'); + cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should('be.focused'); + }); }); - it('Can click outside of trap to deactivate and click carries through ', () => { - cy.get('#demo-special-element').as('testRoot'); - - // activate trap - cy.get('@testRoot').findByRole('button', { name: 'activate trap' }).click(); - - // click outside to deactivate trap and also click carries through - cy.findByRole('button', { name: 'pass thru click' }) - .click() - .should('be.focused'); - cy.get('@testRoot').findByText('Clicked!'); + describe('demo: special element', () => { + // because this test allows click-through to deactivate, we can't use + // verifyCrucialFocusTrapOnClicking() because it clicks outside, and will + // cause the trap to be deactivated + const verifyFocusTrapped = function () { + // 1st element should be focused + cy.get('@testRoot') + .findByRole('link', { name: 'with' }) + .as('firstElementInTrap') + .should('be.focused'); + + // trap is active(keep focus in trap by tabbing through the focus trap's tabbable elements) + cy.get('@firstElementInTrap') + .tab() + .should('have.text', 'some') + .should('be.focused') + .tab() + .should('have.text', 'focusable') + .should('be.focused') + .tab() + .as('lastElementInTrap') + .should('have.text', 'deactivate trap') + .should('be.focused') + .tab(); + + // trap is active(keep focus in trap by shift-tabbing through the focus trap's tabbable elements) + cy.get('@firstElementInTrap').should('be.focused').tab({ shift: true }); + cy.get('@lastElementInTrap').should('be.focused'); + }; + + it('Can be deactivated while staying mounted by clicking on deactivate', () => { + cy.get('#demo-special-element').as('testRoot'); + + // activate trap + cy.get('@testRoot') + .findByRole('button', { name: 'activate trap' }) + .as('lastlyFocusedElementBeforeTrapIsActivated') + .click(); + + verifyFocusTrapped(); + + cy.get('@testRoot') + .findByRole('button', { name: 'deactivate trap' }) + .as('trapChild') + .click(); + + cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should('be.focused'); + + // stay mounted after deactivation + cy.get('@trapChild').should('exist'); + }); + + it('Can be deactivated while staying mounted by pressing ESC key', () => { + cy.get('#demo-special-element').as('testRoot'); + + // activate trap + cy.get('@testRoot') + .findByRole('button', { name: 'activate trap' }) + .as('lastlyFocusedElementBeforeTrapIsActivated') + .click(); + + verifyFocusTrapped(); + + cy.get('@testRoot') + .findByRole('button', { name: 'deactivate trap' }) + .as('trapChild') + .type('{esc}'); + + cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should('be.focused'); + + // stay mounted after deactivation + cy.get('@trapChild').should('exist'); + }); + + it('Can click outside of trap to deactivate and click carries through', () => { + cy.get('#demo-special-element').as('testRoot'); + + // activate trap + cy.get('@testRoot') + .findByRole('button', { name: 'activate trap' }) + .as('lastlyFocusedElementBeforeTrapIsActivated') + .click(); + + verifyFocusTrapped(); + + cy.get('@testRoot') + .findByRole('button', { name: 'deactivate trap' }) + .as('trapChild'); + + // click outside to deactivate trap and also click carries through + cy.findByRole('button', { name: 'pass thru click' }) + .as('passThru') + .click(); + + // activate button should NOT be focused because click should've gone through to passThru button + cy.get('@lastlyFocusedElementBeforeTrapIsActivated').should( + 'not.be.focused' + ); + + // stay mounted after deactivation + cy.get('@trapChild').should('exist'); + + // passThru button has focus and click event was processed + cy.get('@passThru').should('be.focused'); + cy.get('@testRoot').findByText('Clicked!'); + }); }); - it('Focus on element with "autofocus" attribute than the 1st tab order element within mounted trap children', () => { - cy.get('#demo-autofocus').as('testRoot'); + describe('demo: autofocus', () => { + it('Focus on element with "autofocus" attribute than the 1st tab order element within mounted trap children', () => { + cy.get('#demo-autofocus').as('testRoot'); - // activate trap - cy.get('@testRoot').findByRole('button', { name: 'activate trap' }).click(); + // activate trap + cy.get('@testRoot') + .findByRole('button', { name: 'activate trap' }) + .click(); - // element with "autofocus" attribute is focused - cy.findByTestId('autofocus-el').as('trapChild').should('be.focused'); + // element with "autofocus" attribute is focused + cy.findByTestId('autofocus-el').as('trapChild').should('be.focused'); - // crucial focus-trap feature: mouse click is trapped - verifyCrucialFocusTrapOnClicking('@trapChild'); + // crucial focus-trap feature: mouse click is trapped + verifyCrucialFocusTrapOnClicking('@trapChild'); + }); }); }); diff --git a/src/focus-trap-react.js b/src/focus-trap-react.js index 7cb0d4ec..8c27ce7e 100644 --- a/src/focus-trap-react.js +++ b/src/focus-trap-react.js @@ -12,50 +12,64 @@ class FocusTrap extends React.Component { constructor(props) { super(props); - if (typeof document !== 'undefined') { - this.previouslyFocusedElement = document.activeElement; - } - } - - componentDidMount() { // 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. - const specifiedFocusTrapOptions = this.props.focusTrapOptions; - const tailoredFocusTrapOptions = { + this.tailoredFocusTrapOptions = { returnFocusOnDeactivate: false, }; - for (const optionName in specifiedFocusTrapOptions) { - if ( - !Object.prototype.hasOwnProperty.call( - specifiedFocusTrapOptions, - optionName - ) - ) { + // 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; + + const { focusTrapOptions } = props; + for (const optionName in focusTrapOptions) { + if (!Object.prototype.hasOwnProperty.call(focusTrapOptions, optionName)) { continue; } if (optionName === 'returnFocusOnDeactivate') { + this.returnFocusOnDeactivate = !!focusTrapOptions[optionName]; continue; } - tailoredFocusTrapOptions[optionName] = - specifiedFocusTrapOptions[optionName]; + this.tailoredFocusTrapOptions[optionName] = focusTrapOptions[optionName]; } + // now we remember what the currently focused element is, not relying on focus-trap + this.updatePreviousElement(); + } + + /** Update the previously focused element with the currently focused element. */ + updatePreviousElement() { + if (typeof document !== 'undefined') { + this.previouslyFocusedElement = document.activeElement; + } + } + + /** Returns focus to the element that had focus when the trap was activated. */ + returnFocus() { + if (this.previouslyFocusedElement && this.previouslyFocusedElement.focus) { + this.previouslyFocusedElement.focus(); + } + } + + componentDidMount() { const focusTrapElementDOMNode = ReactDOM.findDOMNode(this.focusTrapElement); // eslint-disable-next-line react/prop-types -- _createFocusTrap is an internal prop this.focusTrap = this.props._createFocusTrap( focusTrapElementDOMNode, - tailoredFocusTrapOptions + this.tailoredFocusTrapOptions ); + if (this.props.active) { this.focusTrap.activate(); } + if (this.props.paused) { this.focusTrap.pause(); } @@ -63,11 +77,16 @@ class FocusTrap extends React.Component { componentDidUpdate(prevProps) { if (prevProps.active && !this.props.active) { - const { returnFocusOnDeactivate } = this.props.focusTrapOptions; - const returnFocus = returnFocusOnDeactivate || false; - const config = { returnFocus }; - this.focusTrap.deactivate(config); - } else if (!prevProps.active && this.props.active) { + // NOTE: we never let the trap return the focus since we do that ourselves + this.focusTrap.deactivate({ returnFocus: false }); + if (this.returnFocusOnDeactivate) { + this.returnFocus(); + } + return; // un/pause does nothing on an inactive trap + } + + if (!prevProps.active && this.props.active) { + this.updatePreviousElement(); this.focusTrap.activate(); } @@ -79,13 +98,10 @@ class FocusTrap extends React.Component { } componentWillUnmount() { - this.focusTrap.deactivate(); - if ( - this.props.focusTrapOptions.returnFocusOnDeactivate !== false && - this.previouslyFocusedElement && - this.previouslyFocusedElement.focus - ) { - this.previouslyFocusedElement.focus(); + // NOTE: we never let the trap return the focus since we do that ourselves + this.focusTrap.deactivate({ returnFocus: false }); + if (this.returnFocusOnDeactivate) { + this.returnFocus(); } } diff --git a/test/deactivation.test.js b/test/deactivation.test.js index aa143b3c..857a127b 100644 --- a/test/deactivation.test.js +++ b/test/deactivation.test.js @@ -67,7 +67,7 @@ describe('deactivation', () => { expect(mockFocusTrap.deactivate).toHaveBeenCalledTimes(1); }); - test('deactivation respects `returnFocusOnDeactivate` option', () => { + describe('deactivation respects `returnFocusOnDeactivate` option', () => { class TestZone extends React.Component { state = { trapActive: true, @@ -87,7 +87,8 @@ describe('deactivation', () => { ref={(component) => (this.trap = component)} _createFocusTrap={mockCreateFocusTrap} active={this.state.trapActive} - focusTrapOptions={{ returnFocusOnDeactivate: true }} + // eslint-disable-next-line react/prop-types + focusTrapOptions={this.props.trapOptions} >
@@ -98,15 +99,79 @@ describe('deactivation', () => { } } - const zone = ReactDOM.render(, domContainer); - // mock deactivate on the fouscTrap instance for we can asset - // that we are passing the correct config to the focus trap. - zone.trap.focusTrap.deactivate = jest.fn(); + test('returnFocusOnDeactivate = true', () => { + const zone = ReactDOM.render( + , + domContainer + ); - TestUtils.Simulate.click(ReactDOM.findDOMNode(zone.refs.trigger)); + // mock deactivate on the focusTrap instance so we can assert + // that we are passing the correct config to the focus trap. + zone.trap.focusTrap.deactivate = jest.fn(); + + // mock the FocusTrap instance's returnFocus() method so we can make sure + // it gets called + zone.trap.returnFocus = jest.fn(); + + TestUtils.Simulate.click(ReactDOM.findDOMNode(zone.refs.trigger)); + + // we should always be calling deactivate() with returnFocus=false since + // we take care of returning focus ourselves + expect(zone.trap.focusTrap.deactivate).toHaveBeenCalledWith({ + returnFocus: false, + }); + + // since we set returnFocusOnDeactivate=true, FocusTrap should've tried to return focus + expect(zone.trap.returnFocus).toHaveBeenCalled(); + }); + + test('returnFocusOnDeactivate = false', () => { + const zone = ReactDOM.render( + , + domContainer + ); + + // mock deactivate on the focusTrap instance so we can assert + // that we are passing the correct config to the focus trap. + zone.trap.focusTrap.deactivate = jest.fn(); + + // mock the FocusTrap instance's returnFocus() method so we can make sure + // it gets called + zone.trap.returnFocus = jest.fn(); + + TestUtils.Simulate.click(ReactDOM.findDOMNode(zone.refs.trigger)); + + // we should always be calling deactivate() with returnFocus=false since + // we take care of returning focus ourselves + expect(zone.trap.focusTrap.deactivate).toHaveBeenCalledWith({ + returnFocus: false, + }); + + // since we set returnFocusOnDeactivate=false, FocusTrap should NOT have tried to return focus + expect(zone.trap.returnFocus).not.toHaveBeenCalled(); + }); + + test('returnFocusOnDeactivate defaults to true', () => { + const zone = ReactDOM.render(, domContainer); + + // mock deactivate on the focusTrap instance so we can assert + // that we are passing the correct config to the focus trap. + zone.trap.focusTrap.deactivate = jest.fn(); + + // mock the FocusTrap instance's returnFocus() method so we can make sure + // it gets called + zone.trap.returnFocus = jest.fn(); + + TestUtils.Simulate.click(ReactDOM.findDOMNode(zone.refs.trigger)); + + // we should always be calling deactivate() with returnFocus=false since + // we take care of returning focus ourselves + expect(zone.trap.focusTrap.deactivate).toHaveBeenCalledWith({ + returnFocus: false, + }); - expect(zone.trap.focusTrap.deactivate).toHaveBeenCalledWith({ - returnFocus: true, + // since default is returnFocusOnDeactivate=true, FocusTrap should've tried to return focus + expect(zone.trap.returnFocus).toHaveBeenCalled(); }); });