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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9805148
euiOverlayMask accept ref; mock updates
thompsongl May 5, 2022
559d42c
euiFocusTrap pass event to callback
thompsongl May 5, 2022
2e440ff
euiFlyout take advantage of ref and event data
thompsongl May 5, 2022
ca8f0fe
CL
thompsongl May 5, 2022
6b8df0c
cypress tests
thompsongl May 5, 2022
5d2f52e
use maskRef prop instead of forwardRef
thompsongl May 5, 2022
e635a85
clean up
thompsongl May 5, 2022
0dbcf3d
Merge branch 'main' into flyout-mask
thompsongl May 6, 2022
d017609
outsideClickCloses=false test; comment
thompsongl May 6, 2022
43ce709
refactor onClickOutside; update name to hasOverlayMask
thompsongl May 6, 2022
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
8 changes: 5 additions & 3 deletions src/components/collapsible_nav/collapsible_nav.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,18 @@
* Side Public License, v 1.
*/

import React from 'react';
import React, { forwardRef as mockForwardRef } from 'react';
import { render, mount } from 'enzyme';
import { requiredProps, takeMountedSnapshot } from '../../test';

import { EuiCollapsibleNav } from './collapsible_nav';
import { EuiOverlayMaskProps } from '../overlay_mask';

jest.mock('../overlay_mask', () => ({
EuiOverlayMask: ({ headerZindexLocation, ...props }: any) => (
<div {...props} />
EuiOverlayMask: mockForwardRef(
({ headerZindexLocation, ...props }: any, ref) => (
<div {...props} ref={ref} />
)
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
),
}));

Expand Down
8 changes: 5 additions & 3 deletions src/components/flyout/flyout.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,17 @@
* Side Public License, v 1.
*/

import React from 'react';
import React, { forwardRef as mockForwardRef } from 'react';
import { render, mount } from 'enzyme';
import { requiredProps, takeMountedSnapshot } from '../../test';

import { EuiFlyout, SIZES, PADDING_SIZES, SIDES } from './flyout';

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

Expand Down
28 changes: 18 additions & 10 deletions src/components/flyout/flyout.tsx
View file
Open in desktop
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,9 +338,9 @@ 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);
}}
/>
)}
Expand All @@ -347,10 +349,12 @@ export const EuiFlyout = forwardRef(
}

const isDefaultConfiguration = ownFocus && !isPushed;
const onClickOutside =
(isDefaultConfiguration && outsideClickCloses !== false) ||
const onClickOutside = (event: MouseEvent | TouchEvent) =>
(isDefaultConfiguration &&
outsideClickCloses !== false &&
event.target === maskRef.current) ||
outsideClickCloses === true
? onClose
? onClose(event)
: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This still feels pretty hard for me to read personally 😅 Since this is a function now, can we prefer early returns?

const onClickOutside = (event: MouseEvent | TouchEvent) => {
  if (outsideClickCloses === true) return onClose(event);
  if (outsideClickCloses === false) return undefined;
  // If outsideClickCloses is not specified, only close on clicks to the overlay mask
  if (isDefaultConfiguration && event.target === maskRef) return onClose(event);
  // Otherwise if ownFocus is false, outside clicks should not close the flyout
  return undefined;
}

Copy link
Contributor Author

@thompsongl thompsongl May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'll refactor for readability, but here's the logic:

  • If ownFocus=true (default), only clicks on the mask should close the flyout, even if outsideClickCloses=true
  • If ownFocus=false, the flyout will only close if outsideClickCloses=true, and in that case clicking any external element will close the flyout

Copy link
Contributor

@cee-chen cee-chen May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If ownFocus=true (default), only clicks on the mask should close the flyout, even if outsideClickCloses=true

That's just not what the current branching is doing though?? So you'd need to change this logic if that's the behavior you want. outsideClickCloses === true will always evaluate to returning onClose(). Am I reading this the same way as you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const onClickOutside = (event: MouseEvent | TouchEvent) => {
  // Do not close the flyout for any external click
  if (outsideClickCloses === false) return undefined;
  if (isDefaultConfiguration) {
    // The overlay mask is present, so only clicks targeting the mask should close the flyout
    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, outside clicks should not close the flyout
  return undefined;
};

