search

marionettejs / backbone.marionette

The Backbone Framework
https://marionettejs.com
Other
7.07k stars 1.26k forks source link

Regions hash in LayoutView attaches regions that are outside the LayoutView's DOM tree #1685

Closed jmca closed 9 years ago

jmca commented 10 years ago

The regions hash in LayoutView attaches regions that are outside of the LayoutView's DOM tree.

The expected behavior is that it should only grab the inner/scoped regions under the LayoutView DOM tree.

The example below is using el: ..., I have not tried this using an actual template. I'm using _.template('') only to thwart the "no template" exception.

<!DOCTYPE html>
<html>
<head>
    <script src="http://code.jquery.com/jquery-1.9.1.min.js"></script>
    <script src="http://jashkenas.github.io/underscore/underscore-min.js"></script>
    <script src="http://jashkenas.github.io/backbone/backbone-min.js"></script>
    <script src="http://marionettejs.com/downloads/backbone.marionette.js"></script>
</head>

<body>
    <div class="some-region"></div>
    <div class="some-view">
        <div class="some-layout-view">
        <div class="some-region">A</div>
    </div>

    <script type="text/javascript">
        var SomeLayoutView = Marionette.LayoutView.extend({
            el: '.some-view',
            template: _.template(''),
            regions: {
                someRegion: '.some-region'
            }
        });

        var someLayoutView = new SomeLayoutView();
        someLayoutView.render();

        console.log('someLayout should have 1 region');
        console.log('Actual result: ' + someLayoutView.someRegion.$el.length);
                //Returns a length of 2
    </script>       
</body>
</html>
jasonLaster commented 10 years ago

nice find

jfairbank commented 10 years ago

Need to clean up the fiddle for this, but I'm getting :sleeping:.

The behavior above is indeed happening. Thankfully, Region#show is at least preventing this from populating multiple regions with a view whether outside or inside the LayoutView. Our culprit seems to be good ole' getEl that we redefine in Region.buildRegion. It's used in the Region constructor, but we don't redefine it until after we've instantiated the Region, meaning the old implementation that doesn't take into account parentEl gets run the first time. The implementation with parentEl gets run with Region#show.

jfairbank commented 10 years ago

Pretty basic, but here's the fiddle for what's going on.

jmca commented 9 years ago

Was there any headway on this issue? It's still an issue with v2.2.1. If everybody's busy I can dig in.

jamesplease commented 9 years ago

Was there any headway on this issue?

Not that I know of, @jmca.

If everybody's busy I can dig in.

That'd be awesome!

jmca commented 9 years ago

k

samccone commented 9 years ago

yeah @jmca it was in @jfairbank 's work however that was backed out due to some breaking changes

jmca commented 9 years ago

k

jmca commented 9 years ago

I created a pull request candidate. Let me know if I'm missing anything.

jfairbank commented 9 years ago

Hey, @jmca. Thanks for taking a look at this! I've been swamped the past few weeks, so another set of eyes on this is appreciated.

jmca commented 9 years ago

My latest PR is still an in progress, but I wanted to get some feedback. After weeding through things this issue might be a use-case non-issue, but technically is an issue.

The issue:

The non-issue:

About the changes:

  1. In LayoutView's constructor I moved this._initializeRegions(options); after Marionette.ItemView.call(this, options); I'm not sure why exactly it is placed before, would love some feedback on this. When _initializeRegions is executed before the ItemView constructor call the layout's $el is not formed yet, this is done in the ItemView call. This change is the one thing that is breaking a test case.
  2. I refactored Region's getEl (non breaking according to tests) since the Region constructor first calls getEl in its original form (which queries the DOM from document level), then re-assigns getEl to use the scope of the parentEl. So, before refactor = the Region's initial $el value is every selector in the whole document, after refactor = the Region's initial $el is the correct element in the LayoutView's scope.
jfairbank commented 9 years ago

Unfortunately, I don't think that that will work. Refactoring getEl and moving _initializeRegions was exactly what I did in my previous PR #1705, which broke Marionette. Others have brought up use cases where they need access to the regions in their LayoutView initialize method. If we move _initializeRegions after the ItemView constructor call, then the regions aren't available in initialize.

