Skip to content

Commit

Permalink
Avoid popover click-away to trigger when clicking its anchor
Browse files Browse the repository at this point in the history
  • Loading branch information
acelaya committed Nov 29, 2024
1 parent 000d13f commit 41c7ccb
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 10 deletions.
27 changes: 19 additions & 8 deletions src/components/feedback/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ function usePopoverPositioning(
*/
function useOnClose(
popoverRef: RefObject<HTMLElement | undefined>,
anchorElementRef: RefObject<HTMLElement | undefined>,
onClose: () => void,
popoverOpen: boolean,
asNativePopover: boolean,
Expand Down Expand Up @@ -194,13 +195,23 @@ function useOnClose(
// press and click away, to mimic the native popover behavior.
// Disable these while the popover is closed, otherwise trying to open it
// by interacting with some other element will trigger a click-away and
// immediately close it
useClickAway(popoverRef, onClose, {
enabled: popoverOpen && !asNativePopover,
});
useKeyPress(['Escape'], onClose, {
enabled: popoverOpen && !asNativePopover,
});
// immediately close the popover after it opens..
const enabled = popoverOpen && !asNativePopover;
useClickAway(
popoverRef,
e => {
// Ignore clicking "away" when the target is the anchor element.
// In most cases, popovers will be anchored to a "toggle" which is
// supposed to open/close the popover on click, so closing-on-click-away
// when they are the target will cause the popover to close and
// immediately open again.
if (!e.composedPath().includes(anchorElementRef.current!)) {
onClose();
}
},
{ enabled },
);
useKeyPress(['Escape'], onClose, { enabled });
}

export type PopoverProps = {
Expand Down Expand Up @@ -263,7 +274,7 @@ export default function Popover({
align === 'right',
);

useOnClose(popoverRef, onClose, open, asNativePopover);
useOnClose(popoverRef, anchorElementRef, onClose, open, asNativePopover);

useLayoutEffect(() => {
const restoreFocusTo = open
Expand Down
10 changes: 8 additions & 2 deletions src/components/feedback/test/Popover-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,10 @@ describe('Popover', () => {
context('when popover is not supported', () => {
it('closes popover when Escape is pressed', () => {
const onClose = sinon.stub();
createComponent({ onClose, asNativePopover: false });
const wrapper = createComponent({ onClose, asNativePopover: false });

// The popover needs to be open for the event handler to be attached
togglePopover(wrapper);

document.body.dispatchEvent(
new KeyboardEvent('keydown', { key: 'Escape' }),
Expand All @@ -234,7 +237,10 @@ describe('Popover', () => {

it('closes popover when clicking away', () => {
const onClose = sinon.stub();
createComponent({ onClose, asNativePopover: false });
const wrapper = createComponent({ onClose, asNativePopover: false });

// The popover needs to be open for the event handler to be attached
togglePopover(wrapper);

const externalButton = document.createElement('button');
document.body.append(externalButton);
Expand Down

0 comments on commit 41c7ccb

Please sign in to comment.