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

[Modal] DisablePortal and Open=True SSR Issues #16811

Closed
2 tasks done
lmuller18 opened this issue Jul 30, 2019 · 3 comments · Fixed by #16850
Closed
2 tasks done

[Modal] DisablePortal and Open=True SSR Issues #16811

lmuller18 opened this issue Jul 30, 2019 · 3 comments · Fixed by #16850
Labels
bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@lmuller18
Copy link
Contributor

In a clean Gatsby project with only Material-UI installed, creating a dialog as such:

<div ref={modalRef}>
  <Dialog open keepMounted disablePortal container={modalRef}>
       This is dialog content. Portal is disabled, default opened, provided
       container.
  </Dialog>
</div>

gatsby build will fail since neither node or document exist in Modal.js:116:25.

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Dialog is present in the DOM at compile time using gatsby build.

Current Behavior 😯

Build process fails:

 1 | function ownerDocument(node) {
> 2 |   return node && node.ownerDocument || document;
    | ^
  3 | }
  4 | 
  5 | export default ownerDocument;


  WebpackError: ReferenceError: document is not defined
  
  - ownerDocument.js:2 ownerDocument
    node_modules/@material-ui/core/esm/utils/ownerDocument.js:2:1
  
  - Modal.js:116 getDoc
    node_modules/@material-ui/core/esm/Modal/Modal.js:116:25
  
  - TrapFocus.js:41 
    node_modules/@material-ui/core/esm/Modal/TrapFocus.js:41:1
  
  - TrapFocus.js:36 TrapFocus
    node_modules/@material-ui/core/esm/Modal/TrapFocus.js:36:8

Steps to Reproduce 🕹

Link: https://codesandbox.io/embed/gatsby-starter-default-hnfqq

  1. Initalize new Gatsby project.
  2. Add @material-ui/core @material-ui/styles gatsby-plugin-material-ui
  3. Create Dialog with props: open disablePortal
  4. yarn build

Context 🔦

I am trying to render a pre-opened dialog using Gatsby so that when the project is compiled, the dialog and its content is present in the compiled HTML ahead of time instead of dynamically rendered through JS.

Your Environment 🌎

Tech Version
Material-UI v^4.3.0
React v^16.8.6
Browser Any
gatsby v^2.13.42
gatsby-plugin-material-ui ^2.1.4
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. labels Jul 30, 2019
@oliviertassinari
Copy link
Member

It seems that we had a couple of regressions since #14285. It has been broken for 2 months, probably the sign that it's not very important.

I would propose the following changes:

diff --git a/packages/material-ui/src/Modal/Modal.js b/packages/material-ui/src/Modal/Modal.js
index 98d22e672..a8638c489 100644
--- a/packages/material-ui/src/Modal/Modal.js
+++ b/packages/material-ui/src/Modal/Modal.js
@@ -112,6 +112,11 @@ const Modal = React.forwardRef(function Modal(props, ref) {
     }
   });

+  const isTopModal = React.useCallback(
+    () => manager.isTopModal(getModal(modal, mountNodeRef, modalRef)),
+    [manager],
+  );
+
   const handlePortalRef = useEventCallback(node => {
     mountNodeRef.current = node;

@@ -123,7 +128,7 @@ const Modal = React.forwardRef(function Modal(props, ref) {
       onRendered();
     }

-    if (open) {
+    if (open && isTopModal()) {
       handleMounted();
     } else {
       ariaHidden(modalRef.current, true);
@@ -148,11 +153,6 @@ const Modal = React.forwardRef(function Modal(props, ref) {
     }
   }, [open, handleClose, hasTransition, closeAfterTransition, handleOpen]);

-  const isTopModal = React.useCallback(
-    () => manager.isTopModal(getModal(modal, mountNodeRef, modalRef)),
-    [manager],
-  );
-
   if (!keepMounted && !open && (!hasTransition || exited)) {
     return null;
   }
diff --git a/packages/material-ui/src/Modal/TrapFocus.js b/packages/material-ui/src/Modal/TrapFocus.js
index 45ccc8ee8..9cb863a6f 100644
--- a/packages/material-ui/src/Modal/TrapFocus.js
+++ b/packages/material-ui/src/Modal/TrapFocus.js
@@ -36,7 +36,7 @@ function TrapFocus(props) {
   // ⚠️ You may rely on React.useMemo as a performance optimization, not as a semantic guarantee.
   // https://reactjs.org/docs/hooks-reference.html#usememo
   React.useMemo(() => {
-    if (!open) {
+    if (!open || typeof window === 'undefined') {
       return;
     }

Also, I think that a test case with:

import Modal from '@material-ui/core/Modal';

export default function App() {
  return (
    <Modal open disablePortal>
      <div>Server</div>
    </Modal>
  );
}

would help.

@lmuller18 Do you want to work on it? :)

@lmuller18
Copy link
Contributor Author

I can start looking into implementing those changes tomorrow. Thank you for the pointers.

@oliviertassinari
Copy link
Member

Awesome, if you need any help, you can join me on Gitter.

@oliviertassinari oliviertassinari changed the title Dialog - DisablePortal and Open=True SSR Issues [Modal] DisablePortal and Open=True SSR Issues Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants