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

fix: remove scrollHandler on unbind #17640

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

demike
Copy link

@demike demike commented Feb 11, 2025

Defect Fixes

Fixes a bug when using one menu for multiple targets in popup mode:
p-menu in popup mode does not close on scroll in some cases.

Szenario:
I have 40 icons. Clicking on any of them opens a menu in popup mode (only one instance of menu).
If I scroll now the menu closes ( fine! )

Then I delete the selected icon (entry in the menu).
And then I open the menu on an other icon ( show target changes).
When I scroll now the menu does not disappear.

The reason is that the scrollHandler still has the reference to the already deleted element.
This is bad for two reasons:

  • It holds a reference to a deleted element (the first target)
  • Scrolling will never work again because bindScrollListener wants to bind to the scroll parents of the deleted element (which are not there)

The solution is to remove the scrollHandler member of Menu on unbindScrollListener
--> no more potential leaking of deleted elements
--> works now with differenet targets passed to show();

Copy link

vercel bot commented Feb 11, 2025

Deployment failed with the following error:

Creating the Deployment Timed Out.

@mertsincan mertsincan added the Resolution: Needs Issue An issue needs to be created for the pull request label Feb 14, 2025
@mertsincan
Copy link
Member

Could you please create an issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Needs Issue An issue needs to be created for the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants