Financial-Times / ftdomdelegate

Create and manage a DOM event delegator
MIT License
320 stars 36 forks source link

Behavior is non-standard when handler returns false #62

Open lazd opened 10 years ago

lazd commented 10 years ago

With jQuery, returning false within a handler effectively calls event.preventDefault() and event.stopPropagation(). See the fiddle showing this behavior.

The same is true for Gator:

As an added convenience if you return false in your callback it is a shortcut for calling e.preventDefault() followed by e.stopPropagation().

However, it seems ftdomdelegate performs the equivalent of a preventDefault() and stopImmediatePropagation() when a handler returns false, which prevents other handlers at the same level in the DOM from executing. See the fiddle showing this behavior.

This seems non-standard given older libraries have established a consistent behavior, and I feel it should be changed to match.

matthew-andrews commented 10 years ago

Just because two popular ajax libraries do something doesn't make it a standard…

I honestly don't remember why we do both stopPropagation & stopImmediatePropagation but I do know that this would be a breaking API change and we'd need to do a major version bump & massive retesting of our websites. Is there any other reason than to match older libraries that this should be changed?

lazd commented 10 years ago

@matthew-andrews, the behavior is both unexpected and undocumented, hence the issue report. My suggestion is to match the behavior users familiar with other libraries will expect, but it's fine if the library behaves differently -- at least document it.

It should be noted that stopImmediatePropagation() itself is never actually called; it's breaking out of both the listener loop and the propagation loop by returning that achieves this effect by not calling the rest of the listeners as well as preventing simulated propagation. The comment above it says Stop propagation to subsequent callbacks, which, to me sounds like a mix up the notion of DOM propagation with preventing other listeners from being called, so I wondered if the original behavior may have been coded in error.

matthew-andrews commented 10 years ago

I wondered if the original behavior may have been coded in error.

Quite possibly. I'm not against this change as long as it actually fixes something but I've never come across a problem related to this – I just disagreed with the idea that we were non-standard.

matthew-andrews commented 10 years ago

Alright I think you're right. We ought to fix this… Thanks for taking the time to report the bug so thoroughly. It'll need to be a major version bump though as there's a danger it'll create bugs.

Pinging @wheresrhys @georgecrawford @rowanbeentje