Handles firing closeOnClick before another listener #10926
Conversation
src/util/evented.js
Outdated
| @@ -124,6 +124,12 @@ export class Evented { | |||
|
|
|||
| // make sure adding or removing listeners inside other listeners won't cause an infinite loop | |||
| const listeners = this._listeners && this._listeners[type] ? this._listeners[type].slice() : []; | |||
|
|
|||
| // Check if onClose is an event listener and move to front of list to fire first | |||
| if (listeners.length > 1 && listeners[1].name === "bound _onClose") { | |||
There was a problem hiding this comment.
It may work for this specific use case, but this solution is brittle, with possible cases where it breaks and it's very hard to debug — for example, what if a user puts multiple click listeners on a popup? Then _onClose will not be the second, but later in the array. What if a user adds a listener with the same name? What if a different class in GL JS (not popup) uses the same method name (_onClose) and the execution order is swapped unexpectedly?
I think we should find a solution that's more explicit and can't produce unexpected side effects — e.g. firing a preclick event before click here and then subscribing to it for Popup closeOnClick. This is how Leaflet handles it as seen here.
There was a problem hiding this comment.
Thanks so much for the feedback! Yes I wasn't confident on this initial approach. I appreciate sending over the resources and example from leaflet. I implemented a preclick that works manually with the debug file but is breaking some of the unit tests in test/unit/ui/map_events.test.js that I'm looking into now.
… preclick call on click events
src/ui/handler/map_event.js
Outdated
| @@ -38,8 +38,13 @@ export class MapEventHandler { | |||
| this._map.fire(new MapMouseEvent(e.type, this._map, e)); | |||
| } | |||
|
|
|||
| preclick(e: MouseEvent) { | |||
| this._map.fire(new MapMouseEvent(e.type, this._map, e)); | |||
There was a problem hiding this comment.
e.type will be equal to click here (propagating from the DOM event of the same name) so looks like it'll fire click twice — I think we want to explicitly specify preclick here instead.
… behavior should be true for closeOnClick
|
Not sure what's going on with the render tests, the changes don't seem to have anything that would cause so many failures so it might be something unrelated to the PR entirely... |
|
|
|
@mourner thanks so much for your help! FYI - it says per the unit tests that the expectation for the default behavior for closeOnClick should be true, but it was set as false (which I just changed in the recent commit), I also added the extra documentation in src/ui/map.js but wondering if that is unneeded and should I remove? Yes, I'm working on a unit test right now! .... Edited to add: Guess I must have changed the default behavior of closeOnClick myself and forgot as it's not showing up in changes, oops! |
That was a good call, let's keep it — you never know when a user might find something like this useful, and |
This PR closes #10029.
Previous bug required double click to open a popup in new area when using the same popup as the closeOnClick event listener fired after opening the popup event listener. This change moves the closeOnClick listener to be fired first in the case that there are multiple listeners for an event.
You can manually test this with debug/popup.html and using this code to create the popup:
const popup = new mapboxgl.Popup(); popup.setHTML("Hello world"); map.on("click", (e) => popup.setLngLat(e.lngLat).addTo(map));WIP: this doesn't consider the use-case if a user were to click on the same target intending to close a popup, and would prefer moving into helper function without having the index hard-coded in the logic.
Functionality before the change:
Screen.Recording.2021-08-11.at.11.06.04.AM.mov
Functionality after the change:
Screen.Recording.2021-08-11.at.11.03.34.AM.mov
Launch Checklist