From a5446a1dbdc50aac13e8e475123d141283f075ad Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Fri, 19 Apr 2019 09:42:52 +0100 Subject: [PATCH] Fix unit tests --- .../test/__snapshots__/index.js.snap | 3 - .../src/dropdown-menu/test/index.js | 3 +- .../components/src/dropdown/test/index.js | 6 +- packages/components/src/popover/index.js | 41 +++--- .../popover/test/__snapshots__/index.js.snap | 4 +- packages/components/src/popover/test/index.js | 135 +++++++----------- 6 files changed, 86 insertions(+), 106 deletions(-) diff --git a/packages/block-editor/src/components/url-popover/test/__snapshots__/index.js.snap b/packages/block-editor/src/components/url-popover/test/__snapshots__/index.js.snap index 9613a40af346a..c835e54540c25 100644 --- a/packages/block-editor/src/components/url-popover/test/__snapshots__/index.js.snap +++ b/packages/block-editor/src/components/url-popover/test/__snapshots__/index.js.snap @@ -4,7 +4,6 @@ exports[`URLPopover matches the snapshot in its default state 1`] = `
{ let controls; @@ -79,7 +78,7 @@ describe( 'DropdownMenu', () => { } ); - expect( TestUtils.scryRenderedComponentsWithType( wrapper, Popover ) ).toHaveLength( 1 ); + expect( TestUtils.scryRenderedDOMComponentsWithClass( wrapper, 'components-popover' ) ).toHaveLength( 1 ); } ); } ); } ); diff --git a/packages/components/src/dropdown/test/index.js b/packages/components/src/dropdown/test/index.js index 7b1016a14d60e..3de873a76787b 100644 --- a/packages/components/src/dropdown/test/index.js +++ b/packages/components/src/dropdown/test/index.js @@ -7,12 +7,11 @@ import TestUtils from 'react-dom/test-utils'; * Internal dependencies */ import Dropdown from '../'; -import Popover from '../../popover'; describe( 'Dropdown', () => { const expectPopoverVisible = ( wrapper, visible ) => { expect( - TestUtils.scryRenderedComponentsWithType( wrapper, Popover ) ) + TestUtils.scryRenderedDOMComponentsWithClass( wrapper, 'components-popover' ) ) .toHaveLength( visible ? 1 : 0 ); }; const buttonElement = ( wrapper ) => TestUtils.findRenderedDOMComponentWithTag( @@ -31,13 +30,14 @@ describe( 'Dropdown', () => { buttonElement( wrapper ).getAttribute( 'aria-expanded' ) ).toBe( expanded.toString() ); }; + const wrapper = TestUtils.renderIntoDocument( ( ) } - renderContent={ () => null } + renderContent={ () => test } /> ); expectButtonExpanded( wrapper, false ); diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index ebf32e556c3a1..c432fd7cf0df7 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -182,30 +182,39 @@ const Popover = ( { }, [] ); // Focus handling - useRef( () => { - if ( ! focusOnMount || ! contentRef.current ) { - return; - } + useEffect( () => { + /* + * Without the setTimeout, the dom node is not being focused. Related: + * https://stackoverflow.com/questions/35522220/react-ref-with-focus-doesnt-work-without-settimeout-my-example + * + * TODO: Treat the cause, not the symptom. + */ + const focusTimeout = setTimeout( () => { + if ( ! focusOnMount || ! contentRef.current ) { + return; + } - if ( focusOnMount === 'firstElement' ) { + if ( focusOnMount === 'firstElement' ) { // Find first tabbable node within content and shift focus, falling // back to the popover panel itself. - const firstTabbable = focus.tabbable.find( contentRef.current )[ 0 ]; + const firstTabbable = focus.tabbable.find( contentRef.current )[ 0 ]; + if ( firstTabbable ) { + firstTabbable.focus(); + } else { + contentRef.current.focus(); + } - if ( firstTabbable ) { - firstTabbable.focus(); - } else { - contentRef.current.focus(); + return; } - return; - } - - if ( focusOnMount === 'container' ) { + if ( focusOnMount === 'container' ) { // Focus the popover panel itself so items in the popover are easily // accessed via keyboard navigation. - contentRef.current.focus(); - } + contentRef.current.focus(); + } + }, 0 ); + + return () => clearTimeout( focusTimeout ); }, [] ); // Event handlers diff --git a/packages/components/src/popover/test/__snapshots__/index.js.snap b/packages/components/src/popover/test/__snapshots__/index.js.snap index a9c4ed32102d3..9813681b30734 100644 --- a/packages/components/src/popover/test/__snapshots__/index.js.snap +++ b/packages/components/src/popover/test/__snapshots__/index.js.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`Popover #render() should pass additional props to portaled element 1`] = ` +exports[`Popover should pass additional props to portaled element 1`] = `
`; -exports[`Popover #render() should render content 1`] = ` +exports[`Popover should render content 1`] = `
{ - describe( '#componentDidUpdate()', () => { - let wrapper; - beforeEach( () => { - jest.spyOn( Popover.prototype, 'computePopoverPosition' ).mockImplementation( noop ); - jest.spyOn( Popover.prototype, 'toggleAutoRefresh' ).mockImplementation( noop ); - } ); - - afterEach( () => { - jest.restoreAllMocks(); - - // Resetting keyboard state is deferred, so ensure that timers are - // consumed to avoid leaking into other tests. - jest.runAllTimers(); - - if ( document.activeElement ) { - document.activeElement.blur(); - } - } ); +jest.useFakeTimers(); - it( 'should turn on auto refresh', () => { - wrapper = TestUtils.renderIntoDocument( ); - expect( Popover.prototype.toggleAutoRefresh ).toHaveBeenCalledWith( true ); - expect( Popover.prototype.computePopoverPosition ).toHaveBeenCalled(); - } ); +class PopoverWrapper extends Component { + render() { + return ; + } +} - it( 'should turn off auto refresh', () => { - wrapper = TestUtils.renderIntoDocument( ); - // eslint-disable-next-line react/no-find-dom-node - ReactDOM.unmountComponentAtNode( ReactDOM.findDOMNode( wrapper ).parentNode ); - expect( Popover.prototype.toggleAutoRefresh ).toHaveBeenCalledWith( false ); - } ); +describe( 'Popover', () => { + afterEach( () => { + if ( document.activeElement ) { + document.activeElement.blur(); + } + } ); - it( 'should set offset and forced positions on changed position', () => { - const node = document.createElement( 'div' ); - wrapper = ReactDOM.render( , node ); - jest.clearAllMocks(); + it( 'should focus when opening in response to keyboard event', () => { + // As in the real world, these occur in sequence before the popover + // has been mounted. Keyup's resetting is deferred. + document.dispatchEvent( new window.KeyboardEvent( 'keydown' ) ); + document.dispatchEvent( new window.KeyboardEvent( 'keyup' ) ); - ReactDOM.render( , node ); + // An ideal test here would mount with an input child and focus the + // child, but in context of JSDOM the inputs are not visible and + // are therefore skipped as tabbable, defaulting to popover. + let wrapper; + TestUtils.act( () => { + wrapper = TestUtils.renderIntoDocument( ); - expect( Popover.prototype.toggleAutoRefresh ).not.toHaveBeenCalled(); - expect( Popover.prototype.computePopoverPosition ).toHaveBeenCalled(); - } ); + jest.advanceTimersByTime( 1 ); - it( 'should focus when opening in response to keyboard event', ( done ) => { - // As in the real world, these occur in sequence before the popover - // has been mounted. Keyup's resetting is deferred. - document.dispatchEvent( new window.KeyboardEvent( 'keydown' ) ); - document.dispatchEvent( new window.KeyboardEvent( 'keyup' ) ); - - // An ideal test here would mount with an input child and focus the - // child, but in context of JSDOM the inputs are not visible and - // are therefore skipped as tabbable, defaulting to popover. - wrapper = TestUtils.renderIntoDocument( ); - - setTimeout( () => { - const content = TestUtils.findRenderedDOMComponentWithClass( - wrapper, - 'components-popover__content' - ); - expect( document.activeElement ).toBe( content ); - done(); - }, 1 ); - - jest.runAllTimers(); + const content = TestUtils.findRenderedDOMComponentWithClass( + wrapper, + 'components-popover__content' + ); + expect( document.activeElement ).toBe( content ); } ); + } ); - it( 'should allow focus-on-open behavior to be disabled', ( done ) => { - const activeElement = document.activeElement; - - wrapper = TestUtils.renderIntoDocument( ); + it( 'should allow focus-on-open behavior to be disabled', () => { + const activeElement = document.activeElement; + TestUtils.act( () => { + TestUtils.renderIntoDocument( ); - setTimeout( () => { - expect( document.activeElement ).toBe( activeElement ); - done(); - } ); + jest.advanceTimersByTime( 1 ); - jest.runAllTimers(); + expect( document.activeElement ).toBe( activeElement ); } ); } ); - describe( '#render()', () => { - it( 'should render content', () => { - const wrapper = TestUtils.renderIntoDocument( Hello ); - const content = TestUtils.findRenderedDOMComponentWithTag( wrapper, 'span' ); - - expect( content ).toMatchSnapshot(); + it( 'should render content', () => { + let wrapper; + TestUtils.act( () => { + wrapper = TestUtils.renderIntoDocument( Hello ); } ); + const content = TestUtils.findRenderedDOMComponentWithTag( wrapper, 'span' ); - it( 'should pass additional props to portaled element', () => { - const wrapper = TestUtils.renderIntoDocument( Hello ); - const content = TestUtils.findRenderedDOMComponentWithTag( wrapper, 'span' ); + expect( content ).toMatchSnapshot(); + } ); - expect( content ).toMatchSnapshot(); + it( 'should pass additional props to portaled element', () => { + let wrapper; + TestUtils.act( () => { + wrapper = TestUtils.renderIntoDocument( Hello ); } ); + const content = TestUtils.findRenderedDOMComponentWithTag( wrapper, 'span' ); + + expect( content ).toMatchSnapshot(); } ); } );