Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiFlyout] Scope close events to mask when ownFocus=true #5876

Merged
merged 10 commits into from
May 6, 2022
4 changes: 2 additions & 2 deletions src/components/collapsible_nav/collapsible_nav.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import { EuiCollapsibleNav } from './collapsible_nav';
import { EuiOverlayMaskProps } from '../overlay_mask';

jest.mock('../overlay_mask', () => ({
EuiOverlayMask: ({ headerZindexLocation, ...props }: any) => (
<div {...props} />
EuiOverlayMask: ({ headerZindexLocation, maskRef, ...props }: any) => (
<div {...props} ref={maskRef} />
),
}));

Expand Down
71 changes: 68 additions & 3 deletions src/components/flyout/flyout.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

thompsongl marked this conversation as resolved.
Show resolved Hide resolved
import React, { useState } from 'react';

import { EuiGlobalToastList } from '../toast';
import { EuiFlyout } from './flyout';

const childrenDefault = (
Expand All @@ -21,13 +22,17 @@ const childrenDefault = (
</>
);

const Flyout = ({ children = childrenDefault }) => {
const Flyout = ({ children = childrenDefault, ...rest }) => {
const [isOpen, setIsOpen] = useState(true);

return (
<>
{isOpen ? (
<EuiFlyout data-test-subj="flyoutSpec" onClose={() => setIsOpen(false)}>
<EuiFlyout
data-test-subj="flyoutSpec"
onClose={() => setIsOpen(false)}
{...rest}
>
{children}
</EuiFlyout>
) : null}
Expand Down Expand Up @@ -62,7 +67,7 @@ describe('EuiFlyout', () => {
});
});

describe('Close behavior', () => {
describe('Close behavior: standard', () => {
it('closes the flyout when the close button is clicked', () => {
cy.mount(<Flyout />);
cy.realPress('Enter').then(() => {
Expand All @@ -76,6 +81,39 @@ describe('EuiFlyout', () => {
expect(cy.get('[data-test-subj="flyoutSpec"]').should('not.exist'));
});
});
});

describe('Close behavior: outside clicks', () => {
// We're using toasts here to trigger outside clicks, as a UX case where
// we would generally expect toasts overlaid on top of a flyout *not* to close the flyout
const FlyoutWithToasts = ({ children = childrenDefault, ...rest }) => {
thompsongl marked this conversation as resolved.
Show resolved Hide resolved
const [isOpen, setIsOpen] = useState(true);

return (
<>
{isOpen ? (
<EuiFlyout
data-test-subj="flyoutSpec"
onClose={() => setIsOpen(false)}
{...rest}
>
{children}
</EuiFlyout>
) : null}
<EuiGlobalToastList
toasts={[
{
title: 'Toast!',
text: 'Yeah toast',
id: 'a',
},
]}
dismissToast={() => {}}
toastLifeTimeMs={10000}
/>
</>
);
};

it('closes the flyout when the overlay mask is clicked', () => {
cy.mount(<Flyout />);
Expand All @@ -86,6 +124,15 @@ describe('EuiFlyout', () => {
});
});

it('does not close the flyout when `outsideClickCloses=false` and the overlay mask is clicked', () => {
cy.mount(<Flyout outsideClickCloses={false} />);
cy.get('.euiOverlayMask')
.realClick()
.then(() => {
expect(cy.get('[data-test-subj="flyoutSpec"]').should('exist'));
});
});

it('does not close the flyout when the overlay mask is only the target of mouseup', () => {
cy.mount(<Flyout />);
cy.get('[data-test-subj="itemD"]').realMouseDown().realMouseMove(-100, 0);
Expand All @@ -95,5 +142,23 @@ describe('EuiFlyout', () => {
expect(cy.get('[data-test-subj="flyoutSpec"]').should('exist'));
});
});

it('does not close the flyout when the toast is clicked when `ownFocus=true`', () => {
cy.mount(<FlyoutWithToasts />);
cy.get('[data-test-subj="toastCloseButton"]')
.realClick()
.then(() => {
expect(cy.get('[data-test-subj="flyoutSpec"]').should('exist'));
});
});

it('closes the flyout when the toast is clicked when `ownFocus=false`', () => {
cy.mount(<FlyoutWithToasts ownFocus={false} outsideClickCloses={true} />);
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
cy.get('[data-test-subj="toastCloseButton"]')
.realClick()
.then(() => {
expect(cy.get('[data-test-subj="flyoutSpec"]').should('not.exist'));
});
});
});
});
4 changes: 2 additions & 2 deletions src/components/flyout/flyout.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import { requiredProps, takeMountedSnapshot } from '../../test';
import { EuiFlyout, SIZES, PADDING_SIZES, SIDES } from './flyout';

jest.mock('../overlay_mask', () => ({
EuiOverlayMask: ({ headerZindexLocation, ...props }: any) => (
<div {...props} />
EuiOverlayMask: ({ headerZindexLocation, maskRef, ...props }: any) => (
<div {...props} ref={maskRef} />
),
}));

Expand Down
42 changes: 28 additions & 14 deletions src/components/flyout/flyout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@

import React, {
useEffect,
useRef,
useState,
forwardRef,
ComponentPropsWithRef,
CSSProperties,
ElementType,
Fragment,
FunctionComponent,
MouseEvent,
MouseEvent as ReactMouseEvent,
MutableRefObject,
} from 'react';
import classnames from 'classnames';
Expand Down Expand Up @@ -81,7 +82,7 @@ export const PADDING_SIZES = keysOf(paddingSizeToClassNameMap);
type _EuiFlyoutPaddingSize = typeof PADDING_SIZES[number];

interface _EuiFlyoutProps {
onClose: () => void;
onClose: (event: MouseEvent | TouchEvent | KeyboardEvent) => void;
/**
* Defines the width of the panel.
* Pass a predefined size of `s | m | l`, or pass any number/string compatible with the CSS `width` attribute
Expand Down Expand Up @@ -204,6 +205,7 @@ export const EuiFlyout = forwardRef(
| null
) => {
const Element = as || defaultElement;
const maskRef = useRef<HTMLDivElement>(null);
/**
* Setting the initial state of pushed based on the `type` prop
* and if the current window size is large enough (larger than `pushMinBreakpoint`)
Expand Down Expand Up @@ -282,7 +284,7 @@ export const EuiFlyout = forwardRef(
const onKeyDown = (event: KeyboardEvent) => {
if (!isPushed && event.key === keys.ESCAPE) {
event.preventDefault();
onClose();
onClose(event);
}
};

Expand Down Expand Up @@ -336,22 +338,30 @@ export const EuiFlyout = forwardRef(
data-test-subj="euiFlyoutCloseButton"
{...closeButtonProps}
className={closeButtonClasses}
onClick={(e: MouseEvent<HTMLButtonElement>) => {
onClose();
closeButtonProps?.onClick && closeButtonProps.onClick(e);
onClick={(e: ReactMouseEvent<HTMLButtonElement>) => {
onClose(e.nativeEvent);
thompsongl marked this conversation as resolved.
Show resolved Hide resolved
closeButtonProps?.onClick?.(e);
}}
/>
)}
</EuiI18n>
);
}

const isDefaultConfiguration = ownFocus && !isPushed;
const onClickOutside =
(isDefaultConfiguration && outsideClickCloses !== false) ||
outsideClickCloses === true
? onClose
: undefined;
const hasOverlayMask = ownFocus && !isPushed;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this name change will be super helpful for understanding

Copy link
Member

Choose a reason for hiding this comment

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

🤩 ++++++ this is great!!

const onClickOutside = (event: MouseEvent | TouchEvent) => {
// Do not close the flyout for any external click
if (outsideClickCloses === false) return undefined;
if (hasOverlayMask) {
// The overlay mask is present, so only clicks on the mask should close the flyout, regardless of outsideClickCloses
if (event.target === maskRef.current) return onClose(event);
} else {
// No overlay mask is present, so any outside clicks should close the flyout
if (outsideClickCloses === true) return onClose(event);
}
// Otherwise if ownFocus is false and outsideClickCloses is undefined, outside clicks should not close the flyout
return undefined;
};
/*
* Trap focus even when `ownFocus={false}`, otherwise closing
* the flyout won't return focus to the originating button.
Expand Down Expand Up @@ -386,9 +396,13 @@ export const EuiFlyout = forwardRef(
);

// If ownFocus is set, wrap with an overlay and allow the user to click it to close it.
if (isDefaultConfiguration) {
const mergedMaskProps = {
...maskProps,
maskRef: useCombinedRefs([maskProps?.maskRef, maskRef]),
};
if (hasOverlayMask) {
flyout = (
<EuiOverlayMask headerZindexLocation="below" {...maskProps}>
<EuiOverlayMask headerZindexLocation="below" {...mergedMaskProps}>
{flyout}
</EuiOverlayMask>
);
Expand Down
4 changes: 2 additions & 2 deletions src/components/focus_trap/focus_trap.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,14 @@ export class EuiFocusTrap extends Component<EuiFocusTrapProps, State> {
document.removeEventListener('touchend', this.onMouseupOutside);
};

handleOutsideClick: ReactFocusOnProps['onClickOutside'] = (...args) => {
handleOutsideClick: ReactFocusOnProps['onClickOutside'] = (event) => {
thompsongl marked this conversation as resolved.
Show resolved Hide resolved
const { onClickOutside, clickOutsideDisables, closeOnMouseup } = this.props;
if (clickOutsideDisables) {
this.setState({ hasBeenDisabledByClick: true });
}

if (onClickOutside) {
closeOnMouseup ? this.addMouseupListener() : onClickOutside(...args);
closeOnMouseup ? this.addMouseupListener() : onClickOutside(event);
}
};

Expand Down
13 changes: 11 additions & 2 deletions src/components/overlay_mask/overlay_mask.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,17 @@
import React, {
FunctionComponent,
HTMLAttributes,
MutableRefObject,
ReactNode,
Ref,
useEffect,
useRef,
useState,
} from 'react';
import { createPortal } from 'react-dom';
import classNames from 'classnames';
import { CommonProps, keysOf } from '../common';
import { useCombinedRefs } from '../../services';

export interface EuiOverlayMaskInterface {
/**
Expand All @@ -36,6 +39,10 @@ export interface EuiOverlayMaskInterface {
* Should the mask visually sit above or below the EuiHeader (controlled by z-index)
*/
headerZindexLocation?: 'above' | 'below';
/**
* React ref to be passed to the wrapping container
*/
maskRef?: Ref<HTMLDivElement> | MutableRefObject<HTMLDivElement>;
}

export type EuiOverlayMaskProps = CommonProps &
Expand All @@ -50,9 +57,11 @@ export const EuiOverlayMask: FunctionComponent<EuiOverlayMaskProps> = ({
children,
onClick,
headerZindexLocation = 'above',
maskRef,
...rest
}) => {
const overlayMaskNode = useRef<HTMLDivElement>();
const combinedMaskRef = useCombinedRefs([overlayMaskNode, maskRef]);
const [isPortalTargetReady, setIsPortalTargetReady] = useState(false);

useEffect(() => {
Expand All @@ -65,9 +74,9 @@ export const EuiOverlayMask: FunctionComponent<EuiOverlayMaskProps> = ({

useEffect(() => {
if (typeof document !== 'undefined') {
overlayMaskNode.current = document.createElement('div');
combinedMaskRef(document.createElement('div'));
}
}, []);
}, []); // eslint-disable-line react-hooks/exhaustive-deps

useEffect(() => {
const portalTarget = overlayMaskNode.current;
Expand Down
6 changes: 6 additions & 0 deletions upcoming_changelogs/5876.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
- Updated `EuiOverlayMask` to accept a React ref

**Bug fixes**

- Fixed `EuiFlyout` `outsideClickCloses` not being scoped to overlay mask when `ownFocus=true`