Custom Control Click Handlers not working after map is hidden.

[noahtaylor]

I followed the examples in the showcase to create ClickHandlers on custom controls as in: WeatherLayersWidget.java

The only difference being I show map on a popup which can be opened and closed again. The first time I show the map, the controls and the click handler work fine. When I try to show the map again, the ClickHandlers are now dead.

To rectify I tried 2 things:

1. Add new ClickHandlers when I show the map. This did not work.
2. Remove old control and add a new control with new click handlers. This worked.

I tried to dig into the logic of how the events were being added to the DOM to figure out what the issue was but I didn't have much success. Wondering if anyone else might have some insight.

My basic coded for creating a click handler then adding the control to the map.

controlClickReg = mapControl.addCloseDistrictClickHandler(new ClickHandler() {
    @Override
    public void onClick(ClickEvent event) {

      ///do something
    }
});

// setup map control
nbhMap.setControls(ControlPosition.TOP_LEFT, mapControl);

// where on my map control the above method is just a passthrough:
HandlerRegistration addCloseDistrictClickHandler(ClickHandler clickHandler) {
    return btnCloseDistrict.addClickHandler(clickHandler);
}

[twistedpair]

@noahtaylor thanks for the reports. I'm glad you're getting use out of the library.

Generally in GWT, you've got to make sure you register, then unregister events from actual JSO's when you show/hide them, then apply the register/unregister process the next time you show them. This is especially important for external widgets like GMaps where you don't really know when and if they're going to clean themselves up.

Reusing and moving a GMaps object around has been a known problem in GWT given how the GMaps API does not clean up after itself well. See #168.

If you can send along an example that replicates the behavior (complete with the popup you're using), I'd be happy to investigate further. Thanks.

[noahtaylor]

Yes indeed I am getting a massive amount of use from this library, it's great and thanks to everyone for their hard work.

Also thanks for getting back to me on my question. With respect to event registration, I'm a bit surprised about having to un/register on show/hide. I have an example here, which i won't leave up for long. If you click on the Neighbourhoods button a map will pop up, if you click outside the panel, the popup will hide.

http://1-dot-level-choir-562.appspot.com/

I have written a custom control at the top left corner. It is for this control where I need to re-register my click events. In contrast, if you also look at the bottom left of the map, I have added a simple checkbox. For this there is no need to create re-register my clickevents. You can try by showing and hiding the map then clicking on the checkbox. The text will change from true to false.

In addition, for the geometric areas I have added using the gwt Data api for google maps I have written, there has been no need to re-register my clickhandlers either. Show and hide the map, you will be able to see the click functionality is intact.

Is it a bit of a fluke, my clickhandlers are not needing to be re-registered? Is it best practices to re-register?

[twistedpair]

@noahtaylor, you're right. Registering/Unregistering handlers manually is a GWT edge case, not a standard practice.

As you probably know, a major design goal of GWT was to eliminate the possibility of JS memory leaks. The most common cause was element reference cycles in element listeners. As such all event listeners were to be wrapped in the GWT listener methods so that GWT could guarantee they were properly registered and deregistered with the Widget lifecycle. That's what I was referring to.

The other thing I was referring to was that when you're drilling into an external JS API, and you add a listener to an element it creates, such that the element's lifecycle is beyond your control, you lose the lifecycle guarantee's of GWT as GWT no longer controls these elements.

In the case of your popup, it's not clear what is happening to the popup element when it is being hidden. Perhaps it's being set to display:none or visible:hidden or has a top offset far off screen, or the popup is actually being destroyed. The behavior you describe of it not working on the second popup suggests that either (1) your listeners are still around, but pointing to an old element (control) that is no longer available, or (2) for some reason the listeners were removed. Using Chrome Dev tools, you should be able to answer the above.

Sadly I didn't catch your URL in time. If you want to set it up at a clandestine URL of your choosing, email it to me at the following fragment and I'll debug away since I plan to do something similar to this soon for a project of my own.

[noahtaylor]

Thanks again for all your input, very helpful!!

Interesting what you mentioned, I had no idea you could see the eventhandlers through dev tools, definitely an awesome trick. As you suggested, I tried tracing this through with chrome dev tool and I can see the clickhandler is still there on the control but as you say it must be pointing to old elements but given the control is there, the handler still exists and the handler points back to the control. Does this make sense? Maybe my understanding is limited but it seems very circular.

Perhaps the most logical conclusion is it is something going on in the JS API and no really anything to do with gwt and the only remedy is to re-register the handlers.

[noahtaylor]

@twistedpair Ok so I followed your advice and debugged the issue to the root by setting the compiler style to pretty. It seems the culprit for removing the listeners is actually this bit of code in MapWidget (line 97-100)

protected void onDetach() {
    super.onDetach();
    removeControls();
}

This bit of code removes the references to the controls within the map widget so when a ClickEvent is fired on the map it no longer tries to call any of the events on the controls. If I remove the removeControls() method in onDetach() this fixes this issue. Do you know why the controls need to removed, do you think they should?

[twistedpair]

@noahtaylor the odd behavior that stands out is that onDetach() is being called, but the elements are not actually being destroyed. For example, if I save a reference to the Transit button to the DOM on the first load, and then close and reopen, it's the same object. So, the map and its elements were not actually destroyed.

The impetus for onDetach() is the call to PopupPanel.hide(). From the JS, it looks like autoClosed is true. Do you really want to close and destroy the popup on hide, or would you rather want to reuse it? You might want to confirm this config. Without SourceMaps, it's hard for me to tell the setup from the JS.

It's a tad painful, but on stepping through the pretty printed JS, I see that controls == null in MapWidget during the first removeControls invocation (line 7561). So it looks like there is a MapWidget being hidden with no controls. Then a second call is made to that method and this time 2

controls are removed. Are there really 2 maps being removed?

To answer your direct question controls have to removed because it is part of the Widget lifecycle contract. The MapWidget is being removed from the DOM as the final step in its lifecycle and as such it must clean up all of its own child widgets which must clean up their children, etc, etc... However, on reading up on the Map object in the JS API, I only see documentation for adding Controls but never for removing them. This appears to be an oversight in their API.

My only thought on the above is that perhaps we could move that code to onUnload so that the objects being removed are still around and might behave as expected. onUnload is best for cleaning up DOM objects, since you're still attached, but in onDetach you're removed from the DOM, so DOM calls could result in unpredictable behavior.

Hope that give you some more leads. :)

