View's region using el from child view, not own view

ambischof commented 7 years ago

Description

See this fiddle https://jsfiddle.net/hsrkbs8d/

If a view has more than one region and one region's view contains an element with the same selector as another region's selector and comes before it in the DOM and is rendered first, the element from the child view will be used.

I need it to happen like this because my view can be recursive and changing the order of rendering is not an option.

I think this is happening because the regions seem to be grabbing their elements on ensureElement, which may happen after other views have already rendered.

Expected behavior

The elements for the regions should be picked only from the views own element, not child elements of other regions.

Seems like they could try to grab their element after the template is rendered and before any 'render' event handlers can be called.

Actual behavior

Regions will find their element from childviews of other regions if the childview was rendered first and comes first in the DOM.

Environment

See JSFiddle

paulfalgout commented 7 years ago

I'm not entirely sure what a great fix for this is going to be.

An easy one is to make your selectors more specific.. such as this for the fiddle:

regions: {
  c1: '> .child1',
  c2: '> .child2'
}

Otherwise, yes the view would need to loop through it's regions and set the el onRender before any child shows.

But Marionette is moving towards less of that, and more towards not instantiating the region at until a getRegion is called. This will also allow for regions to be fairly hidden and created on the fly where a user could this.showChildView('.some-region-selector', new MyView()); Currently you can addRegion at any time, so developers still have to be aware of scope of children.

But this isn't a problem that is unique to regions. events (and triggers kind-of) are scoped to any selector within the view tree.. so if you have:

events: {
  'click button': 'onButtonClick'
}

on a recursive view structure then the top parent is going to handle the click for itself and all of it's children.

This is also true for ui and those same keys in behavior

Something that might be worth doing.. something similar to what @scott-w suggested before.. if a region is looking for an el and gets more than 1 results, we should probably throw at least a warning if not an error, to make this issue easier to understand. But the same can't be done for ui or handlers where they could already be handling a number of elements.

As far as a bug is concerned I think this is a wontfix, but better documentation and ways of notifying the developer are probably needed.

scott-w commented 7 years ago

I've added the docs label as I agree, if we're not going to fix this at this point then we'll need to let people know about the unexpected behaviour somehow.

ambischof commented 7 years ago

Thank you for your replies, I had a feeling this would be an issue that would just need a workaround rather than an actual fix, but felt it was worth mentioning.

I think it would definitely be good to have this mentioned in the documentation. I've been using marionette for a while now and I certainly wasn't expecting this to happen.

paulfalgout commented 7 years ago

I am working on a blog post about this issue as it relates to events/triggers.. (by that I mean I wrote a title) so this is interesting context.. but @scott-w what do you think about throwing a warning or error on region if it has more than one element? I know we talked a little bit about this for views.. which is a little out of our control.. but regions isn't and I think it's a much more likely scenario than setting the el on a view.

scott-w commented 7 years ago

I'm totally in favour of this. Do we have existing mechanisms for trapping/displaying warnings that don't halt execution?

paulfalgout commented 7 years ago

So this has an open PR scheduled for v4.0, but I'm leaning towards wanting to hold off on this. I think this is going to bite people (@JSteunou) who currently have multiple regions, but the default one chosen is working for them. For all I know that's true for me as well. I think this will be better resolved by a view better understanding how to query the el for the region, rather than the region querying an el from a "parentEl" at some point. At that point, perhaps an error/warning will be important as it will much more likely alert to the same selector in the same template, not including children. In the very least I don't think we should do this for v4.0

blikblum commented 7 years ago

A good compromise is adding a warning when region has two els

marionettejs / backbone.marionette