akre54 / Backbone.NativeView

A reference implementation of a native Backbone.View
MIT License
113 stars 18 forks source link

Should delegate align with jQuery behavior? #16

Closed Florian-R closed 8 years ago

Florian-R commented 9 years ago

Related to akre54/chaplin/pull/4

There's some small inconsistencies between how jQuery and NativeView treat currentTarget and delegateTarget. I've put two ugly jsbin two illustrate this : one for jQuery and and one for NativeView, i think it comes from this jQuery bug

Basically, when using NativeView#delegate without selector, delegatedTarget is undefined, whilst in jQuery's case, it always equals currentTarget.

When used with a selector, currentTarget and delegatedTarget are swapped according to the implementation.

Not sure if it's possible or desirable to do something here. Thoughts?

akre54 commented 9 years ago

As much as possible I'd like to pass back the native event object, rather than a delegate. As far as I can remember it isn't possible to set currentTarget without creating a separate delegate, but I'm open to patches if you have any ideas.

Florian-R commented 9 years ago

As much as possible I'd like to pass back the native event object

Agree, the jQuery s' event normalization is fairly complicated and the goal here (in my view) is to stay as close as possible as native.

As far as I can remember it isn't possible to set currentTarget without creating a separate delegate, but I'm open to patches if you have any ideas

Just tried different thing, but didn't manage to have something working. I'd say for now, the check actually used by Chaplin might be the best bet.

if Backbone.$ then event.currentTarget else event.delegateTarget

One other idea to explore, as the name delegateTarget is not standart but jQuery specific, is to get rid of it to replace it by something else, for example bubblingTarget (this name sucks) and do instead

target = event.bubblingTarget else event.currentTarget
akre54 commented 9 years ago

target = event.bubblingTarget else event.currentTarget

Yeah that's a much more robust check. I like that better than checking for the existence of Backbone.$

I like the name delegateTarget best, but if there are compatibility issues then let's scrap it. Some good ideas to play around with.

Florian-R commented 9 years ago

Thanks for the answer. I'll fill a PR for this soon. Just to limit the back and forth, any idea on a better name than bubblingTarget? Also, this might breaks some others people code, so do we keep for the moment the current delegateTarget, and add some deprecation warning (in the readme or directly through console.warn)?

akre54 commented 9 years ago

Forgive the dumb question but what's the harm in keeping delegateTarget? Is it that it doesn't align with jquery's?

Florian-R commented 9 years ago

Is it that it doesn't align with jquery's?

Yap. So as currentTarget. In current implementation both are swapped regarding jQuery's. This is what caused failures in Chaplin's layout with NativeView when i removed the Backbone.$ check. As jQuery expose its own delegateTarget there's no way to do something like

target = event.delegateTarget else event.currentTarget

which works in all cases.

Florian-R commented 9 years ago

@akre Any news on this?

akre54 commented 9 years ago

Ha sorry -- was just responding to this.

My main goal in keeping delegateTarget is that it has become the de facto standard name for these types of shims (see https://github.com/component/delegate/ for instance). Is it a big deal to stick with delegateTarget for now and mimic jQuery here?

Florian-R commented 9 years ago

Is it a big deal to stick with delegateTarget for now and mimic jQuery here?

Not sure to fully understand what you mean here. Mind share a snippet?

akre54 commented 9 years ago

Ok so let me see if I understand this.

  1. The currentTarget and delegateTarget in NativeView are swapped with jQuery's currentTarget and delegateTarget.
  2. We can't assign currentTarget directly to the event object passed back from the DOM API because it is read-only.
  3. jQuery creates a clone of the event object, so that it can assign currentTarget and delegateTarget directly.

We should either 1) fix our implementation to match jQuery's (incorrect) naming, or 2) force any libraries using our library to adopt our (correct) naming or wrap if (Backbone.$) checks around everything.

The first option is bad but definitely the lesser of two evils. The second I'd rather avoid but may be inevitable.

Florian-R commented 9 years ago

fix our implementation to match jQuery's (incorrect) naming

This open a big can of worms (having to deal with expando to not normalize the event object in each handler in the bubbling phase) and moreover make the current implementation divergent of others shims.

force any libraries using our library to adopt our (correct) naming

Not sure this is possible (given it means modifying jQuery behaviour in the first place)

or wrap if (Backbone.$) checks around everything.

Maybe the simplest solution. That's worth a note in https://github.com/jashkenas/backbone/wiki/Using-Backbone-without-jQuery#for-plugin-authors i suppose?