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

Close button not working in 5.2.0 if popup is added to map again #5576

Open
siimots opened this issue Mar 3, 2025 · 2 comments
Open

Close button not working in 5.2.0 if popup is added to map again #5576

siimots opened this issue Mar 3, 2025 · 2 comments
Labels
need more info Further information is requested

Comments

@siimots
Copy link

siimots commented Mar 3, 2025

maplibre-gl-js version: 5.2.0

browser: Edge 133

Steps to Trigger Behavior

  1. Add popup to map
  2. Do not close that popup
  3. Add same popup again with addTo(map)
  4. It is not possible to close popup with button
  5. If already opened popup is changed without .addTo(map) then close button works again

Link to Demonstration

https://jsbin.com/gorenunoga/edit?html,js,output

Expected Behavior

Close button should work even after adding popup again.

Actual Behavior

Close button does not work in 5.2.0, but did work in 5.1.1.

Comment

If popup is already open and addTo(map) is not used then it is possible to close popup.
Probably it is good idea to check if popup is already open but it was not necessary in previous release.

@HarelM
Copy link
Collaborator

HarelM commented Mar 3, 2025

Thanks for taking the time to open this PR!
I believe this is related to a recent change to prevent a memory leak.
@kamil-sienkiewicz-asi can you take a look at this issue?

To be honest, I'm not sure what the right behavior is here as other events are also removed when the popup is removed...

@HarelM HarelM added the need more info Further information is requested label Mar 3, 2025
@kamil-sienkiewicz-asi
Copy link
Contributor

😖 fixing a bug caused another one to pop up? :D
I wonder what's the point of calling .addToMap() again when it's already on map. I think that it would be a huge breaking change for everyone, but shouldn't addToMap() just return early when popup is already constructed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need more info Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants