From 63053438ea60c0375cc8ba41a23a710c8e90da8d Mon Sep 17 00:00:00 2001
From: Tobias Bosch <tbosch1009@gmail.com>
Date: Mon, 1 Dec 2014 15:18:55 -0800
Subject: [PATCH] fix(compiler): always wrap views into an own `<template>`
 element
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is needed to allow view instantiation also in browsers that
don’t support the `<template>` element and because of this would
return elements from the content of `<template>` elements when
using `element.querySelectorAll`.

Also stores the `elementBinder.nestedProtoView` correctly
when a template directive was used on a `<template>` element.
---
 .../src/compiler/pipeline/compile_control.js  | 21 +++++--
 .../src/compiler/pipeline/compile_pipeline.js | 19 +++++--
 .../src/compiler/pipeline/view_splitter.js    | 57 +++++++++++++++----
 .../test/compiler/pipeline/pipeline_spec.js   | 32 +++++++----
 .../compiler/pipeline/view_splitter_spec.js   | 30 +++++++---
 5 files changed, 116 insertions(+), 43 deletions(-)

diff --git a/modules/core/src/compiler/pipeline/compile_control.js b/modules/core/src/compiler/pipeline/compile_control.js
index f7402a1a03..6cf6e8d8d3 100644
--- a/modules/core/src/compiler/pipeline/compile_control.js
+++ b/modules/core/src/compiler/pipeline/compile_control.js
@@ -1,5 +1,6 @@
+import {isBlank} from 'facade/lang';
 import {List, ListWrapper} from 'facade/collection';
-import {DOM} from 'facade/dom';
+import {DOM, Element} from 'facade/dom';
 import {CompileElement} from './compile_element';
 import {CompileStep} from './compile_step';
 
@@ -13,12 +14,14 @@ export class CompileControl {
   _parent:CompileElement;
   _current:CompileElement;
   _results;
+  _additionalChildren;
   constructor(steps) {
     this._steps = steps;
     this._currentStepIndex = 0;
     this._parent = null;
     this._current = null;
     this._results = null;
+    this._additionalChildren = null;
   }
 
   // only public so that it can be used by compile_pipeline
@@ -39,15 +42,21 @@ export class CompileControl {
 
     this._currentStepIndex = previousStepIndex;
     this._parent = previousParent;
+
+    var localAdditionalChildren = this._additionalChildren;
+    this._additionalChildren = null;
+    return localAdditionalChildren;
   }
 
   addParent(newElement:CompileElement) {
-    var currEl = this._current.element;
-    var newEl = newElement.element;
-    DOM.parentElement(currEl).insertBefore(newEl, currEl);
-    DOM.appendChild(newEl, currEl);
-
     this.internalProcess(this._results, this._currentStepIndex+1, this._parent, newElement);
     this._parent = newElement;
   }
+
+  addChild(element:CompileElement) {
+    if (isBlank(this._additionalChildren)) {
+      this._additionalChildren = ListWrapper.create();
+    }
+    ListWrapper.push(this._additionalChildren, element);
+  }
 }
diff --git a/modules/core/src/compiler/pipeline/compile_pipeline.js b/modules/core/src/compiler/pipeline/compile_pipeline.js
index bad8f809d8..087e0dd3f0 100644
--- a/modules/core/src/compiler/pipeline/compile_pipeline.js
+++ b/modules/core/src/compiler/pipeline/compile_pipeline.js
@@ -1,3 +1,4 @@
+import {isPresent} from 'facade/lang';
 import {List, ListWrapper} from 'facade/collection';
 import {Element, Node, DOM} from 'facade/dom';
 import {CompileElement} from './compile_element';
