fix: use shared modal manager to sync container state (#5917)

* fix: use shared modal manager to sync container state * fix types * prettier
react-bootstrap · Jul 15, 2021 · 46eb1bb · 46eb1bb
1 parent 9e9b43a
commit 46eb1bb
Show file tree

Hide file tree

Showing 7 changed files with 103 additions and 57 deletions.
diff --git a/src/BootstrapModalManager.tsx b/src/BootstrapModalManager.tsx
@@ -9,7 +9,7 @@ const Selector = {
   NAVBAR_TOGGLER: '.navbar-toggler',
 };
 
-export default class BootstrapModalManager extends ModalManager {
+class BootstrapModalManager extends ModalManager {
   private adjustAndStore<T extends keyof CSSStyleDeclaration>(
     prop: T,
     element: HTMLElement,
@@ -66,3 +66,11 @@ export default class BootstrapModalManager extends ModalManager {
     );
   }
 }
+
+let sharedManager: BootstrapModalManager | undefined;
+export function getSharedManager() {
+  if (!sharedManager) sharedManager = new BootstrapModalManager();
+  return sharedManager;
+}
+
+export default BootstrapModalManager;
diff --git a/src/Modal.tsx b/src/Modal.tsx
@@ -13,7 +13,8 @@ import PropTypes from 'prop-types';
 import * as React from 'react';
 import { useCallback, useMemo, useRef, useState } from 'react';
 import BaseModal, { BaseModalProps } from 'react-overlays/Modal';
-import BootstrapModalManager from './BootstrapModalManager';
+import { ModalInstance } from 'react-overlays/ModalManager';
+import { getSharedManager } from './BootstrapModalManager';
 import Fade from './Fade';
 import ModalBody from './ModalBody';
 import ModalContext from './ModalContext';
@@ -52,8 +53,6 @@ export interface ModalProps
   [other: string]: any;
 }
 
-let manager;
-
 const propTypes = {
   /**
    * @default 'modal'
@@ -284,9 +283,7 @@ const Modal: BsPrefixRefForwardingComponent<'div', ModalProps> =
       const waitingForMouseUpRef = useRef(false);
       const ignoreBackdropClickRef = useRef(false);
       const removeStaticModalAnimationRef = useRef<(() => void) | null>(null);
-
-      // TODO: what's this type
-      const [modal, setModalRef] = useCallbackRef<{ dialog: any }>();
+      const [modal, setModalRef] = useCallbackRef<ModalInstance>();
       const mergedRef = useMergedRefs(ref, setModalRef);
       const handleHide = useEventCallback(onHide);
 
@@ -301,15 +298,15 @@ const Modal: BsPrefixRefForwardingComponent<'div', ModalProps> =
 
       function getModalManager() {
         if (propsManager) return propsManager;
-        if (!manager) manager = new BootstrapModalManager();
-        return manager;
+        return getSharedManager();
       }
 
       function updateDialogStyle(node) {
         if (!canUseDOM) return;
 
-        const containerIsOverflowing =
-          getModalManager().isContainerOverflowing(modal);
+        const containerIsOverflowing = getModalManager().isContainerOverflowing(
+          modal!,
+        );
 
         const modalIsOverflowing =
           node.scrollHeight > ownerDocument(node).documentElement.clientHeight;
@@ -358,7 +355,7 @@ const Modal: BsPrefixRefForwardingComponent<'div', ModalProps> =
       const handleStaticModalAnimation = () => {
         setAnimateStaticModal(true);
         removeStaticModalAnimationRef.current = transitionEnd(
-          modal!.dialog,
+          modal!.dialog as any,
           () => {
             setAnimateStaticModal(false);
           },

diff --git a/src/Offcanvas.tsx b/src/Offcanvas.tsx
@@ -1,5 +1,4 @@
 import classNames from 'classnames';
-import useCallbackRef from '@restart/hooks/useCallbackRef';
 import useEventCallback from '@restart/hooks/useEventCallback';
 import PropTypes from 'prop-types';
 import * as React from 'react';
@@ -8,8 +7,6 @@ import BaseModal, {
   ModalProps as BaseModalProps,
   ModalHandle,
 } from 'react-overlays/Modal';
-import ModalManager from 'react-overlays/ModalManager';
-import useRootClose from 'react-overlays/useRootClose';
 import Fade from './Fade';
 import OffcanvasBody from './OffcanvasBody';
 import OffcanvasToggling from './OffcanvasToggling';
@@ -18,6 +15,9 @@ import OffcanvasHeader from './OffcanvasHeader';
 import OffcanvasTitle from './OffcanvasTitle';
 import { BsPrefixRefForwardingComponent } from './helpers';
 import { useBootstrapPrefix } from './ThemeProvider';
+import BootstrapModalManager, {
+  getSharedManager,
+} from './BootstrapModalManager';
 
 export type OffcanvasPlacement = 'start' | 'end' | 'top' | 'bottom';
 
@@ -217,18 +217,12 @@ const Offcanvas: BsPrefixRefForwardingComponent<'div', OffcanvasProps> =
       },
       ref,
     ) => {
-      const [dialogElement, setDialogElement] = useCallbackRef<HTMLElement>();
-      const modalManager = useRef<ModalManager>();
+      const modalManager = useRef<BootstrapModalManager>();
       const handleHide = useEventCallback(onHide);
 
       bsPrefix = useBootstrapPrefix(bsPrefix, 'offcanvas');
       const modalBsPrefix = useBootstrapPrefix(undefined, 'modal');
 
-      // If there's a backdrop, let BaseModal handle closing.
-      useRootClose(dialogElement, handleHide, {
-        disabled: backdrop,
-      });
-
       const modalContext = useMemo(
         () => ({
           onHide: handleHide,
@@ -238,23 +232,27 @@ const Offcanvas: BsPrefixRefForwardingComponent<'div', OffcanvasProps> =
 
       function getModalManager() {
         if (propsManager) return propsManager;
-        if (!modalManager.current)
-          modalManager.current = new ModalManager({
-            handleContainerOverflow: !scroll,
-          });
-        return modalManager.current;
+        if (scroll) {
+          // Have to use a different modal manager since the shared
+          // one handles overflow.
+          if (!modalManager.current)
+            modalManager.current = new BootstrapModalManager({
+              handleContainerOverflow: false,
+            });
+          return modalManager.current;
+        }
+
+        return getSharedManager();
       }
 
       const handleEnter = (node, ...args) => {
         if (node) node.style.visibility = 'visible';
         onEnter?.(node, ...args);
-        setDialogElement(node);
       };
 
       const handleExited = (node, ...args) => {
         if (node) node.style.visibility = '';
         onExited?.(...args);
-        setDialogElement(null);
       };
 
       const renderBackdrop = useCallback(
@@ -295,7 +293,7 @@ const Offcanvas: BsPrefixRefForwardingComponent<'div', OffcanvasProps> =
             container={container}
             keyboard={keyboard}
             autoFocus={autoFocus}
-            enforceFocus={enforceFocus}
+            enforceFocus={enforceFocus && !scroll}
             restoreFocus={restoreFocus}
             restoreFocusOptions={restoreFocusOptions}
             onEscapeKeyDown={onEscapeKeyDown}

diff --git a/src/helpers.ts b/src/helpers.ts
@@ -40,8 +40,10 @@ export class BsPrefixComponent<
 > extends React.Component<ReplaceProps<As, BsPrefixProps<As> & P>> {}
 
 // Need to use this instead of typeof Component to get proper type checking.
-export type BsPrefixComponentClass<As extends React.ElementType, P = unknown> =
-  React.ComponentClass<ReplaceProps<As, BsPrefixProps<As> & P>>;
+export type BsPrefixComponentClass<
+  As extends React.ElementType,
+  P = unknown,
+> = React.ComponentClass<ReplaceProps<As, BsPrefixProps<As> & P>>;
 
 export type SelectCallback = (
   eventKey: string | null,

diff --git a/test/ModalSpec.js b/test/ModalSpec.js
@@ -1,5 +1,6 @@
 import { mount } from 'enzyme';
 import * as React from 'react';
+import ModalManager from 'react-overlays/ModalManager';
 import Modal from '../src/Modal';
 
 describe('<Modal>', () => {
@@ -407,4 +408,23 @@ describe('<Modal>', () => {
 
     expect(onEscapeKeyDownSpy).to.not.have.been.called;
   });
+
+  it('Should use custom props manager if specified', (done) => {
+    const noOp = () => {};
+
+    class MyModalManager extends ModalManager {
+      add() {
+        done();
+      }
+    }
+
+    const managerRef = React.createRef();
+    managerRef.current = new MyModalManager();
+
+    mount(
+      <Modal show onHide={noOp} manager={managerRef.current}>
+        <strong>Message</strong>
+      </Modal>,
+    );
+  });
 });
diff --git a/test/OffcanvasSpec.js b/test/OffcanvasSpec.js
@@ -1,6 +1,6 @@
 import { mount } from 'enzyme';
 import * as React from 'react';
-import simulant from 'simulant';
+// import simulant from 'simulant';
 import ModalManager from 'react-overlays/ModalManager';
 import Offcanvas from '../src/Offcanvas';
 
@@ -120,23 +120,25 @@ describe('<Offcanvas>', () => {
     expect(onHideSpy).to.have.been.called;
   });
 
-  it('Should close when anything outside offcanvas clicked and backdrop=false', () => {
-    const onHideSpy = sinon.spy();
-    mount(
-      <>
-        <Offcanvas show onHide={onHideSpy} backdrop={false}>
-          <strong>Message</strong>
-        </Offcanvas>
-        <button type="button" id="mybutton">
-          my button
-        </button>
-      </>,
-    );
-
-    simulant.fire(document.body, 'click');
-
-    expect(onHideSpy).to.have.been.called;
-  });
+  // TODO: unsure if we need this, since it seems like Offcanvas is still undergoing some
+  // changes upstream.
+  // it('Should close when anything outside offcanvas clicked and backdrop=false', () => {
+  //   const onHideSpy = sinon.spy();
+  //   mount(
+  //     <>
+  //       <Offcanvas show onHide={onHideSpy} backdrop={false}>
+  //         <strong>Message</strong>
+  //       </Offcanvas>
+  //       <button type="button" id="mybutton">
+  //         my button
+  //       </button>
+  //     </>,
+  //   );
+
+  //   simulant.fire(document.body, 'click');
+
+  //   expect(onHideSpy).to.have.been.called;
+  // });
 
   it('Should not call onHide if the click target comes from inside the offcanvas', () => {
     const onHideSpy = sinon.spy();
@@ -227,4 +229,18 @@ describe('<Offcanvas>', () => {
       </Offcanvas>,
     );
   });
+
+  it('should not change overflow style when scroll=true', () => {
+    const containerRef = React.createRef();
+    const noOp = () => {};
+    mount(
+      <div ref={containerRef} style={{ height: '2000px', overflow: 'scroll' }}>
+        <Offcanvas show onHide={noOp} container={containerRef} scroll>
+          <strong>Message</strong>
+        </Offcanvas>
+      </div>,
+    );
+
+    expect(containerRef.current.style.overflow).to.equal('scroll');
+  });
 });
diff --git a/www/src/examples/Offcanvas/Backdrop.js b/www/src/examples/Offcanvas/Backdrop.js
@@ -1,14 +1,19 @@
 const options = [
-  {
-    name: 'Enable body scrolling',
-    scroll: true,
-    backdrop: false,
-  },
   {
     name: 'Enable backdrop (default)',
     scroll: false,
     backdrop: true,
   },
+  {
+    name: 'Disable backdrop',
+    scroll: false,
+    backdrop: false,
+  },
+  {
+    name: 'Enable body scrolling',
+    scroll: true,
+    backdrop: false,
+  },
   {
     name: 'Enable both scrolling & backdrop',
     scroll: true,
@@ -20,11 +25,11 @@ function OffCanvasExample({ name, ...props }) {
   const [show, setShow] = useState(false);
 
   const handleClose = () => setShow(false);
-  const handleShow = () => setShow(true);
+  const toggleShow = () => setShow((s) => !s);
 
   return (
     <>
-      <Button variant="primary" onClick={handleShow} className="me-2">
+      <Button variant="primary" onClick={toggleShow} className="me-2">
         {name}
       </Button>
       <Offcanvas show={show} onHide={handleClose} {...props}>