Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

123 changes: 29 additions & 94 deletions packages/components/src/popover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,12 @@ import {
forwardRef,
} from '@wordpress/element';
import { getRectangleFromRange } from '@wordpress/dom';
import { ESCAPE } from '@wordpress/keycodes';
import deprecated from '@wordpress/deprecated';
import {
useViewportMatch,
useResizeObserver,
useFocusOnMount,
__experimentalUseFocusOutside as useFocusOutside,
useConstrainedTabbing,
useFocusReturn,
useMergeRefs,
useRefEffect,
__experimentalUseDialog as useDialog,
} from '@wordpress/compose';
import { close } from '@wordpress/icons';

Expand Down Expand Up @@ -485,97 +480,37 @@ 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 );
const focusOutsideProps = useFocusOutside( handleOnFocusOutside );
const mergedRefs = useMergeRefs( [
ref,
containerRef,
// 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,
] );

/**
* Shims an onFocusOutside callback to be compatible with a deprecated
* onClickOutside prop function, if provided.
*
* @param {FocusEvent} event Focus event from onFocusOutside.
*/
function handleOnFocusOutside( event ) {
// Defer to given `onFocusOutside` if specified. Call `onClose` only if
// both `onFocusOutside` and `onClickOutside` are unspecified. Doing so
// assures backwards-compatibility for prior `onClickOutside` default.
if ( onFocusOutside ) {
const onDialogClose = ( type, event ) => {
// Ideally the popover should have just a single onClose prop and
// not three props that potentially do the same thing.
if ( type === 'focus-outside' && onFocusOutside ) {
onFocusOutside( event );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we deprecate this in the future as well, in favour of onClose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like something we could try, I'm not certain yet of the impact.

return;
} else if ( ! onClickOutside ) {
if ( onClose ) {
onClose();
}
return;
}

// Simulate MouseEvent using FocusEvent#relatedTarget as emulated click
// target. MouseEvent constructor is unsupported in Internet Explorer.
let clickEvent;
try {
clickEvent = new window.MouseEvent( 'click' );
} catch ( error ) {
clickEvent = document.createEvent( 'MouseEvent' );
clickEvent.initMouseEvent(
'click',
true,
true,
window,
0,
0,
0,
0,
0,
false,
false,
false,
false,
0,
null
);
} else if ( type === 'focus-outside' && onClickOutside ) {
// Simulate MouseEvent using FocusEvent#relatedTarget as emulated click target.
const clickEvent = new window.MouseEvent( 'click' );

Object.defineProperty( clickEvent, 'target', {
get: () => event.relatedTarget,
} );

deprecated( 'Popover onClickOutside prop', {
since: '5.3',
alternative: 'onFocusOutside',
} );

onClickOutside( clickEvent );
} else if ( onClose ) {
onClose();
}
};

Object.defineProperty( clickEvent, 'target', {
get: () => event.relatedTarget,
} );
const [ dialogRef, dialogProps ] = useDialog( {
focusOnMount,
__unstableOnClose: onDialogClose,
onClose: onDialogClose,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between these two?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also mean: do we need to set both here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should remove __unstableOnClose it's there because Popover need to differentiate between the closing when it happens on focus out and the closing when you use escape... Dialogs in general don't care and if we do your suggestion (merge onFocusOutside and onClose) we should be able to remove the unstable one

} );

deprecated( 'Popover onClickOutside prop', {
since: '5.3',
alternative: 'onFocusOutside',
} );

onClickOutside( clickEvent );
}
const mergedRefs = useMergeRefs( [ containerRef, dialogRef, ref ] );

/** @type {false | string} */
const animateClassName =
Expand Down Expand Up @@ -603,8 +538,8 @@ const Popover = (
}
) }
{ ...contentProps }
{ ...focusOutsideProps }
ref={ mergedRefs }
{ ...dialogProps }
tabIndex="-1"
>
{ isExpanded && <ScrollLock /> }
Expand Down
23 changes: 0 additions & 23 deletions packages/components/src/popover/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,29 +15,6 @@ describe( 'Popover', () => {
}
} );

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' ) );

expect( document.activeElement ).toBe( document.body );

// 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 result;
act( () => {
result = render( <Popover /> );

jest.advanceTimersByTime( 1 );
} );

expect( document.activeElement ).toBe(
result.container.querySelector( '.components-popover' )
);
} );

it( 'should allow focus-on-open behavior to be disabled', () => {
expect( document.activeElement ).toBe( document.body );

Expand Down
1 change: 0 additions & 1 deletion packages/compose/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
"@wordpress/priority-queue": "file:../priority-queue",
"clipboard": "^2.0.1",
"lodash": "^4.17.21",
"memize": "^1.1.0",
"mousetrap": "^1.6.5",
"react-resize-aware": "^3.1.0",
"use-memo-one": "^1.1.1"
Expand Down
28 changes: 18 additions & 10 deletions packages/compose/src/hooks/use-dialog/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,33 +23,41 @@ import useMergeRefs from '../use-merge-refs';
* @param {Object} options Dialog Options.
*/
function useDialog( options ) {
const onClose = useRef();
const currentOptions = useRef();
useEffect( () => {
onClose.current = options.onClose;
}, [ options.onClose ] );
currentOptions.current = options;
}, Object.values( options ) );
const constrainedTabbingRef = useConstrainedTabbing();
const focusOnMountRef = useFocusOnMount();
const focusOnMountRef = useFocusOnMount( options.focusOnMount );
const focusReturnRef = useFocusReturn();
const focusOutsideProps = useFocusOutside( options.onClose );
const focusOutsideProps = useFocusOutside( ( event ) => {
// This unstable prop is here only to manage backward compatibility
// for the Popover component otherwise, the onClose should be enough.
if ( currentOptions.current.__unstableOnClose ) {
currentOptions.current.__unstableOnClose( 'focus-outside', event );
} else if ( currentOptions.current.onClose ) {
currentOptions.current.onClose();
}
} );
const closeOnEscapeRef = useCallback( ( node ) => {
if ( ! node ) {
return;
}

node.addEventListener( 'keydown', ( event ) => {
// Close on escape
if ( event.keyCode === ESCAPE && onClose.current ) {
if ( event.keyCode === ESCAPE && currentOptions.current.onClose ) {
event.stopPropagation();
onClose.current();
currentOptions.current.onClose();
}
} );
}, [] );

return [
useMergeRefs( [
constrainedTabbingRef,
focusReturnRef,
focusOnMountRef,
options.focusOnMount !== false ? constrainedTabbingRef : null,
options.focusOnMount !== false ? focusReturnRef : null,
options.focusOnMount !== false ? focusOnMountRef : null,
closeOnEscapeRef,
] ),
{
Expand Down