From 0f0191518dc81cf6eff424f8566df159be0aabac Mon Sep 17 00:00:00 2001
From: Paul Falgout <paul@otterball.com>
Date: Tue, 15 Nov 2016 16:07:13 +0900
Subject: [PATCH] Don't detach pre-existing HTML when view's el is already in
 the region
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Resolves #3227

This PR also moves some of `Region.show` into a `_setupChildView` function which mirrors what is happening in a CollectionView currently and helps reduce the length of the `show` function which grew to handle the should empty check.

In the specs, there was a failing test regarding “when setting the "replaceElement" class option”.  I believe this test was failing because it was reusing `this.view` which wasn’t cleaned up in a prior test.  So effectively `this.view` was currently already shown in another region.  I switched this to `this.view1` which is instantiated as part of this `describe` block (or closer at least) and should be better for these tests.
---
 src/region.js            | 29 ++++++++++++++++++-----------
 test/unit/region.spec.js | 24 ++++++++++++++++++++----
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/src/region.js b/src/region.js
index d99475730e..f66b7052d1 100644
--- a/src/region.js
+++ b/src/region.js
@@ -57,25 +57,34 @@ const Region = MarionetteObject.extend({
 
     this.triggerMethod('before:show', this, view, options);
 
-    monitorViewEvents(view);
-
-    this.empty(options);
-
-    // We need to listen for if a view is destroyed in a way other than through the region.
-    // If this happens we need to remove the reference to the currentView since once a view
-    // has been destroyed we can not reuse it.
-    view.on('destroy', this._empty, this);
+    // Assume an attached view is already in the region for pre-existing DOM
+    if (!view._isAttached) {
+      this.empty(options);
+    }
 
-    this._proxyChildViewEvents(view);
+    this._setupChildView(view);
 
     this._renderView(view);
 
     this._attachView(view, options);
 
+    this.currentView = view;
+
     this.triggerMethod('show', this, view, options);
     return this;
   },
 
+  _setupChildView(view) {
+    monitorViewEvents(view);
+
+    this._proxyChildViewEvents(view);
+
+    // We need to listen for if a view is destroyed in a way other than through the region.
+    // If this happens we need to remove the reference to the currentView since once a view
+    // has been destroyed we can not reuse it.
+    view.on('destroy', this._empty, this);
+  },
+
   _proxyChildViewEvents(view) {
     const parentView = this._parentView;
 
@@ -119,8 +128,6 @@ const Region = MarionetteObject.extend({
       view._isAttached = true;
       triggerMethodOn(view, 'attach', view);
     }
-
-    this.currentView = view;
   },
 
   _ensureElement(options = {}) {
diff --git a/test/unit/region.spec.js b/test/unit/region.spec.js
index 617eb8e713..680984fff3 100644
--- a/test/unit/region.spec.js
+++ b/test/unit/region.spec.js
@@ -293,7 +293,7 @@ describe('region', function() {
             this.regionHtml = this.$parentEl.html();
             this.showOptions = {preventDestroy: true};
             this.region.replaceElement = true;
-            this.region.show(this.view, this.showOptions);
+            this.region.show(this.view1, this.showOptions);
           });
 
           it('should have replaced the "el"', function() {
@@ -301,7 +301,7 @@ describe('region', function() {
           });
 
           it('should append the view HTML to the parent "el"', function() {
-            expect(this.$parentEl).to.contain.$html(this.view.$el.html());
+            expect(this.$parentEl).to.contain.$html(this.view1.$el.html());
           });
 
           it('should remove the region\'s "el" from the DOM', function() {
@@ -319,7 +319,7 @@ describe('region', function() {
           });
 
           it('should not restore if the "currentView.el" has been remove from the DOM', function() {
-            this.view.remove();
+            this.view1.remove();
             this.region._restoreEl();
             expect(this.region.currentView.el.parentNode).is.falsy;
           });
@@ -330,7 +330,7 @@ describe('region', function() {
             });
 
             it('should remove the view from the parent', function() {
-              expect(this.$parentEl).to.not.contain.$html(this.view.$el.html());
+              expect(this.$parentEl).to.not.contain.$html(this.view1.$el.html());
             });
 
             it('should restore the region\'s "el" to the DOM', function() {
@@ -547,6 +547,22 @@ describe('region', function() {
     });
   });
 
+  describe('when a view is already attached and shown in a region', function() {
+    beforeEach(function() {
+      this.setFixtures('<div id="region"><div id="view">Foo</div></div>');
+      this.myRegion = new Marionette.Region({
+        el: '#region'
+      });
+      this.sinon.spy(this.myRegion, 'empty');
+
+      this.myRegion.show(new Marionette.View({ el: '#view' }));
+    });
+
+    it('should not empty the region', function() {
+      expect(this.myRegion.empty).to.not.have.been.called;
+    });
+  });
+
   describe('when a view is already shown and showing another', function() {
     beforeEach(function() {
       this.MyRegion = Backbone.Marionette.Region.extend({