Please keep sounding your ideas. I would love to see a BC fix for this. Any feedback @marionettejs/marionette-core?

jamesplease commented 9 years ago

I've put thought into this before, and I only ever came to the conclusion it was only possible to do all of the refactors I wanted in a v3 non-BC way.

It's been awhile since I've studied it, but it seems like that might still be the way to go. v3 isn't that far off, relatively. In 3 weeks, once 2.3 comes out, we'll be able to start merging v3 ideas into a branch. Maybe we just wait til then?

samccone commented 9 years ago

it is not a questions

do all of the refactors I wanted

rather how can we fix this one bad bug

jamesplease commented 9 years ago

Ha, I know. I meant to convey that this bug fix necessarily requires breaking refactors from what I could tell.

jmca commented 9 years ago

@jfairbank How does/would refactoring getEl break the current build?

jmca commented 9 years ago

@jfairbank

Others have brought up use cases where they need access to the regions in their LayoutView initialize method

I'm curious what the use case would be, mainly because the correct Region's $el would not be available before View init. And since Region's are view based, they should probably be messed with in the View's onRender method.

Also we could keep the Region init before the View init, then re-init all Regions $el after View init. Not the most streamlined but it should also not break anything.

jmca commented 9 years ago

Another thought on the above comment, should a LayoutView's regions really be available in its constructor? Initialization methods assume the DOM is not constructed. While the onRender events (or at the end of the render method) assume that the DOM is constructed and available.

jmca commented 9 years ago

I created a work-around for now that "shouldn't" break anything.

jamesplease commented 9 years ago

Awesome @jmca! Excited to see. In the future, you should comment on the PR if you have a comment about the PR. It saves us time having to go figure out the PR that you're talking about :P

jmca commented 9 years ago

K. For some reason PR thread comments disappear when a re-pull happens. I'll keep comments there in the future though.

stephanebachelier commented 9 years ago

@jmca @samccone @jmeas The idea of my PR as stated is to force the scope after when the layout has been rendered so we have a valid $el. @jmca fix did not work for me.

I had the same issue has described by @jmca.

My use case is :

jamesplease commented 9 years ago

Fixed by #2149

fabien commented 8 years ago

@jmeas it appears that in the end it's still not possible to have scoping unless el is already defined (e.g. the view has been rendered). Am I missing something?

jamesplease commented 8 years ago

@fabien what do you mean? Can you provide more info?

fabien commented 8 years ago

Well, unless the LayoutView has already been rendered, the parentEl that is passed in here will be undefined - which is always the case for template based layouts:

https://github.com/marionettejs/backbone.marionette/blob/master/src/layout-view.js#L107

However, if I take out this this._isFirstRender clause, and let this._reinitializeRegions run, it will work fine, and the region selector will be correctly scoped to the LayoutView:

https://github.com/marionettejs/backbone.marionette/blob/master/src/layout-view.js#L39

jamesplease commented 8 years ago

Make a new issue? //cc @megawac

fabien commented 8 years ago

@jmeas I don't have the time right now, sorry.

My current workaround to have the regions correctly scoped and available when I need them:

initialize: function(options) {
    this.once('render', this.setupRegions);
},

setupRegions: function() {
    this.addRegion('container', {
        selector: '[data-region="container"]',
        regionClass: ContainerRegion
    });
    this.addRegion('details', '[data-region="details"]');
}
sllanes commented 8 years ago

I ran into the same issue as @fabien. It seems the parentEl is not set if using a LayoutView with a template because this.el is not yet initialized. I was able to workaround the issue by overriding the LayoutView constructor and moving the call to _initializeRegions after the call to the Marionette.ItemView constructor. This allows this.el to be initialized now that Backbone.View._ensureElement is called first. Changing the order does not appear to break anything in our project but I did notice the comment in the source that states // Ensure the regions are available when the 'initialize' method is called so my change would break that expectation. I have not had a need to use regions in my initialize method and just mostly in the "before:show" or "render" view events so as of now I am okay with the change. I would be interested in any thoughts or other suggestions.