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

Menu miss anchorEl position when scroll in virtualized list #17596

Closed
2 tasks done
eAmin opened this issue Sep 27, 2019 · 8 comments · Fixed by #17599
Closed
2 tasks done

Menu miss anchorEl position when scroll in virtualized list #17596

eAmin opened this issue Sep 27, 2019 · 8 comments · Fixed by #17599
Labels
component: Popover The React component. component: Popper The React component. See <Popup> for the latest version. good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@eAmin
Copy link
Contributor

eAmin commented Sep 27, 2019

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

Current Behavior 😯

I use BaseTable component, it's a virtualized data viewer based on react-window.
I have to use Material-UI Menu in that list, for the first time, when you click on the button it's all fine and Menu showed correctly under Button(anchorEl). But when you scroll the data then click again on the Button you will see menu showed on top of the List(virtualized table) and second click on the button will be fine again and after scroll again problem repeated.

Steps to Reproduce 🕹

You can see the example of problem in here:
https://codesandbox.io/s/funny-night-57ecb?fontsize=14

in

Steps:

  1. Click on some button everything is fine.
  2. Scroll and click on some button again problem is showed up.

Your Environment 🌎

Tech Version
Material-UI v4.4.3
React 16.9.0
Browser Chrome , Electron and maybe all
@eAmin eAmin changed the title Menu miss anchorEl when scroll in virtualized list Menu miss anchorEl position when scroll in virtualized list Sep 27, 2019
@oliviertassinari oliviertassinari added support: question Community support but can be turned into an improvement good first issue Great for first contributions. Enable to learn the contribution process. component: Popover The React component. component: Popper The React component. See <Popup> for the latest version. and removed support: question Community support but can be turned into an improvement labels Sep 27, 2019
@oliviertassinari
Copy link
Member

@eAmin Oh wow, this is an interesting edge case. The provided anchorEl is present in the document after the prop-type checks run but is not before the effect that positions the modal runs. I would propose the following diff:

diff --git a/packages/material-ui/src/Popover/Popover.js b/packages/material-ui/src/Popover/Popover.js
index 451dae089..6b0d4c760 100644
--- a/packages/material-ui/src/Popover/Popover.js
+++ b/packages/material-ui/src/Popover/Popover.js
@@ -140,6 +140,27 @@ const Popover = React.forwardRef(function Popover(props, ref) {
           ? resolvedAnchorEl
           : ownerDocument(paperRef.current).body;
       const anchorRect = anchorElement.getBoundingClientRect();
+
+      if (process.env.NODE_ENV !== 'production') {
+        const box = anchorElement.getBoundingClientRect();
+
+        if (
+          process.env.NODE_ENV !== 'test' &&
+          box.top === 0 &&
+          box.left === 0 &&
+          box.right === 0 &&
+          box.bottom === 0
+        ) {
+          console.warn(
+            [
+              'Material-UI: the `anchorEl` prop provided to the component is invalid.',
+              'The anchor element should be part of the document layout.',
+              "Make sure the element is present in the document or that it's not display none.",
+            ].join('\n'),
+          );
+        }
+      }
+
       const anchorVertical = contentAnchorOffset === 0 ? anchorOrigin.vertical : 'center';

       return {
diff --git a/packages/material-ui/src/Popper/Popper.js b/packages/material-ui/src/Popper/Popper.js
index 1073cefea..37813d887 100644
--- a/packages/material-ui/src/Popper/Popper.js
+++ b/packages/material-ui/src/Popper/Popper.js
@@ -97,7 +97,33 @@ const Popper = React.forwardRef(function Popper(props, ref) {
       setPlacement(data.placement);
     };

-    const popper = new PopperJS(getAnchorEl(anchorEl), popperNode, {
+    const resolvedAnchorEl = getAnchorEl(anchorEl);
+
+    if (process.env.NODE_ENV !== 'production') {
+      const containerWindow = ownerWindow(resolvedAnchorEl);
+
+      if (resolvedAnchorEl instanceof containerWindow.Element) {
+        const box = resolvedAnchorEl.getBoundingClientRect();
+
+        if (
+          process.env.NODE_ENV !== 'test' &&
+          box.top === 0 &&
+          box.left === 0 &&
+          box.right === 0 &&
+          box.bottom === 0
+        ) {
+          console.warn(
+            [
+              'Material-UI: the `anchorEl` prop provided to the component is invalid.',
+              'The anchor element should be part of the document layout.',
+              "Make sure the element is present in the document or that it's not display none.",
+            ].join('\n'),
+          );
+        }
+      }
+    }
+
+    const popper = new PopperJS(resolvedAnchorEl, popperNode, {
       placement: rtlPlacement,
       ...popperOptions,
       modifiers: {

Do you want to submit a pull request? :)

Regarding your issue, you need to memoize the TableCell component (React.useCallback): https://codesandbox.io/s/hungry-jones-5tn9x.

@eAmin
Copy link
Contributor Author

eAmin commented Sep 28, 2019

@oliviertassinari Thanks mate, you made my day :)

Do you want to submit a pull request? :)

Of course, you made the changes and I just brought them all together.
#17599

@eAmin eAmin closed this as completed Sep 28, 2019
@croraf
Copy link
Contributor

croraf commented Sep 28, 2019

@eAmin Not sure it is important, but the sandbox you provided has lots of warnings in the console. Can you fix those in the sandbox?

@eAmin
Copy link
Contributor Author

eAmin commented Sep 28, 2019

@croraf Fixed: https://codesandbox.io/s/musing-khorana-6e97h

@joshwooding joshwooding reopened this Sep 28, 2019
@croraf
Copy link
Contributor

croraf commented Sep 28, 2019

I mean this react-base-table has the worst documentation ever. I cannot do anything with it. I suggest you use some other library.

@eAmin
Copy link
Contributor Author

eAmin commented Sep 28, 2019

@croraf I agree with you, but unfortunately I couldn't, actually it's like react-virtualized but better than it. Almost some APIs is similar to react-virtualized (if you do some stuff with react-virtualized you realized that).

BTW, my main issue is fixed and thanks to @oliviertassinari it turns out I have to memoize TableCell component.
I think we can use react-window library, if you want I can reproduce with react-window for more research.

@croraf
Copy link
Contributor

croraf commented Sep 28, 2019

@eAmin Cool. I made this sandbox to play with https://codesandbox.io/s/silent-surf-hbhpe

@eAmin
Copy link
Contributor Author

eAmin commented Sep 28, 2019

@croraf I've made change to that sandbox to use with react-window, you can access it in this link: https://codesandbox.io/s/frosty-fast-hgyjr

If we remove React.useCallback and use plain function, we missed anchorEl position completely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popover The React component. component: Popper The React component. See <Popup> for the latest version. 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.

4 participants