Copy link
Contributor Author

@thompsongl thompsongl May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That's just not what the current branching is doing though??

Yep I had the conditional wrong. #5876 (comment) is correct; will push the change once we get confirmation from @cchaos

Copy link
Contributor

Choose a reason for hiding this comment

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

@thompsongl Nice! Definitely much easier to grok now, especially w/ comments. For the last comment/return, I might make it a bit more specific to help follow the possible branch paths:

// Otherwise if ownFocus is false and outsideClickCloses is undefined, outside clicks should not close the flyout

Copy link
Contributor

@cee-chen cee-chen May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I also think we should be more specific about the isDefaultConfiguration mask behavior:

// The overlay mask is present, so only clicks on the mask should close the flyout, regardless of outsideClickCloses

It's not that the business logic/UX doesn't make sense, it's just that it's a bit tortuous so more comments should help with that (I think?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh man, I remember how tricky this logic was. I'm going to repeat the logic just to ensure I understand.

  • Default ownFocus = true: The overlay mask renders and the flyout will only be closed when clicking mask (or close button)
  • ownFocus = false && outsideClickCloses = false: No overlay mask and can only be closed via close button (or some other in-page toggle)
  • ownFocus = false && outsideClickCloses = true: Clicking anywhere outside will close flyout (does this include dragging with mouse up or does this now mean a special extra configuration?)

I feel like that's how it was before, but maybe I'm remembering another config wrong?

Copy link
Contributor Author

@thompsongl thompsongl May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ownFocus = false && outsideClickCloses = true: Clicking anywhere outside will close flyout (does this include dragging with mouse up or does this now mean a special extra configuration?)

By default the close event will happen if mousedown happens outside the flyout. Consumers can change this to occur on mouseup if they have some reason to do so.
By default the close event will not happen if mousedown happens inside the flyout, even if mouseup happens outside the flyout (e.g., mousedown inside, drag outside, release. The flyout remains open)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

43ce709 incorporates all suggestions in this thread

/*
* Trap focus even when `ownFocus={false}`, otherwise closing
Expand Down Expand Up @@ -388,7 +392,11 @@ export const EuiFlyout = forwardRef(
// If ownFocus is set, wrap with an overlay and allow the user to click it to close it.
if (isDefaultConfiguration) {
flyout = (
<EuiOverlayMask headerZindexLocation="below" {...maskProps}>
<EuiOverlayMask
headerZindexLocation="below"
{...maskProps}
ref={maskRef}
>
{flyout}
</EuiOverlayMask>
);
Expand Down
6 changes: 3 additions & 3 deletions src/components/focus_trap/focus_trap.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export class EuiFocusTrap extends Component<EuiFocusTrapProps, State> {
node.setAttribute('data-autofocus', 'true');
};

onMouseupOutside = (e: MouseEvent | TouchEvent) => {
onMouseupOutside = (e: MouseEvent | TouchEvent) => () => {
this.removeMouseupListener();
// Timeout gives precedence to the consumer to initiate close if it has toggle behavior.
// Otherwise this event may occur first and the consumer toggle will reopen the flyout.
Expand All @@ -98,14 +98,14 @@ export class EuiFocusTrap extends Component<EuiFocusTrapProps, State> {
document.removeEventListener('touchend', this.onMouseupOutside);
};

handleOutsideClick: ReactFocusOnProps['onClickOutside'] = (...args) => {
handleOutsideClick: ReactFocusOnProps['onClickOutside'] = (e) => {
const { onClickOutside, clickOutsideDisables, closeOnMouseup } = this.props;
if (clickOutsideDisables) {
this.setState({ hasBeenDisabledByClick: true });
}

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

Expand Down
162 changes: 82 additions & 80 deletions src/components/overlay_mask/overlay_mask.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,17 @@
*/

import React, {
FunctionComponent,
HTMLAttributes,
ReactNode,
forwardRef,
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 @@ -45,86 +46,87 @@ export type EuiOverlayMaskProps = CommonProps &
> &
EuiOverlayMaskInterface;

export const EuiOverlayMask: FunctionComponent<EuiOverlayMaskProps> = ({
className,
children,
onClick,
headerZindexLocation = 'above',
...rest
}) => {
const overlayMaskNode = useRef<HTMLDivElement>();
const [isPortalTargetReady, setIsPortalTargetReady] = useState(false);

useEffect(() => {
document.body.classList.add('euiBody-hasOverlayMask');

return () => {
document.body.classList.remove('euiBody-hasOverlayMask');
};
}, []);

useEffect(() => {
if (typeof document !== 'undefined') {
overlayMaskNode.current = document.createElement('div');
}
}, []);

useEffect(() => {
const portalTarget = overlayMaskNode.current;

if (portalTarget) {
document.body.appendChild(portalTarget);
}
export const EuiOverlayMask = forwardRef<HTMLDivElement, EuiOverlayMaskProps>(
(
{ className, children, onClick, headerZindexLocation = 'above', ...rest },
ref
) => {
const overlayMaskNode = useRef<HTMLDivElement>();
const combinedMaskRef = useCombinedRefs([overlayMaskNode, ref]);
const [isPortalTargetReady, setIsPortalTargetReady] = useState(false);

useEffect(() => {
document.body.classList.add('euiBody-hasOverlayMask');

return () => {
document.body.classList.remove('euiBody-hasOverlayMask');
};
}, []);

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

setIsPortalTargetReady(true);
useEffect(() => {
const portalTarget = overlayMaskNode.current;

return () => {
if (portalTarget) {
document.body.removeChild(portalTarget);
}
};
}, []);

useEffect(() => {
if (!overlayMaskNode.current) return;
keysOf(rest).forEach((key) => {
if (typeof rest[key] !== 'string') {
throw new Error(
`Unhandled property type. EuiOverlayMask property ${key} is not a string.`
);
}
if (overlayMaskNode.current) {
overlayMaskNode.current.setAttribute(key, rest[key]!);
document.body.appendChild(portalTarget);
}
});
}, []); // eslint-disable-line react-hooks/exhaustive-deps

useEffect(() => {
if (!overlayMaskNode.current) return;
overlayMaskNode.current.className = classNames(
'euiOverlayMask',
`euiOverlayMask--${headerZindexLocation}Header`,
className
);
}, [className, headerZindexLocation]);

useEffect(() => {
const portalTarget = overlayMaskNode.current;
if (!portalTarget || !onClick) return;

const listener = (e: Event) => {
if (e.target === portalTarget) {
onClick();
}
};
portalTarget.addEventListener('click', listener);

return () => {
portalTarget.removeEventListener('click', listener);
};
}, [onClick]);

return isPortalTargetReady ? (
<>{createPortal(children, overlayMaskNode.current!)}</>
) : null;
};

setIsPortalTargetReady(true);

return () => {
if (portalTarget) {
document.body.removeChild(portalTarget);
}
};
}, []);

useEffect(() => {
if (!overlayMaskNode.current) return;
keysOf(rest).forEach((key) => {
if (typeof rest[key] !== 'string') {
throw new Error(
`Unhandled property type. EuiOverlayMask property ${key} is not a string.`
);
}
if (overlayMaskNode.current) {
overlayMaskNode.current.setAttribute(key, rest[key]!);
}
});
}, []); // eslint-disable-line react-hooks/exhaustive-deps

useEffect(() => {
if (!overlayMaskNode.current) return;
overlayMaskNode.current.className = classNames(
'euiOverlayMask',
`euiOverlayMask--${headerZindexLocation}Header`,
className
);
}, [className, headerZindexLocation]);

useEffect(() => {
const portalTarget = overlayMaskNode.current;
if (!portalTarget || !onClick) return;

const listener = (e: Event) => {
if (e.target === portalTarget) {
onClick();
}
};
portalTarget.addEventListener('click', listener);

return () => {
portalTarget.removeEventListener('click', listener);
};
}, [onClick]);

return isPortalTargetReady ? (
<>{createPortal(children, overlayMaskNode.current!)}</>
) : null;
}
);
EuiOverlayMask.displayName = 'EuiOverlayMask';