From c118bd8576b7067df2d0315c86b9d6c9c831f949 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 25 May 2021 10:21:03 +0800 Subject: [PATCH 1/4] Fix escape events not stopping propagation to customizer --- .../components/writing-flow/use-tab-nav.js | 1 + packages/components/src/popover/index.js | 30 ++++++++++++------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/block-editor/src/components/writing-flow/use-tab-nav.js b/packages/block-editor/src/components/writing-flow/use-tab-nav.js index 00528be96aa79..b94a6cfb86fb9 100644 --- a/packages/block-editor/src/components/writing-flow/use-tab-nav.js +++ b/packages/block-editor/src/components/writing-flow/use-tab-nav.js @@ -91,6 +91,7 @@ export default function useTabNav() { const ref = useRefEffect( ( node ) => { function onKeyDown( event ) { if ( event.keyCode === ESCAPE && ! hasMultiSelection() ) { + event.stopPropagation(); setNavigationMode( true ); return; } diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index b0e8d4bccb7a3..293a7509b1637 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -10,6 +10,7 @@ import { useRef, useState, useLayoutEffect, + useEffect, forwardRef, } from '@wordpress/element'; import { getRectangleFromRange } from '@wordpress/dom'; @@ -490,18 +491,25 @@ const Popover = ( ] ); // Event handlers - const maybeClose = ( event ) => { - // Close on escape - if ( event.keyCode === ESCAPE && onClose ) { - event.stopPropagation(); - onClose(); - } + useEffect( () => { + const container = containerRef.current; + + if ( container ) { + function maybeClose( event ) { + // Close on escape + if ( event.keyCode === ESCAPE && onClose ) { + event.stopPropagation(); + onClose(); + } + } - // Preserve original content prop behavior - if ( onKeyDown ) { - onKeyDown( event ); + container.addEventListener( 'keydown', maybeClose ); + + return () => { + container.removeEventListener( 'keydown', maybeClose ); + }; } - }; + }, [ onClose ] ); /** * Shims an onFocusOutside callback to be compatible with a deprecated @@ -587,7 +595,7 @@ const Popover = ( } ) } { ...contentProps } - onKeyDown={ maybeClose } + onKeyDown={ onKeyDown } { ...focusOutsideProps } ref={ mergedRefs } tabIndex="-1" From c470aac004a5838a613897842759e2d7a1f6e8c4 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 25 May 2021 11:20:52 +0800 Subject: [PATCH 2/4] Add e2e tests --- .../specs/widgets/customizing-widgets.test.js | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/packages/e2e-tests/specs/widgets/customizing-widgets.test.js b/packages/e2e-tests/specs/widgets/customizing-widgets.test.js index cf3a7b99e1dcd..a2d38addfd709 100644 --- a/packages/e2e-tests/specs/widgets/customizing-widgets.test.js +++ b/packages/e2e-tests/specs/widgets/customizing-widgets.test.js @@ -511,6 +511,57 @@ describe( 'Widgets Customizer', () => { "The page delivered both an 'X-Frame-Options' header and a 'Content-Security-Policy' header with a 'frame-ancestors' directive. Although the 'X-Frame-Options' header alone would have blocked embedding, it has been ignored." ); } ); + + it( 'should handle esc key events', async () => { + const widgetsPanel = await find( { + role: 'heading', + name: /Widgets/, + level: 3, + } ); + await widgetsPanel.click(); + + const footer1Section = await find( { + role: 'heading', + name: /^Footer #1/, + level: 3, + } ); + await footer1Section.click(); + + const paragraphBlock = await addBlock( 'Paragraph' ); + await page.keyboard.type( 'First Paragraph' ); + await showBlockToolbar(); + + // Open the more menu dropdown in block toolbar. + await clickBlockToolbarButton( 'Options' ); + await expect( { + role: 'menu', + name: 'Options', + } ).toBeFound(); + + // Expect pressing the Escape key to close the dropdown, + // but not close the editor. + await page.keyboard.press( 'Escape' ); + await expect( { + role: 'menu', + name: 'Options', + } ).not.toBeFound(); + await expect( paragraphBlock ).toBeVisible(); + + await paragraphBlock.focus(); + + // Expect pressing the Escape key to enter navigation mode, + // but not close the editor. + await page.keyboard.press( 'Escape' ); + await expect( { + text: /^You are currently in navigation mode\./, + selector: '*[aria-live="polite"][aria-relevant="additions text"]', + } ).toBeFound(); + await expect( paragraphBlock ).toBeVisible(); + + expect( console ).toHaveWarned( + "The page delivered both an 'X-Frame-Options' header and a 'Content-Security-Policy' header with a 'frame-ancestors' directive. Although the 'X-Frame-Options' header alone would have blocked embedding, it has been ignored." + ); + } ); } ); /** From d640c33fa17f154305ef200bff7e92ca83a54e3e Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 25 May 2021 18:14:49 +0800 Subject: [PATCH 3/4] Use useRefEffect --- packages/components/src/popover/index.js | 44 ++++++++++++------------ 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index 293a7509b1637..33cd215ed2185 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -10,7 +10,6 @@ import { useRef, useState, useLayoutEffect, - useEffect, forwardRef, } from '@wordpress/element'; import { getRectangleFromRange } from '@wordpress/dom'; @@ -24,6 +23,7 @@ import { useConstrainedTabbing, useFocusReturn, useMergeRefs, + useRefEffect, } from '@wordpress/compose'; import { close } from '@wordpress/icons'; @@ -478,6 +478,26 @@ const Popover = ( __unstableBoundaryParent, ] ); + // Event handlers for closing the popover. + const closeEventRef = useRefEffect( + ( node ) => { + function maybeClose( event ) { + // Close on escape. + if ( event.keyCode === ESCAPE && onClose ) { + event.stopPropagation(); + onClose(); + } + } + + node.addEventListener( 'keydown', maybeClose ); + + return () => { + node.removeEventListener( 'keydown', maybeClose ); + }; + }, + [ onClose ] + ); + const constrainedTabbingRef = useConstrainedTabbing(); const focusReturnRef = useFocusReturn(); const focusOnMountRef = useFocusOnMount( focusOnMount ); @@ -485,32 +505,12 @@ const Popover = ( const mergedRefs = useMergeRefs( [ ref, containerRef, + closeEventRef, focusOnMount ? constrainedTabbingRef : null, focusOnMount ? focusReturnRef : null, focusOnMount ? focusOnMountRef : null, ] ); - // Event handlers - useEffect( () => { - const container = containerRef.current; - - if ( container ) { - function maybeClose( event ) { - // Close on escape - if ( event.keyCode === ESCAPE && onClose ) { - event.stopPropagation(); - onClose(); - } - } - - container.addEventListener( 'keydown', maybeClose ); - - return () => { - container.removeEventListener( 'keydown', maybeClose ); - }; - } - }, [ onClose ] ); - /** * Shims an onFocusOutside callback to be compatible with a deprecated * onClickOutside prop function, if provided. From 25f07fb19bef96b987588f6e26ea7478194e9c65 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Wed, 26 May 2021 01:49:05 +0800 Subject: [PATCH 4/4] Address code reviews --- packages/components/src/popover/index.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/components/src/popover/index.js b/packages/components/src/popover/index.js index 33cd215ed2185..befbb5b9139f7 100644 --- a/packages/components/src/popover/index.js +++ b/packages/components/src/popover/index.js @@ -233,7 +233,6 @@ const Popover = ( { headerTitle, onClose, - onKeyDown, children, className, noArrow = true, @@ -505,7 +504,8 @@ const Popover = ( const mergedRefs = useMergeRefs( [ ref, containerRef, - closeEventRef, + // Don't register the event at all if there's no onClose callback. + onClose ? closeEventRef : null, focusOnMount ? constrainedTabbingRef : null, focusOnMount ? focusReturnRef : null, focusOnMount ? focusOnMountRef : null, @@ -595,7 +595,6 @@ const Popover = ( } ) } { ...contentProps } - onKeyDown={ onKeyDown } { ...focusOutsideProps } ref={ mergedRefs } tabIndex="-1"