Material-ui: Menu not positioning correctly in popout window using AnchorEl

[zekehernandez]

I have a button that opens a menu, and the button is the AnchorEl of the menu. Which normally works fine, but if the button and menu are in a popout window, the menu is no longer positioned correctly.

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The Menu appears far below the button that is its AnchorEl.

Expected Behavior 🤔

The Menu should appear just below the button that is its AnchorEl.

Steps to Reproduce 🕹

https://codesandbox.io/s/308nkoz2xp

Steps:

1. Click the Open Popout button
2. Click the Open Menu button in the popout window

Context 🔦

I just upgraded to Material-UI v4, and I used to have a button in a popout window that would open a menu and it would be positioned correctly. But after upgrading, it no longer does. I searched the issues here, and came across this comment: https://github.com/mui-org/material-ui/issues/13625#issuecomment-493608458

I saw that the same issue was happening on the linked CodeSandbox.

In my debugging, it seemed as if the issue was on line 175 in Popover.js

var anchorElement = resolvedAnchorEl instanceof Element ? resolvedAnchorEl : (0, _ownerDocument.default)(paperRef.current).body;

In my case, resolvedAnchorEl was an HtmlButtonElement, which should be an instanceOf Element, so I would have thought that anchorElement would have been set to resolvedAnchorEl, but instead it is being set to the body.

I thought this was particularly my problem, not Material-UI's, but after I saw the discussion I linked above, I realized it was not unique to me.

Your Environment 🌎

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.2.0 |
| React | 16.8.6 |
| Browser | Chrome |
| TypeScript | None |
| etc. | |

[ryancogswell]

it seemed as if the issue was on line 175 in Popover.js

Yes, the instanceof check will not work correctly across windows (search for "Cross-window Issues of instanceof" here).

Here is a modified version of your sandbox with some console logs added to handleMenuOpenClick:

  handleMenuOpenClick = event => {
    console.log(
      "Is Menu Open Button an Element",
      event.currentTarget instanceof Element
    );
    const targetDoc = event.currentTarget.ownerDocument;
    const targetWin = targetDoc.parentWindow;
    console.log(
      "Is Menu Open Button an ownerDocument.Element",
      event.currentTarget instanceof targetWin.Element
    );

    this.setState({ anchorEl: event.currentTarget, menuOpen: true });
  };

The first instanceof check returns false and the second one returns true (when using the popout window's Element for comparison).

[oliviertassinari]

@zekehernandez Oh wow, it's definitely a regression. Would the following diff solve the problem?

diff --git a/packages/material-ui/src/Popover/Popover.js b/packages/material-ui/src/Popover/Popover.js
index dac48ee93..c829974e1 100644
--- a/packages/material-ui/src/Popover/Popover.js
+++ b/packages/material-ui/src/Popover/Popover.js
@@ -139,9 +139,11 @@ const Popover = React.forwardRef(function Popover(props, ref) {
       }

       const resolvedAnchorEl = getAnchorEl(anchorEl);
+      const containerWindow = ownerWindow(resolvedAnchorEl);
+
       // If an anchor element wasn't provided, just use the parent body element of this Popover
       const anchorElement =
-        resolvedAnchorEl instanceof Element
+        resolvedAnchorEl instanceof containerWindow.Element
           ? resolvedAnchorEl
           : ownerDocument(paperRef.current).body;
       const anchorRect = anchorElement.getBoundingClientRect();
@@ -392,8 +394,9 @@ Popover.propTypes = {
   anchorEl: chainPropTypes(PropTypes.oneOfType([PropTypes.object, PropTypes.func]), props => {
     if (props.open && (!props.anchorReference || props.anchorReference === 'anchorEl')) {
       const resolvedAnchorEl = getAnchorEl(props.anchorEl);
+      const containerWindow = ownerWindow(resolvedAnchorEl);

-      if (resolvedAnchorEl instanceof Element) {
+      if (resolvedAnchorEl instanceof containerWindow.Element) {
         const box = resolvedAnchorEl.getBoundingClientRect();

         if (
diff --git a/packages/material-ui/src/Popper/Popper.js b/packages/material-ui/src/Popper/Popper.js
index 1471e127a..55a4ff090 100644
--- a/packages/material-ui/src/Popper/Popper.js
+++ b/packages/material-ui/src/Popper/Popper.js
@@ -5,6 +5,7 @@ import { chainPropTypes } from '@material-ui/utils';
 import Portal from '../Portal';
 import { createChainedFunction } from '../utils/helpers';
 import { setRef, useForkRef } from '../utils/reactHelpers';
+import ownerWindow from '../utils/ownerWindow';

 /**
  * Flips placement if in <body dir="rtl" />
@@ -206,8 +207,9 @@ Popper.propTypes = {
   anchorEl: chainPropTypes(PropTypes.oneOfType([PropTypes.object, PropTypes.func]), props => {
     if (props.open) {
       const resolvedAnchorEl = getAnchorEl(props.anchorEl);
+      const containerWindow = ownerWindow(resolvedAnchorEl);

-      if (resolvedAnchorEl instanceof Element) {
+      if (resolvedAnchorEl instanceof containerWindow.Element) {
         const box = resolvedAnchorEl.getBoundingClientRect();

         if (

[zekehernandez]

Yes, that does the trick, thank you!
Are you going to make that change in the codebase? Otherwise I could submit a PR.

Thanks again

[oliviertassinari]

Thanks for trying it out. It would really help if you could take care of the changes :).

[zekehernandez]

Absolutely, will do.