From d07ee375b6ede1c4fd360c51c8fbef1b5d516d18 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 15 Aug 2019 12:24:35 +0100 Subject: [PATCH 1/3] Adds check to determine whether event occured within element trapping focus Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element. This may have been a regression introduced in https://github.com/WordPress/gutenberg/pull/16878 --- .../components/src/higher-order/with-focus-outside/index.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/components/src/higher-order/with-focus-outside/index.js b/packages/components/src/higher-order/with-focus-outside/index.js index 960f5abe12aa6a..3f1cc6985d9db8 100644 --- a/packages/components/src/higher-order/with-focus-outside/index.js +++ b/packages/components/src/higher-order/with-focus-outside/index.js @@ -79,7 +79,8 @@ export default createHigherOrderComponent( } this.blurCheckTimeout = setTimeout( () => { - if ( 'function' === typeof this.node.handleFocusOutside ) { + const eventTargetElIsWithinComponent = this.node.containerRef.current && this.node.containerRef.current.contains( event.target ); + if ( 'function' === typeof this.node.handleFocusOutside && ! eventTargetElIsWithinComponent ) { this.node.handleFocusOutside( event ); } }, 0 ); From 490d211efa860b8be12f3ad70aaeb50dcf4d4ea5 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 19 Aug 2019 14:56:36 +0100 Subject: [PATCH 2/3] Fix handling blur event when document is not actually focused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur. --- .../src/higher-order/with-focus-outside/index.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/components/src/higher-order/with-focus-outside/index.js b/packages/components/src/higher-order/with-focus-outside/index.js index 3f1cc6985d9db8..5f178b4a34df28 100644 --- a/packages/components/src/higher-order/with-focus-outside/index.js +++ b/packages/components/src/higher-order/with-focus-outside/index.js @@ -79,8 +79,15 @@ export default createHigherOrderComponent( } this.blurCheckTimeout = setTimeout( () => { - const eventTargetElIsWithinComponent = this.node.containerRef.current && this.node.containerRef.current.contains( event.target ); - if ( 'function' === typeof this.node.handleFocusOutside && ! eventTargetElIsWithinComponent ) { + // If document is not focused then focus should remain + // inside the wrapped component and therefore we cancel + // this blur event thereby leaving focus in place. + // https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus. + if ( ! document.hasFocus() ) { + event.preventDefault(); + return; + } + if ( 'function' === typeof this.node.handleFocusOutside ) { this.node.handleFocusOutside( event ); } }, 0 ); From 73a26b6dfc18541547d28da14ea0656c26e2f2e5 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 19 Aug 2019 15:21:13 +0100 Subject: [PATCH 3/3] Update to test for document loss of focus scenario --- .../with-focus-outside/test/index.js | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/components/src/higher-order/with-focus-outside/test/index.js b/packages/components/src/higher-order/with-focus-outside/test/index.js index dd39f910981c7e..ac47e3a57290a2 100644 --- a/packages/components/src/higher-order/with-focus-outside/test/index.js +++ b/packages/components/src/higher-order/with-focus-outside/test/index.js @@ -17,6 +17,8 @@ import ReactDOM from 'react-dom'; let wrapper, onFocusOutside; describe( 'withFocusOutside', () => { + let origHasFocus; + const EnhancedComponent = withFocusOutside( class extends Component { handleFocusOutside() { @@ -51,12 +53,21 @@ describe( 'withFocusOutside', () => { }; beforeEach( () => { + // Mock document.hasFocus() to always be true for testing + // note: we overide this for some tests. + origHasFocus = document.hasFocus; + document.hasFocus = () => true; + onFocusOutside = jest.fn(); wrapper = TestUtils.renderIntoDocument( getTestComponent( EnhancedComponent, { onFocusOutside } ) ); } ); + afterEach( () => { + document.hasFocus = origHasFocus; + } ); + it( 'should not call handler if focus shifts to element within component', () => { simulateEvent( 'focus' ); simulateEvent( 'blur' ); @@ -91,6 +102,19 @@ describe( 'withFocusOutside', () => { expect( onFocusOutside ).toHaveBeenCalled(); } ); + it( 'should not call handler if focus shifts outside the component when the document does not have focus', () => { + // Force document.hasFocus() to return false to simulate the window/document losing focus + // See https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus. + document.hasFocus = () => false; + + simulateEvent( 'focus' ); + simulateEvent( 'blur' ); + + jest.runAllTimers(); + + expect( onFocusOutside ).not.toHaveBeenCalled(); + } ); + it( 'should cancel check when unmounting while queued', () => { simulateEvent( 'focus' ); simulateEvent( 'input' );