Sitelet https://web.archive.org/web/20210123231909/https://github.com/mui-org/material-ui/issues/24550
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

[Popper] Use ownerDocument of anchorEl #24550

Open
simplecommerce opened this issue Jan 22, 2021 · 1 comment
Open

[Popper] Use ownerDocument of anchorEl #24550

simplecommerce opened this issue Jan 22, 2021 · 1 comment

Comments

@simplecommerce
Copy link

@simplecommerce simplecommerce commented Jan 22, 2021

Hi, I have a question in regards to passing default theme props to a component.
My example is using the TablePagination component.
Underlying it uses the Select which uses the Menu, Popover and Modal.

I am trying to set a default container to the Modal component, but it seems that it only works if I use the Modal directly.

If I use the TablePagination component, I need to do it differently, which seems odd to me.

Here is a sandbox to demonstrate the issue.

https://codesandbox.io/s/material-demo-forked-p06py?file=/demo.js

Current Behavior 😯

In order to pass a container prop to the Modal inside of the Select I have to use

    MuiSelect: {
      MenuProps: {
        // this is required for fullscreen mode to work, it baically binds the tooltip to the fullscreen element if any
        container: () => {
          console.log("Select container");
        }
      }
    }

Expected Behavior 🤔

I was expecting to be able to simply pass the prop to the Modal component by doing:

    MuiModal: {
      container: () => {
        console.log("container");
      }
    },

Steps to Reproduce 🕹

In order to reproduce the issue, do the following.

On the page, simply click on the button to toggle the modal and then click on the paging option in the pagination.

You should see two console logs. container and select container.

Then comment out the MuiSelect props and try to toggle the select in the pagination again, and you should only see container when toggling the modal.

Context 🔦

My use case, was to simply pass a default prop to any component using the Modal as the underlying component which access theme props.

If my understanding of it is wrong please let me know. And sorry if I am being unclear.

Thanks!

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jan 23, 2021 •

@simplecommerce Thanks for reporting this behavior. Yes, it's unfortunate, the Popover has a different default container logic than the Modal. It uses the owner document of the anchor. In your case, I would recommend customizing the default prop of the Popover.


Actually, it looks like the Popper component should do the same. We forgot to port this part of the logic:

function getAnchorEl(anchorEl) {
return typeof anchorEl === 'function' ? anchorEl() : anchorEl;
}

You can reproduce the bug on https://codesandbox.io/s/tender-lumiere-mhk0g?file=/src/App.tsx. I think that we can apply this fix:

diff --git a/packages/material-ui/src/Popper/Popper.js b/packages/material-ui/src/Popper/Popper.js
index efb1aa2042..6f8bea8f80 100644
--- a/packages/material-ui/src/Popper/Popper.js
+++ b/packages/material-ui/src/Popper/Popper.js
@@ -4,6 +4,7 @@ import { createPopper } from '@popperjs/core';
 import { chainPropTypes, refType, HTMLElementType } from '@material-ui/utils';
 import { useTheme } from '@material-ui/styles';
 import Portal from '../Portal';
+import ownerDocument from '../utils/ownerDocument';
 import setRef from '../utils/setRef';
 import useForkRef from '../utils/useForkRef';
 import useEnhancedEffect from '../utils/useEnhancedEffect';
@@ -42,7 +43,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
   const {
     anchorEl,
     children,
-    container,
+    container: containerProp,
     disablePortal = false,
     keepMounted = false,
     modifiers,
@@ -211,6 +212,12 @@ const Popper = React.forwardRef(function Popper(props, ref) {
     };
   }

+  // If the container prop is provided, use that
+  // If the anchorEl prop is provided, use its parent body element as the container
+  // If neither are provided let the Modal take care of choosing the container
+  const container =
+    containerProp || (anchorEl ? ownerDocument(getAnchorEl(anchorEl)).body : undefined);
+
   return (
     <Portal disablePortal={disablePortal} container={container}>
       <div
@oliviertassinari oliviertassinari changed the title [Question]: Modal default theme props [Popper] Use ownerDocument of anchorEl Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants