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

.off(): unexpected behavior #6596

Closed
3 tasks done
johnd0e opened this issue Apr 14, 2019 · 5 comments
Closed
3 tasks done

.off(): unexpected behavior #6596

johnd0e opened this issue Apr 14, 2019 · 5 comments
Labels

Comments

@johnd0e
Copy link
Collaborator

johnd0e commented Apr 14, 2019

.off() method unexpectedly removes listeners of the parent object

  • I've looked at the documentation to make sure the behavior is documented and expected
  • I'm sure this is a Leaflet code issue, not an issue with my own code nor with the framework I'm using (Cordova, Ionic, Angular, React…)
  • I've searched through the issues to make sure it's not yet reported

Steps to reproduce
Steps to reproduce the behavior:

Expected behavior

Only listeners directly attached to layer should be removed

Current behavior

map listeners get removed too, unexpectedly

Environment

  • Leaflet version: 1.4
  • Browser (with version): Chrome 73.0.3683.103 (Official Build) (32-bit)
  • OS/Platform (with version): Windows 10 (32-bit)

Additional context

It can appear that this behavior is by current leaflet design.
In this case we should at least document it explicitly and in detail!

Minimal example reproducing the issue

var map = mymap;
var layer = L.layerGroup();
L.control.layers({},{layer:layer}).addTo(map);

map.on('overlayadd',()=>map.openTooltip('map event',map.getCenter()));

// uncomment this to see the issue:

//layer.off()

https://jsfiddle.net/johnd0e/jy84gsL2/16/

@johnd0e
Copy link
Collaborator Author

johnd0e commented Apr 14, 2019

Current behavior is highly undesirable in my setup, because:

  • map listeners is bound in main script
  • layer(s) is processed in independent plugin(s)

So I'd prefer if plugin'd not affect main script base functionality.

@IvanSanchez
Copy link
Member

Expected behavior

Only listeners directly attached to layer should be removed

I disagree. The point of .off()is to remove all event listeners, no matter where they were attached from.

In this case we should at least document it explicitly and in detail!

I agree with this. This should be a simple case of changing the wording at...

* Removes all listeners to all events on the object.

...and adding something like "This includes implicitly attached events". Not sure what the best wording would be.

@johnd0e
Copy link
Collaborator Author

johnd0e commented Apr 15, 2019

The point of .off()is to remove all event listeners, no matter where they were attached from.

I can agree with this.
But then it would be convenient to have also more 'safe' version of .off, which unbind only explicit listeners.

@IvanSanchez
Copy link
Member

But there is no way to tell apart "implicit" versus "explicit" event listeners in the code.

@johnd0e
Copy link
Collaborator Author

johnd0e commented Apr 15, 2019

Well, I suppose we can improve the code. Like, add owner to every listener as a property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants