Clean up DOM event listeners when destroying Map's animation proxy

[samclaus]

In the constructor for Map, a listener is added for the 'transitionend' event of the animation proxy element (if zoom animation is enabled):

DomEvent.on(this._proxy, 'transitionend', this._catchTransitionEnd, this);

This PR adds a DomEvent.off() call to clean up that listener inside of the Map._destroyAnimProxy() method.

Closes #9046.

[Falke-Design]

Thank you for making this PR. Can you please apply the requested changes.

Can you please move this code into a function _animateProxyZoom and then remove the zoomanim event in _destroyAnimProxy too. this.on('zoomanim',this._animateProxyZoom, this);

Leaflet/src/map/Map.js

Lines 1629 to 1638 in 847fbb4

	this.on('zoomanim', function (e) {
	const transform = this._proxy.style.transform;
	DomUtil.setTransform(this._proxy, this.project(e.center, e.zoom), this.getZoomScale(e.zoom, 1));
	// workaround for case when transform is the same and so transitionend event is not fired
	if (transform === this._proxy.style.transform && this._animatingZoom) {
	this._onZoomTransitionEnd();
	}
	}, this);

[Falke-Design]

Suggested change

DomEvent.off(this._proxy, 'transitionend', this._catchTransitionEnd, this);

Please move this line into the remove function after the event unload:

Leaflet/src/map/Map.js

Lines 775 to 780 in 847fbb4

	if (this._loaded) {
	// @section Map state change events
	// @event unload: Event
	// Fired when the map is destroyed with [remove](#map-remove) method.
	this.fire('unload');
	}

[samclaus]

@Falke-Design May I simply add a this.off() call to the Map.remove() method? I believe the map is already unusable after removal, so that simply removing all event listeners in a single shot makes a lot of sense for performance AND JavaScript bundle size (because we don't need a ton of individual event listener removals)? Perhaps that is out of the scope of this PR, but I would gladly open another one to do that if it is okay with you. I will make the requested changes for this PR and postpone that other idea.

[samclaus]

@Falke-Design I made the following changes:

1. The Map constructor originally had the following:

   // zoom transitions run with the same duration for all layers, so if one of transitionend events
   // happens after starting zoom animation (propagating to the map pane), we know that it ended globally
   if (this._zoomAnimated) {
       this._createAnimProxy();
       DomEvent.on(this._proxy, 'transitionend', this._catchTransitionEnd, this);
   }

   I moved the 'transitionend' listener to the end of _createAnimProxy() so it is grouped with the rest of the proxy setup code.

2. I extracted the 'zoomanim' event closure into an _animateProxyZoom(e) method like you requested.

3. I changed the _destroyAnimProxy() from an 'unload' listener to a direct call inside of Map.remove(), like you requested. Because of this change, an if guard is required to make sure the animation proxy was created in the first place, and I opted to place the guard inside of _destroyAnimProxy() instead of wrapping the method call inside of remove(), so that the method is idempotent and less dangerous to call, with no harm to performance or code size or readability.

4. I reordered the proxy-related methods into the order they are used:

   1. _createAnimProxy()
   2. _animateProxyZoom(e)
   3. _animMoveEnd()
   4. _destroyAnimProxy()

5. I cleaned up the indentation of code within the proxy-related methods, e.g.:

   // BEFORE (very long line)
   DomUtil.setTransform(this._proxy, this.project(e.center, e.zoom), this.getZoomScale(e.zoom, 1));
   
   // NOW
   DomUtil.setTransform(
       this._proxy,
       this.project(e.center, e.zoom),
       this.getZoomScale(e.zoom, 1),
   );

[Falke-Design]

Looks good

[samclaus]

@jonkoops Pinging because it has been almost 3 months. Can this fix be merged?