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

[Bug] InfoWindow anchor reference not cleared on unmount #386

Closed
nickrschnelle opened this issue May 28, 2024 · 8 comments · Fixed by #393
Closed

[Bug] InfoWindow anchor reference not cleared on unmount #386

nickrschnelle opened this issue May 28, 2024 · 8 comments · Fixed by #393
Labels
bug Something isn't working

Comments

@nickrschnelle
Copy link

Description

When used with @googlemaps/markerclusterer, InfoWindows do not seem to correctly clear their anchor reference during component dismount. This causes empty InfoWindows to reappear against markers that have previously had their anchored InfoWindow closed.

image

This issue does not seem to occur when you actually click the 'X' to close the InfoWindow. Only when clicking from one marker with an open InfoWindow to another marker and thus opening it's InfoWindow and un-mounting the first marker's InfoWindow

I'm not sure if this is best to post as part of the description however I was able to do some digging to diagnose the issue.
I modified the library source file info-window.tsx to add each created google.maps.InfoWindow into a global array which I then console.loged out during the cleanup function of the useEffect

  // top of file
  const arrayOfWindows = [];
  
  ...

  useEffect(
    () => {
      if (!mapsLibrary) return;

      if (pixelOffset) {
        (infoWindowOptions as google.maps.InfoWindowOptions).pixelOffset =
          new google.maps.Size(pixelOffset[0], pixelOffset[1]);
      }

      // intentionally shadowing the state variables here
      const infoWindow = new google.maps.InfoWindow(infoWindowOptions);
>>    arrayOfWindows.push(infoWindow);
      const contentContainer = document.createElement('div');

      infoWindow.setContent(contentContainer);

      setInfoWindow(infoWindow);
      setContentContainer(contentContainer);

      // cleanup: remove infoWindow, all event listeners and contentElement
      return () => {
        console.log('infowindow cleanup lib');
        google.maps.event.clearInstanceListeners(infoWindow);
        infoWindow.close();
        contentContainer.remove();

        setInfoWindow(null);
        setContentContainer(null);
        // @ts-ignore
 >>     console.log('cleanup', arrayOfWindows);
      };
    }, [mapsLibrary]
  );

When reproducing the bug by opening an InfoWindow against one marker and then clicking another marker to open it's InfoWindow I was able to see the following logged out:
image

As you can see the first InfoWindow in the array still has a value assigned to it's anchor property and the bug with an empty InfoWindow occurs

If I was to repeat the process but close the InfoWindow by clicking the 'X' before opening a second InfoWindow the following is logged out:

image

Here the first InfoWindow's anchor prop is correctly set to null and no empty InfoWindow is present

Modifying the cleanup of the useEffect to manually set the anchor to null seems to fix the bug. I'm not sure if this is the best solution but it's working for us at the moment.

 return () => {
        google.maps.event.clearInstanceListeners(infoWindow);
        infoWindow.close();
   >>   infoWindow.anchor = null;
        contentContainer.remove();

        setInfoWindow(null);
        setContentContainer(null);
      };

Steps to Reproduce

https://codesandbox.io/p/devbox/jovial-glade-tx888r

  • Load map with multiple points
  • Click on Marker A
  • InfoWindow appears anchored to Marker A containing content
  • Click on Marker B
  • InfoWindow appears anchored to Marker B containing content
  • An InfoWindow with no content will still be anchored to Marker A

Through a lot of extra react state manipulation which involved explicitly resetting the markers state to {} and then adding markers back on the next render I was able to stop the issue happening immediately upon clicking from one marker to another, however the issue would still occur if I:

  • Clicked from one marker to another (which closes Marker A's InfoWindow and opens Marker B's InfoWindow)
  • Zoom out on the map so Marker A and Marker B cluster together
  • Zoom back in so Marker A and Marker B un-cluster
  • Empty InfoWindows appeared for both Marker A and Marker B

Environment

  • Library version: @vis.gl/[email protected] it has been happening for us since we started using the library in the 0.4 range at the end of last year
  • Google maps version: weekly
  • Browser and Version: Brave Version 1.64.122 Chromium: 123.0.6312.122 and Chrome Version 125.0.6422.77
  • OS: Mac OS 13.0

Logs

