Reconsider stopPropagation in view triggers

[JSteunou]

View triggers actually set stopPropagation to true by default. I do not know why this was set at first, but from my pov it's a real pain. I have a lot of views inside region triggering events on click for parent of behavior to listen, and when that parent or another parent far above have to listen a click too, I fall into the usual trap: click is stopped by view triggers stopPropagation.

Can we re-discuss that behavior and maybe change it for v3 as a breaking change?

[maniator]

Better idea:

Instead of a "breaking change" just add some sort of options for the trigger to set stopProp to false for certain triggers as needed.

[paulfalgout]

There are already options for stopPropagation and preventDefault, I think @JSteunou is suggesting changing the default.

@JSteunou are you suggesting leaving preventDefault: true but changing `stopPropagation: false'?

I'm not entirely aware of the caveats of changing the defaults. Makes me a little nervous.

[JSteunou]

It's clearly a call for a breaking change. stopPropagation was introduced way back when adding those default values to triggers because people wanted it for forms. But preventDefault is the only one needed to stop forms to be send. stopPropagation is rarely use and do more harm from my pov.

If some people relied on that feature to stop an event to bubble up to a parent view, they only be concerned if they listened that event on the parent view and want the event specifically come from that parent layout and not from the child. But I do think that case is minor compared to normal use case, and so default value should represent majority.

I'm really interested to know people usage of this feature and see if I'm the only one seeing that or if I'm totally wrong. My PoV is obviously tight to the apps I'm writing.

[JSteunou]

@marionettejs/marionette-core could this be considered for v3? I am afraid it will not be after v3 go out because it introduces a breaking change.

Again, from my experience, triggers default options introduce not expected behavior. When using events to handle click, we have to manually call to preventDefault or stopPropagation when we need to, but when using triggers we are always surprised it is set by default.

I had the case again today with a co-worker, not understanding why its link <a href=""></a> did not work anymore. It was because he added a triggers on click on it.

[paulfalgout]

I still am unaware of the caveats and don't feel comfortable pushing for either direction, but I have an idea. Why don't we implement this with a feature flag? We can add a feature flag that changes the default for v3.. (or even v3.x) then if it seems to work fine we can swap the default for v4.. and if there are no major complaints we could remove the flag altogether for v5

[bazineta]

In examining our own codebase, we found that 65% of all use of triggers included overriding the default stopPropagation flag, and further, many, many instances where we'd avoided using triggers because of the default behavior being confusing.

In our case at least, it doesn't seem like that great of a default setting.

[JSteunou]

That reassuring. I wouldnt push this change too much so far because I though I was the only one bothered by this.

[paulfalgout]

PR welcomed.. I'd still say this is an excellent candidate for a breaking change behind a feature flag for v3.x

[paulfalgout]

So I just realized why stopPropagation() is true by default. And I think it's highly likely we may want to keep it around. Consider this issue: #2967

It is specifically talking about events and not triggers. But with events you can have similar listeners leak up to the parents rather easily by simply using the same selectors in both views..

But I believe triggers avoids this because it stops the propagation. And this can be a very difficult and odd thing to debug if it is occurring.

So now more than ever I think #2975 is important and we should discourage using events and stopPropagation() is a feature of triggers.

That said, I'd still support a flag for changing the default.. But I think now I'm leaning towards leaving it true.

[JSteunou]

What could solve all those cases:

• Fix backbone event delegation lake of target check (maybe by using view.cid or event.target/currentTarget) so event do not leak outside its view. Will remove this awkward need of stopPropagation which could be not desired. Event propagation is natural and expected.
• consider Trigger View Handler should return event object #2975 and forward event object
• make application level options to choose the default behavior for trigger / event regarding stopPropagation, capture phase, etc.

[paulfalgout]

The 2nd 2 are in PR #3261. The first one is a bit complicated. At least when looking into it, the solution wasn't immediately clear. Also this would be one of the first instances where we would modify how backbone works. I think it would be interesting to see if backbone would accept a PR for this. In the very least they could speak to the downsides.

[paulfalgout]

Here's a comment from backbone on this: jashkenas/backbone#1354 (comment)

However I still don't know how any of this works without stopPropagation unless the developer handles each case individually. Backbone events are delegated from the el. So after children are rendered any of the same selector will trigger the event in the parent. Sure you could do something like parentNode or $closest, but those a very big assumptions. My parentView template could certainly be:

<div class="something">
  <div class="very">
    <div class="deep">
      <div class="trigger-element">
      </div>
    </div>
  </div>
</div>
<div class="childview-region"><!-- .trigger-element will be in the childview shown here --></div>

At that point I don't know a generic solution other than stopPropagation that handles the case.

So I maintain my argument that we forward the event argument and we have a feature flag for triggersStopPropagation (and maybe triggersPreventDefault as well?) and they default to true.

Am I missing a solution that works for the general case without leaving the user open to very confusing unexpected bugs?

[JSteunou]

Maybe it's possible to flag the event in the first callback so when passing to another callback it can be obvious it does not came from this view but a child view.

[paulfalgout]

Oddly that is what jquery is doing with stopPropagation.

[JSteunou]

That would be our own stopPropagation at Backbone / Mn level without interfering with the one at jQuery / Native DOM level. I very like this idea.

[paulfalgout]

Will we still need a way to turn it off? I would assume so? Or maybe not? This'll definitely be a breaking feature. Not sure if anyone is depending on the jquery propagation that this wouldn't cover, but certainly possible. I am ok with turning off stopPropagation if I don't have to do something to the event every time and if the event is encapsulated to the view it's triggered in.

What's a little odd about this in general is that backbone view events have a single handler. A single place to handle the events object. This isn't true for triggers (although we would recommend not messing with the Event object outside the view). What's hard though is if we say we're stopping propagation, would people assume this related to the other handlers of the trigger?

[JSteunou]

I might open a new issue about the idea of fixing backbone event delegation lake of target check by using view.cid but the main point of this discussion was fixed by #3261