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

Re-implement Modal component using HTMLDialogElement (#461) #544

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
119 changes: 76 additions & 43 deletions src/components/Modal/Modal.jsx
Copy link
Member

@adamkudrna adamkudrna Jun 17, 2024

Choose a reason for hiding this comment

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

obrazek

I'd expect more deletions than additions. 😞 Can we get rid of some more features custom code in our focus management? Or anywhere else?

Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
import PropTypes from 'prop-types';
import React, { useRef } from 'react';
import React, {
useCallback,
useEffect,
useRef,
} from 'react';
import { createPortal } from 'react-dom';
import { withGlobalProps } from '../../provider';
import { transferProps } from '../_helpers/transferProps';
import { classNames } from '../../utils/classNames';
import { dialogOnCancelHandler } from './_helpers/dialogOnCancelHandler';
import { dialogOnClickHandler } from './_helpers/dialogOnClickHandler';
import { dialogOnCloseHandler } from './_helpers/dialogOnCloseHandler';
import { dialogOnKeyDownHandler } from './_helpers/dialogOnKeyDownHandler';
import { getPositionClassName } from './_helpers/getPositionClassName';
import { getSizeClassName } from './_helpers/getSizeClassName';
import { useModalFocus } from './_hooks/useModalFocus';
Expand All @@ -12,41 +20,30 @@ import styles from './Modal.module.scss';

const preRender = (
children,
childrenWrapperRef,
closeButtonRef,
dialogRef,
position,
restProps,
size,
events,
restProps,
) => (
<div
className={styles.backdrop}
onClick={(e) => {
e.preventDefault();
if (closeButtonRef?.current != null) {
closeButtonRef.current.click();
}
}}
role="presentation"
<dialog
{...transferProps(restProps)}
{...transferProps(events)}
className={classNames(
styles.root,
getSizeClassName(size, styles),
getPositionClassName(position, styles),
)}
ref={dialogRef}
>
<div
{...transferProps(restProps)}
className={classNames(
styles.root,
getSizeClassName(size, styles),
getPositionClassName(position, styles),
)}
onClick={(e) => {
e.stopPropagation();
}}
ref={childrenWrapperRef}
role="presentation"
>
{children}
</div>
</div>
{children}
</dialog>
);

export const Modal = ({
allowCloseOnBackdropClick,
allowCloseOnEscapeKey,
allowPrimaryActionOnEnterKey,
autoFocus,
children,
closeButtonRef,
Expand All @@ -57,42 +54,66 @@ export const Modal = ({
size,
...restProps
}) => {
const childrenWrapperRef = useRef();
const dialogRef = useRef();

useModalFocus(
autoFocus,
childrenWrapperRef,
primaryButtonRef,
closeButtonRef,
);
useEffect(() => {
dialogRef.current.showModal();
}, []);

useModalFocus(allowPrimaryActionOnEnterKey, autoFocus, dialogRef, primaryButtonRef);
useModalScrollPrevention(preventScrollUnderneath);

const onCancel = useCallback(
(e) => dialogOnCancelHandler(e, closeButtonRef),
[closeButtonRef],
);
const onClick = useCallback(
(e) => dialogOnClickHandler(e, closeButtonRef, dialogRef, allowCloseOnBackdropClick),
[allowCloseOnBackdropClick, closeButtonRef, dialogRef],
);
const onClose = useCallback(
(e) => dialogOnCloseHandler(e, closeButtonRef),
[closeButtonRef],
);
const onKeyDown = useCallback(
(e) => dialogOnKeyDownHandler(e, closeButtonRef, allowCloseOnEscapeKey),
[allowCloseOnEscapeKey, closeButtonRef],
);
const events = {
onCancel,
onClick,
onClose,
onKeyDown,
};

if (portalId === null) {
return preRender(
children,
childrenWrapperRef,
closeButtonRef,
dialogRef,
position,
restProps,
size,
events,
restProps,
);
}

return createPortal(
preRender(
children,
childrenWrapperRef,
closeButtonRef,
dialogRef,
position,
restProps,
size,
events,
restProps,
),
document.getElementById(portalId),
);
};

Modal.defaultProps = {
allowCloseOnBackdropClick: true,
allowCloseOnEscapeKey: true,
allowPrimaryActionOnEnterKey: true,
autoFocus: true,
children: null,
closeButtonRef: null,
Expand All @@ -104,6 +125,18 @@ Modal.defaultProps = {
};

Modal.propTypes = {
/**
* If `true`, the `Modal` can be closed by clicking on the backdrop.
*/
allowCloseOnBackdropClick: PropTypes.bool,
/**
* If `true`, the `Modal` can be closed by pressing the Escape key.
*/
allowCloseOnEscapeKey: PropTypes.bool,
/**
* If `true`, the `Modal` can be submitted by pressing the Enter key.
*/
allowPrimaryActionOnEnterKey: PropTypes.bool,
/**
* If `true`, focus the first input element in the `Modal`, or primary button (referenced by the `primaryButtonRef`
* prop), or other focusable element when the `Modal` is opened. If there are none or `autoFocus` is set to `false`,
Expand All @@ -121,7 +154,7 @@ Modal.propTypes = {
*/
children: PropTypes.node,
/**
* Reference to close button element. It is used to close modal when Escape key is pressed or the backdrop is clicked.
* Reference to close button element. It is used to close modal when Escape key is pressed.
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
*/
closeButtonRef: PropTypes.shape({
// eslint-disable-next-line react/forbid-prop-types
Expand Down
33 changes: 16 additions & 17 deletions src/components/Modal/Modal.module.scss
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
// 1. Modal uses <dialog> element that uses the browser's built-in dialog functionality, so that:
// * visibility of the .root element and its backdrop is managed by the browser
// * positioning of the .root element and its backdrop is managed by the browser
// * z-index of the .root element and its backdrop is not needed as dialog is rendered in browser's Top layer

@use "sass:map";
@use "../../styles/theme/typography";
@use "../../styles/tools/accessibility";
@use "../../styles/tools/breakpoint";
@use "../../styles/tools/reset";
@use "../../styles/tools/spacing";
@use "animations";
@use "settings";
@use "theme";

Expand All @@ -13,33 +19,30 @@
--rui-local-max-width: calc(100% - (2 * var(--rui-local-outer-spacing)));
--rui-local-max-height: calc(100% - (2 * var(--rui-local-outer-spacing)));

position: fixed;
left: 50%;
z-index: settings.$z-index;
display: flex;
flex-direction: column;
max-width: var(--rui-local-max-width);
max-height: var(--rui-local-max-height);
padding: 0;
overflow-y: auto;
overscroll-behavior: contain;
border-width: 0;
border-radius: settings.$border-radius;
background: theme.$background;
box-shadow: theme.$box-shadow;
transform: translateX(-50%);

@include breakpoint.up(sm) {
--rui-local-outer-spacing: #{theme.$outer-spacing-sm};
}
}

.backdrop {
position: fixed;
top: 0;
left: 0;
z-index: settings.$backdrop-z-index;
width: 100vw;
height: 100vh;
.root[open] {
display: flex;
animation: fade-in theme.$animation-duration ease-out;
}

.root[open]::backdrop {
background: theme.$backdrop-background;
animation: inherit;
}

.isRootSizeSmall {
Expand Down Expand Up @@ -69,12 +72,8 @@
max-width: min(var(--rui-local-max-width), #{map.get(theme.$sizes, auto, max-width)});
}

.isRootPositionCenter {
top: 50%;
transform: translate(-50%, -50%);
}

.isRootPositionTop {
top: var(--rui-local-outer-spacing);
bottom: auto;
}
}
44 changes: 23 additions & 21 deletions src/components/Modal/README.md
Copy link
Member

Choose a reason for hiding this comment

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

I checked the "Launch modal as form" example and found out there is no <form> element in it. Is it purposely? Maybe we didn't want the <form> there before switching to <dialog> but it would add more semantic meaning (and maybe functionality) now.

There is the method="dialog" form attribute (or formmethod="dialog" for buttons) if we find it useful.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#usage_notes

Original file line number Diff line number Diff line change
Expand Up @@ -1065,27 +1065,29 @@ accessibility.

## Theming

| Custom Property | Description |
|------------------------------------------------------|---------------------------------------------------------------|
| `--rui-Modal__padding-x` | Inline padding of individual modal components |
| `--rui-Modal__padding-y` | Block padding of individual modal components |
| `--rui-Modal__background` | Modal background (including `url()` or gradient) |
| `--rui-Modal__box-shadow` | Modal box shadow |
| `--rui-Modal__separator__width` | Width of separator between modal header, body, and footer |
| `--rui-Modal__separator__color` | Color of separator between modal header, body, and footer |
| `--rui-Modal__outer-spacing-xs` | Spacing around modal, `xs` screen size |
| `--rui-Modal__outer-spacing-sm` | Spacing around modal, `sm` screen size and bigger |
| `--rui-Modal__header__gap` | Modal header gap between children |
| `--rui-Modal__footer__background` | Modal footer background (including `url()` or gradient) |
| `--rui-Modal__footer__gap` | Modal footer gap between children |
| `--rui-Modal__backdrop__background` | Modal backdrop background (including `url()` or gradient) |
| `--rui-Modal--auto__min-width` | Min width of auto-sized modal (when enough screen estate) |
| `--rui-Modal--auto__max-width` | Max width of auto-sized modal (when enough screen estate) |
| `--rui-Modal--small__width` | Width of small modal |
| `--rui-Modal--medium__width` | Width of medium modal |
| `--rui-Modal--large__width` | Width of large modal |
| `--rui-Modal--fullscreen__width` | Width of fullscreen modal |
| `--rui-Modal--fullscreen__height` | Height of fullscreen modal |
| Custom Property | Description |
|------------------------------------------------------|-------------------------------------------------------------|
| `--rui-Modal__padding-x` | Inline padding of individual modal components |
| `--rui-Modal__padding-y` | Block padding of individual modal components |
| `--rui-Modal__background` | Modal background (including `url()` or gradient) |
| `--rui-Modal__box-shadow` | Modal box shadow |
| `--rui-Modal__separator__width` | Width of separator between modal header, body, and footer |
| `--rui-Modal__separator__color` | Color of separator between modal header, body, and footer |
| `--rui-Modal__outer-spacing-xs` | Spacing around modal, `xs` screen size |
| `--rui-Modal__outer-spacing-sm` | Spacing around modal, `sm` screen size and bigger |
| `--rui-Modal__header__gap` | Modal header gap between children |
| `--rui-Modal__footer__background` | Modal footer background (including `url()` or gradient) |
| `--rui-Modal__footer__gap` | Modal footer gap between children |
| `--rui-Modal__backdrop__background` | Modal backdrop background (including `url()` or gradient) |
| `--rui-Modal--auto__min-width` | Min width of auto-sized modal (when enough screen estate) |
| `--rui-Modal--auto__max-width` | Max width of auto-sized modal (when enough screen estate) |
| `--rui-Modal--small__width` | Width of small modal |
| `--rui-Modal--medium__width` | Width of medium modal |
| `--rui-Modal--large__width` | Width of large modal |
| `--rui-Modal--fullscreen__width` | Width of fullscreen modal |
| `--rui-Modal--fullscreen__height` | Height of fullscreen modal |
| `--rui-Modal__animation__duration` | Duration of animation used (when opening modal) |
Copy link
Member

Choose a reason for hiding this comment

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

when opening modal

Currently, closing of Modal is not animated because the component is simply removed from the DOM.

@mbohal Do we also want animated closing? That would mean delayed removal from the DOM.

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 would like to add one thing. Closing animation is hard to impelement. We do not have close handler, we have closeButtonRef. I tried to implement it somehow today but without success. So it is up to our decision whether we want to spend some time on it or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

After a fruitless afternoon I concur with @bedrich-schindler that:

Closing animation is hard to implement.

However, this change is not BC in terms of usage, so I think we should merge it as is.

We can add hide animation int he future. This will very likely change the API though…

Copy link
Contributor

Choose a reason for hiding this comment

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

On video call agreed that for the time being we will only support opening animation.



[button-attributes]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attributes
[div-attributes]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/div#attributes
Expand Down
7 changes: 5 additions & 2 deletions src/components/Modal/__tests__/Modal.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ import { ModalContent } from '../ModalContent';
import { ModalFooter } from '../ModalFooter';
import { ModalHeader } from '../ModalHeader';

describe('rendering', () => {
// Test suites skipped due to missing implementation of HTMLDialogElement in jsdom
// See https://github.com/jsdom/jsdom/issues/3294
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved

describe.skip('rendering', () => {
it('renders with "portalId" props', () => {
document.body.innerHTML = '<div id="portal-id" />';
render((
Expand Down Expand Up @@ -74,7 +77,7 @@ describe('rendering', () => {
});
});

describe('functionality', () => {
describe.skip('functionality', () => {
it.each([
() => userEvent.keyboard('{Escape}'),
() => userEvent.click(screen.getByTestId('id').parentNode),
Expand Down
9 changes: 9 additions & 0 deletions src/components/Modal/_animations.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
@keyframes fade-in {
0% {
opacity: 0;
}

100% {
opacity: 1;
}
}
12 changes: 12 additions & 0 deletions src/components/Modal/_helpers/dialogOnCancelHandler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export const dialogOnCancelHandler = (e, closeButtonRef) => {
// Prevent the default behaviour of the event as we want to close dialog manually.
e.preventDefault();

// If the close button is not disabled, close the modal.
if (
closeButtonRef?.current != null
&& closeButtonRef?.current?.disabled === false
) {
closeButtonRef.current.click();
}
};
32 changes: 32 additions & 0 deletions src/components/Modal/_helpers/dialogOnClickHandler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
export const dialogOnClickHandler = (
e,
closeButtonRef,
dialogRef,
allowCloseOnBackdropClick,
) => {
// If it is not allowed to close modal on backdrop click, do nothing.
if (!allowCloseOnBackdropClick) {
return;
}

// Detection of the click on the backdrop is based on the following conditions:
// 1. The click target is the dialog itself. This prevents detection of clicks on the dialog's children.
// 2. The click is outside the dialog's boundaries.
const dialogRect = dialogRef.current.getBoundingClientRect();
const isClickedOnBackdrop = dialogRef.current === e.target && (
e.clientX < dialogRect.left
|| e.clientX > dialogRect.right
|| e.clientY < dialogRect.top
|| e.clientY > dialogRect.bottom
);

// If user does not click on the backdrop, do nothing.
if (!isClickedOnBackdrop) {
return;
}

// If the close button is not disabled, close the modal.
if (closeButtonRef?.current != null && closeButtonRef?.current?.disabled === false) {
closeButtonRef.current.click();
}
};
9 changes: 9 additions & 0 deletions src/components/Modal/_helpers/dialogOnCloseHandler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export const dialogOnCloseHandler = (e, closeButtonRef) => {
// Prevent the default behaviour of the event as we want to close dialog manually.
e.preventDefault();

// If the close button is not disabled, close the modal.
if (closeButtonRef?.current != null && closeButtonRef?.current?.disabled === false) {
closeButtonRef.current.click();
}
};
Loading