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

Popover listens to onScroll event even before displayed the first time #9481

Closed
1 task done
romanzenka opened this issue Dec 12, 2017 · 3 comments
Closed
1 task done
Labels

Comments

@romanzenka
Copy link

romanzenka commented Dec 12, 2017

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

I have found a very similar issue: #7012 but that talks about the handler being detached AFTER closing popover. Our popovers do not even open, yet they hook into the onScroll event.

Expected Behavior

I expect Popover to not touch scroll events until it is actually on screen.

Current Behavior

Every Popover element (including the ones in IconMenu) hooks into the onScroll event. If there are many of these present, the handling of the scroll events is impacted.

Steps to Reproduce (for bugs)

  1. Go to http://www.material-ui.com/#/components/card
  2. Check Chrome's Dev Tools > Elements > Event Listeners > scroll
    You will see there is one scroll listener
    This is your baseline
  3. Now click on the Popover menu item to see the example Popover page
  4. Check Chrome's Dev Tools > Elements > Event Listeners > scroll
    You will see FOUR listeners.
    One listener was registered for each Popover on the screen, even though they are not visible at the moment

This means Popovers are registering even if they are not active at all.

Context

We are (ab)using Popovers in our app, using hundreds/thousands of IconMenu elements within a table. Each of these contains a Popover and each Popover hooks into onScroll. This leads to bad performance when the table size grows.

Your Environment

Tech Version
Material-UI 0.19.4
React 15.6.2
browser Chrome 63.0.3239.84
@romanzenka
Copy link
Author

I made a trivial pull request #9482 but I am not sure if it does not break some higher-level concept somewhere... tests passed.

@m2mathew
Copy link
Member

m2mathew commented Jan 8, 2018

PR merged. Thanks again, @romanzenka!

@m2mathew m2mathew closed this as completed Jan 8, 2018
@romanzenka
Copy link
Author

Thank you for a prompt review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants