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.

Sign up for GitHub

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

Jump to bottom

Handles firing closeOnClick before another listener #10926

Merged
merged 9 commits into from
Aug 12, 2021
Merged

Handles firing closeOnClick before another listener #10926

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a6ec310
Initial fix of force firing of onClose to happen first if multiple ev…
Aug 11, 2021
3f877d4
removed unused added importand extra space
Aug 11, 2021
9a10afd
changed == to ===
Aug 11, 2021
7513961
Implemented preclick solution and adjusted map event tests to reflect…
Aug 11, 2021
5dc231c
added sythetic preclick, WIP: closeonclick listener not activating
Aug 12, 2021
f30e3ac
Added documentation, fixed lint issue, as per unit test popup default…
Aug 12, 2021
a929463
added unit test for preclick event
Aug 12, 2021
ad50976
added whitespace to unit test file
Aug 12, 2021
a846524
fixed arrow expression error for lint test
Aug 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/util/evented.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

listeners.unshift(listeners.splice(1, 1)[0]);
}

for (const listener of listeners) {
listener.call(this, event);
}
Expand Down