[noahtaylor]

@twistedpair thanks plenty of leads to go on now. I need to read up on DOM etc. to get a better understanding... Very astute observation about the 2 maps. Yes indeed it is intentional, I have 2 maps, one overlaying the other in order to place the polygons I've drawn below the labels. The first map has all the background and polygons, the second map is transparent except the labels hence it allows you to simulate placing items between the background and the labels. Works pretty well except a few rendering issues with the labels (both big and small appearing twice). As one map is placed in front of the other, the controls only exist on top and that is why you see controls=null on the second map.

[noahtaylor]

@twistedpair ok so having delved into the details more regarding DOM detachment and memory leaks etc. I see when the controls are added they are being treating the same as if we were to create the controls by wrapping DOM elements. I think this is necessary because we are adding the controls directly via JS rather than taking the usual approach of instantiating and element and then calling add or something which generally calls adopt() or setParent() on the widget, putting the widget in the logical hierarchy. Since the map widget is neither a Panel nor a Composite we cannot place these objects in the logical hierarchy and therefore instead we call RootPanel.detachOnWindowClose() on the MapPanels which contain the controls (and the InfoPanels), in order for the these objects to be properly garbage collected.

Now with the PopupPanel I am using to contain the map, this panel is detached from the DOM (Intentionally) when the panel is hidden and attached again when we show the panel. The issue is, I believe the controls, unlike everything else, are not being re-attached to the DOM. To me it would make sense that since we are overriding onDetach() in MapWidget to detach the controls from the rootPanel @Override protected void onDetach() { super.onDetach(); removeControls(); } We should similarly override onAttach() or onAttachChildren() on MapWidget to re-attach the controls. Then we would have behaviour which is consistent with the other widgets contained in the popup panel, avoid having to recreate widgets when showing and hiding, and continue to prevent memory leaks.

[branflake2267]

I've been wondering when someone might find that. Yeah I remember thinking I needed to come back and fix that.

[branflake2267]

In this case, at the moment, you'll have to clean up the children widgets manually and then detach.

[noahtaylor]

I took the liberty of forking and committing some changes which override the doAttachChildren/doDetachChildren methods in the MapWidget and no longer automatically add the mapPanel to the detach list. This now works for me locally, where i can show and hide my map in the popup panel and not need to re-add the custom control. I am going to mark the issue as closed, let me know if you're happy with the changes. Thanks for all your input.

[valsoray17]

I'm having the exact same issue now with the maps. @noahtaylor thank you for taking time to create the fix for this. Does anybody know if this fix will be incorporated in future version of the GWT-Maps-V3-Api and if so when would be the release?

Thank you!