@@ -17,18 +18,24 @@ export class CompilePipeline {
 
   process(rootElement:Element):List {
     var results = ListWrapper.create();
-    this._process(results, null, rootElement);
+    this._process(results, null, new CompileElement(rootElement));
     return results;
   }
 
-  _process(results, parent:CompileElement, element:Element) {
-    var current = new CompileElement(element);
-    this._control.internalProcess(results, 0, parent, current);
-    var childNodes = DOM.templateAwareRoot(element).childNodes;
+  _process(results, parent:CompileElement, current:CompileElement) {
+    var additionalChildren = this._control.internalProcess(results, 0, parent, current);
+
+    var childNodes = DOM.templateAwareRoot(current.element).childNodes;
     for (var i=0; i<childNodes.length; i++) {
       var node = childNodes[i];
       if (node.nodeType === Node.ELEMENT_NODE) {
-        this._process(results, current, node);
+        this._process(results, current, new CompileElement(node));
+      }
+    }
+
+    if (isPresent(additionalChildren)) {
+      for (var i=0; i<additionalChildren.length; i++) {
+        this._process(results, current, additionalChildren[i]);
       }
     }
   }
diff --git a/modules/core/src/compiler/pipeline/view_splitter.js b/modules/core/src/compiler/pipeline/view_splitter.js
index ea664151ed..c0be92ff39 100644
--- a/modules/core/src/compiler/pipeline/view_splitter.js
+++ b/modules/core/src/compiler/pipeline/view_splitter.js
@@ -1,6 +1,6 @@
 import {isBlank, isPresent} from 'facade/lang';
 import {DOM, TemplateElement} from 'facade/dom';
-import {MapWrapper, StringMapWrapper} from 'facade/collection';
+import {MapWrapper, ListWrapper} from 'facade/collection';
 
 import {Parser} from 'change_detection/parser/parser';
 
@@ -9,9 +9,20 @@ import {CompileElement} from './compile_element';
 import {CompileControl} from './compile_control';
 
 /**
- * Splits views at template directives:
- * Replaces the element with an empty <template> element that contains the
- * template directive and all property bindings needed for the template directive.
+ * Splits views at `<template>` elements or elements with `template` attribute:
+ * For `<template>` elements:
+ * - moves the content into a new and disconnected `<template>` element
+ *   that is marked as view root.
+ *
+ * For elements with a `template` attribute:
+ * - replaces the element with an empty `<template>` element,
+ *   parses the content of the `template` attribute and adds the information to that
+ *   `<template>` element. Marks the elements as view root.
+ *
+ * Note: In both cases the root of the nested view is disconnected from its parent element.
+ * This is needed for browsers that don't support the `<template>` element
+ * as we want to do locate elements with bindings using `getElementsByClassName` later on,
+ * which should not descend into the nested view.
  *
  * Fills:
  * - CompileElement#isViewRoot
@@ -26,20 +37,42 @@ export class ViewSplitter extends CompileStep {
 
   process(parent:CompileElement, current:CompileElement, control:CompileControl) {
     var element = current.element;
-    if (isBlank(parent) || (current.element instanceof TemplateElement)) {
+    if (isBlank(parent)) {
       current.isViewRoot = true;
     } else {
-      var templateBindings = MapWrapper.get(current.attrs(), 'template');
-      if (isPresent(templateBindings)) {
-        current.isViewRoot = true;
-        var templateElement = DOM.createTemplate('');
-        var newParentElement = new CompileElement(templateElement);
-        this._parseTemplateBindings(templateBindings, newParentElement);
-        control.addParent(newParentElement);
+      if (current.element instanceof TemplateElement) {
+        if (!current.isViewRoot) {
+          var viewRoot = new CompileElement(DOM.createTemplate(''));
+          this._moveChildNodes(current.element.content, viewRoot.element.content);
+          viewRoot.isViewRoot = true;
+          control.addChild(viewRoot);
+        }
+      } else {
+        var templateBindings = MapWrapper.get(current.attrs(), 'template');
+        if (isPresent(templateBindings)) {
+          var newParent = new CompileElement(DOM.createTemplate(''));
+          current.isViewRoot = true;
+          this._parseTemplateBindings(templateBindings, newParent);
+          this._addParentElement(current.element, newParent.element);
+
+          control.addParent(newParent);
+          current.element.remove();
+        }
       }
     }
   }
 
+  _moveChildNodes(source, target) {
+    while (isPresent(source.firstChild)) {
+      DOM.appendChild(target, source.firstChild);
+    }
+  }
+
+  _addParentElement(currentElement, newParentElement) {
+    DOM.parentElement(currentElement).insertBefore(newParentElement, currentElement);
+    DOM.appendChild(newParentElement, currentElement);
+  }
+
   _parseTemplateBindings(templateBindings:string, compileElement:CompileElement) {
     var bindings = this._parser.parseTemplateBindings(templateBindings);
     for (var i=0; i<bindings.length; i++) {
diff --git a/modules/core/test/compiler/pipeline/pipeline_spec.js b/modules/core/test/compiler/pipeline/pipeline_spec.js
index c9ff2e8303..f7038bc564 100644
--- a/modules/core/test/compiler/pipeline/pipeline_spec.js
+++ b/modules/core/test/compiler/pipeline/pipeline_spec.js
@@ -1,7 +1,7 @@
 import {describe, beforeEach, it, expect, iit, ddescribe} from 'test_lib/test_lib';
 import {ListWrapper, List} from 'facade/collection';
 import {DOM} from 'facade/dom';
-import {isPresent, NumberWrapper} from 'facade/lang';
+import {isPresent, NumberWrapper, StringWrapper} from 'facade/lang';
 
 import {CompilePipeline} from 'core/compiler/pipeline/compile_pipeline';
 import {CompileElement} from 'core/compiler/pipeline/compile_element';
@@ -21,16 +21,6 @@ export function main() {
     });
 
     describe('control.addParent', () => {
-      it('should wrap the underlying DOM element', () => {
-        var element = createElement('<div id="1"><span wrap0="1" id="2"><b id="3"></b></span></div>');
-        var pipeline = new CompilePipeline([
-          createWrapperStep('wrap0', [])
-        ]);
-        pipeline.process(element);
-
-        expect(DOM.getOuterHTML(element)).toEqual('<div id="1"><a id="wrap0#0"><span wrap0="1" id="2"><b id="3"></b></span></a></div>');
-      });
-
       it('should report the new parent to the following processor and the result', () => {
         var element = createElement('<div id="1"><span wrap0="1" id="2"><b id="3"></b></span></div>');
         var step0Log = [];
@@ -95,6 +85,26 @@ export function main() {
 
     });
 
+    describe('control.addChild', () => {
+      it('should report the new child to all processors and the result', () => {
+        var element = createElement('<div id="1"><div id="2"></div></div>');
+        var resultLog = [];
+        var newChild = new CompileElement(createElement('<div id="3"></div>'));
+        var pipeline = new CompilePipeline([
+          new MockStep((parent, current, control) => {
+            if (StringWrapper.equals(current.element.id, '1')) {
+              control.addChild(newChild);
+            }
+          }),
+          createLoggerStep(resultLog)
+        ]);
+        var result = pipeline.process(element);
+        expect(result[2]).toBe(newChild);
+        expect(resultLog).toEqual(['1', '1<2', '1<3']);
+        expect(resultIdLog(result)).toEqual(['1', '2', '3']);
+      });
+    });
+
   });
 }
 
diff --git a/modules/core/test/compiler/pipeline/view_splitter_spec.js b/modules/core/test/compiler/pipeline/view_splitter_spec.js
index 3913a61e83..3aea463728 100644
--- a/modules/core/test/compiler/pipeline/view_splitter_spec.js
+++ b/modules/core/test/compiler/pipeline/view_splitter_spec.js
@@ -22,21 +22,35 @@ export function main() {
       expect(results[0].isViewRoot).toBe(true);
     });
 
-    it('should mark <template> elements as viewRoot', () => {
-      var rootElement = createElement('<div><template></template></div>');
-      var results = createPipeline().process(rootElement);
-      expect(results[1].isViewRoot).toBe(true);
+    describe('<template> elements', () => {
+
+      it('should move the content into a new <template> element and mark that as viewRoot', () => {
+        var rootElement = createElement('<div><template if="true">a</template></div>');
+        var results = createPipeline().process(rootElement);
+        expect(DOM.getOuterHTML(results[1].element)).toEqual('<template if="true"></template>');
+        expect(results[1].isViewRoot).toBe(false);
+        expect(DOM.getOuterHTML(results[2].element)).toEqual('<template>a</template>');
+        expect(results[2].isViewRoot).toBe(true);
+      });
+
+      it('should not wrap a root <template> element', () => {
+        var rootElement = createElement('<div></div>');
+        var results = createPipeline().process(rootElement);
+        expect(results.length).toBe(1);
+        expect(DOM.getOuterHTML(rootElement)).toEqual('<div></div>');
+      });
+
     });
 
     describe('elements with template attribute', () => {
 
-      it('should insert an empty <template> element', () => {
-        var rootElement = createElement('<div><div template></div></div>');
+      it('should replace the element with an empty <template> element', () => {
+        var rootElement = createElement('<div><span template=""></span></div>');
         var originalChild = rootElement.childNodes[0];
         var results = createPipeline().process(rootElement);
         expect(results[0].element).toBe(rootElement);
-        expect(results[1].element instanceof TemplateElement).toBe(true);
-        expect(DOM.getInnerHTML(results[1].element)).toEqual('');
+        expect(DOM.getOuterHTML(results[0].element)).toEqual('<div><template></template></div>');
+        expect(DOM.getOuterHTML(results[2].element)).toEqual('<span template=""></span>')
         expect(results[2].element).toBe(originalChild);
       });