N/A
@nickrschnelle nickrschnelle added the bug Something isn't working label May 28, 2024
@usefulthink
Copy link
Collaborator

Wow! Thank you so much for the detailed write up. I haven't had the time to dive in yet, but that is truly amazing!

@usefulthink
Copy link
Collaborator

Did you actually manage to reproduce this with just 2 markers? In the clusterer example you shared, I can no longer reproduce it when I remove the markerclusterer.

I would really like to understand what is happening there and why. An InfoWindow that renders when it doesn't have a map set shouldn't render period and that would be a bug to report to the google maps team.

But if you found a solution that makes sense and makes the problem go away, I'll happily accept a PR for this (or I'll do it myself, whatever happens first).

@nickrschnelle
Copy link
Author

The bug only seems to occur when used with clustering. As I showed above it seems like when the InfoWindow react component is unmounted there is some sort of issue where the .close() function doesn't actually remove the reference to the anchor element. I couldn't work out why this was happening.

I've had a quick glance at how the clustering library works and it takes control of rendering all markers if they are clustered or not. So I believe whenever the map changes it recalculates how to render markers (either in a cluster or by itself). My guess is that during this rendering process, as that link between InfoWindow and Marker hasn't been removed properly, it renders an InfoWindow but as we have removed the component from the React render tree there's no content for this InfoWindow. Thus the empty window.

That's a semi informed guess, I could be completely on the wrong track but it seems like it's something along those lines.

I'll raise a PR with the fix I've found 😄

@usefulthink
Copy link
Collaborator

It looks to me like this has something to do with the fast re-rendering happening in the markerclusterer example.

To be honest, the markerclusterer example contains a couple of errors and bad practices that will – when fixed – stop this issue from surfacing (mostly missing memoizations that cause unneeded re-rendering). But now I want to get to the root cause and understand why this infowindow keeps hanging around so I can raise a bug-report with the maps platform team.

@usefulthink
Copy link
Collaborator

Another thing is that the anchor property even existing is very confusing, since typically the closure-compiler used by the maps API will obfuscate and mangle the names of everything that isn't public.

@nickrschnelle
Copy link
Author

It looks to me like this has something to do with the fast re-rendering happening in the markerclusterer example.

To be honest, the markerclusterer example contains a couple of errors and bad practices that will – when fixed – stop this issue from surfacing (mostly missing memoizations that cause unneeded re-rendering). But now I want to get to the root cause and understand why this infowindow keeps hanging around so I can raise a bug-report with the maps platform team.

Yeah for my implementation I did start from the clustering example but spent a lot of time trying to fix that code to find a solution to this bug. I used the example code as a base for the sandbox as it was the easiest way to just demo the bug. The sandbox example is a lot more broken than the state I was able to get to before diving in the library code to try to find a fix there but I wasn't able to find a complete fix without modifying the library (not to say one doesn't exist).

The best state I could get to without modifying the InfoWindow source was where clicking from one marker to another no longer caused the empty windows (the left over anchor reference was still there but it seems that cutting down on re-renders stopped them showing up). however they would still appear if you clicked through a few markers then zoomed the map out to make the markers you clicked cluster together, then zoomed back in so they separated again. At this point the empty windows would appear.

@usefulthink
Copy link
Collaborator

Ok, so here's what I found out:

  • when a google.maps.InfoWindow is opened with an anchor, and that anchor is removed from the map, the InfoWindow will be hidden, but it will keep the anchor reference around so it can be shown again when the anchor is re-added to the map. I can understand that this behavior is making sense for a lot of use-cases.

  • when the infowindow gets hidden this way, it will publish the closed event and set an internal flag like isOpen = false

  • the closed event is handled in react and eventually cause the InfoWindow component to be unmounted, leading to a call of infoWindow.close(), which get's ignored since the isOpen flag says its already closed.

  • when the marker appears again (which it happens to do with the markerclusterer – with regular react-components the same marker object wouldn't re-appear) the infowindow will show up again, but without the content, since that has already been flushed by react.

While debugging, I also found out how we can unset that anchor without accessing undocumented properties or hurting typescripts feelings: infowindow.set('anchor', null);. I'll work that into a PR I'm working on right now...

@usefulthink
Copy link
Collaborator

I reported this to the maps platform team as well: https://issuetracker.google.com/issues/343750849

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants