mdbootstrap / mdb-react-ui-kit

React 18 & Bootstrap 5 & Material Design 2.0 UI KIT
https://mdbootstrap.com/docs/b5/react/
Other
1.41k stars 264 forks source link

MDBModalDialog container should be placed in same container as modal's backdrop #161

Closed swayok closed 3 years ago

swayok commented 3 years ago

Currently MDBModalDialog placed inside container where component used while backdrop placed in document.body This sometimes causes modal content be shown behind backdrop and be inaccessible.

Backdrop and modal contents should be placed inside same container to avoid this. Bootstrap places everything in document.body (at least in v3 and v4). I think MDB should work the same way to avoid described issue. Or at least place backdrop alongside modal contents.

Note: z-indexes are correct but in some case the does not work as intended. I failed to find out the reason why some of the modals bugged while others are ok. Maybe it is related to situation when modal placed inside 'scrollable' container.

Current workaround is:

ReactDOM.createPortal(<MDBModal...>....</MDBModal>, document.body);

It seems to be working correctly in any situation.

PS: Cannot post issue on https://mdbootstrap.com/support-ask-question/ - it is down with 502 error =(

swayok commented 3 years ago

Here is a refactored Modla component with some extra functionality. Maybe you can use some of my ideas to improve MDBModal. I've tested it in my project and it seems to be working fine in most cases. Changes:

  1. getOpenState replaced by 4 separate callbacks: onClose - called immediately after user requested it (before any animations) onClosed - called after animation ended onOpen - called immediately after user requested it (before any animations) onOpened - called after animation ended This way developer can handle complex situations granually. This way it is easy to focus an input in modal after it opens or reset form inside modal after animation ends. Also it makes code cleaner unlike getOpenState usage.
  2. I've refactored callbacks inside component so that there are less place for unexpected bugs due to callbacks outdating because of props changes. There are no much use for useCallback here when some callbacks can be moved inside useEffect().
  3. Prop closeOnEsc was not used in handleKeydown so I fixed this.
  4. Modal now always rendered inside document.body to prevent accessibility issues like described above.
  5. show prop can now be null. When it is null - modal will not be rendered at all. This prevents document.body pollution and allows usage of lots of modals without any risk of crashing browser or insane memory usage. Perfect usage of nullable show:
    state: State = {
    isFilesModalVisible: null
    };
    <Modal
    show={this.state.isFilesModalVisible}
    staticBackdrop={false}
    closeOnEsc={true}
    onClose={() => {
         // initiate hiding animation
         this.setState({
            isFilesModalVisible: false
        });
    }
    onClosed={() => {
        // remove modal from document.body
        this.setState({
            isFilesModalVisible: null
        });
    }}
    >
    {contents}
    </Modal>

    This way modal will be rendered only when opened (open and close animation works correctly)

Component code:

import React, {useCallback, useEffect, useRef, useState} from 'react';
import clsx from 'clsx';
import ReactDOM from 'react-dom';

type ModalProps = {
    animationDirection?: string;
    backdrop?: boolean;
    className?: string;
    closeOnEsc?: boolean;
    show?: boolean | null;
    staticBackdrop?: boolean;
    modalRef?: React.RefObject<HTMLDivElement>;
    onClose?: () => void;
    onClosed?: () => void;
    onShow?: () => void;
    onShown?: () => void;
    children: React.ReactNode;
    [rest: string]: any;
}

export default function Modal(props: ModalProps) {
    const {
        animationDirection,
        backdrop,
        children,
        className,
        onClose,
        onClosed,
        onShown,
        onShow,
        modalRef,
        show,
        staticBackdrop,
        closeOnEsc,
        ...otherProps
    } = props;

    const [isOpenBackdrop, setIsOpenBackrop] = useState(show);
    const [isOpenModal, setIsOpenModal] = useState(show);
    const [innerShow, setInnerShow] = useState(show);
    const [staticModal, setStaticModal] = useState(false);

    const modalInnerRef = useRef<HTMLDivElement>(null);
    const modalReference = modalRef ? modalRef : modalInnerRef;

    const classes = clsx(
        'modal',
        staticModal && 'modal-static',
        animationDirection,
        'fade',
        isOpenModal && 'show',
        className
    );
    const backdropClasses = clsx('modal-backdrop', 'fade', isOpenBackdrop && 'show');

    const closeModal = useCallback(() => {
        setIsOpenModal(false);
        onClose && onClose();

        setTimeout(() => {
            setIsOpenBackrop(false);
        }, 150);
        setTimeout(() => {
            setInnerShow(false);
            onClosed && onClosed();
        }, 350);
    }, [onClose, onClosed]);

    useEffect(() => {
        const getScrollbarWidth = () => {
            const documentWidth = document.documentElement.clientWidth;
            return Math.abs(window.innerWidth - documentWidth);
        };

        const hasVScroll = window.innerWidth > document.documentElement.clientWidth && window.innerWidth >= 576;

        if (innerShow && hasVScroll) {
            const scrollbarWidth = getScrollbarWidth();
            document.body.classList.add('modal-open');
            document.body.style.overflow = 'hidden';
            document.body.style.paddingRight = `${scrollbarWidth}px`;
        } else {
            document.body.classList.remove('modal-open');
            document.body.style.overflow = '';
            document.body.style.paddingRight = '';
        }
    }, [innerShow]);

    useEffect(() => {
        if (show) {
            setInnerShow(true);
            setTimeout(() => {
                setIsOpenBackrop(true);
                onShow && onShow();
            }, 0);
            setTimeout(() => {
                setIsOpenModal(true);
                onShown && onShown();
            }, 150);
        } else if (show === false) {
            closeModal();
        }
    }, [show, onShown, onShow]);

    useEffect(() => {
        const handleClickOutside = (event: any) => {
            if (isOpenModal && event.target === modalReference.current) {
                if (!staticBackdrop) {
                    closeModal();
                } else {
                    setStaticModal(true);
                    setTimeout(() => {
                        setStaticModal(false);
                    }, 300);
                }
            }
        };
        window.addEventListener('click', handleClickOutside);

        const handleKeydown = (event: KeyboardEvent) => {
            if (isOpenModal && event.key === 'Escape') {
                if (closeOnEsc) {
                    closeModal();
                } else {
                    setStaticModal(true);
                    setTimeout(() => {
                        setStaticModal(false);
                    }, 300);
                }
            }
        };
        window.addEventListener('keydown', handleKeydown);

        return () => {
            window.removeEventListener('click', handleClickOutside);
            window.removeEventListener('keydown', handleKeydown);
        };
    }, [closeOnEsc, staticBackdrop, isOpenModal, closeModal]);

    if (show === null) {
        return null;
    }

    return (
        <>
            {ReactDOM.createPortal(
                (
                    <div
                        className={classes}
                        ref={modalReference}
                        style={{ display: innerShow || show ? 'block' : 'none' }}
                        tabIndex={-1}
                        {...otherProps}
                    >
                        {children}
                    </div>
                ),
                document.body
            )}
            {ReactDOM.createPortal(backdrop && innerShow && <div className={backdropClasses}/>, document.body)}
        </>
    );

}
Modal.defaultProps = { backdrop: true } as Partial<ModalProps>;
Wojstan commented 3 years ago

Thanks for suggestions. We have already thought about changing the way modal is rendered, but everything needs to be consulted with other MDB technologies. In the next release we are going to make some changes in MDBModal component.