fix(core): call lifecycle hooks for siblings in declaration order

modules/@angular/core/src/view/element.ts

@@ -31,7 +31,6 @@ export function anchorDef(
   return {
     // will bet set by the view definition
     index: undefined,
-    reverseChildIndex: undefined,
     parent: undefined,
     renderParent: undefined,
     bindingIndex: undefined,
@@ -39,6 +38,7 @@ export function anchorDef(
     // regular values
     flags,
     childFlags: 0,
+    directChildFlags: 0,
     childMatchedQueries: 0, matchedQueries, matchedQueryIds, references, ngContentIndex, childCount,
     bindings: [],
     outputs: [],
@@ -131,7 +131,6 @@ export function elementDef(
   return {
     // will bet set by the view definition
     index: undefined,
-    reverseChildIndex: undefined,
     parent: undefined,
     renderParent: undefined,
     bindingIndex: undefined,
@@ -139,6 +138,7 @@ export function elementDef(
     // regular values
     flags,
     childFlags: 0,
+    directChildFlags: 0,
     childMatchedQueries: 0, matchedQueries, matchedQueryIds, references, ngContentIndex, childCount,
     bindings: bindingDefs,
     outputs: outputDefs,

modules/@angular/core/src/view/ng_content.ts

@@ -13,7 +13,6 @@ export function ngContentDef(ngContentIndex: number, index: number): NodeDef {
   return {
     // will bet set by the view definition
     index: undefined,
-    reverseChildIndex: undefined,
     parent: undefined,
     renderParent: undefined,
     bindingIndex: undefined,
@@ -21,6 +20,7 @@ export function ngContentDef(ngContentIndex: number, index: number): NodeDef {
     // regular values
     flags: NodeFlags.TypeNgContent,
     childFlags: 0,
+    directChildFlags: 0,
     childMatchedQueries: 0,
     matchedQueries: {},
     matchedQueryIds: 0,

modules/@angular/core/src/view/provider.ts

@@ -94,7 +94,6 @@ export function _def(
   return {
     // will bet set by the view definition
     index: undefined,
-    reverseChildIndex: undefined,
     parent: undefined,
     renderParent: undefined,
     bindingIndex: undefined,
@@ -102,6 +101,7 @@ export function _def(
     // regular values
     flags,
     childFlags: 0,
+    directChildFlags: 0,
     childMatchedQueries: 0, matchedQueries, matchedQueryIds, references,
     ngContentIndex: undefined, childCount, bindings, outputs,
     element: undefined,
@@ -432,25 +432,43 @@ export function callLifecycleHooksChildrenFirst(view: ViewData, lifecycles: Node
   if (!(view.def.nodeFlags & lifecycles)) {
     return;
   }
-  const len = view.def.nodes.length;
+  const nodes = view.def.nodes;
-  for (let i = 0; i < len; i++) {
+  for (let i = 0; i < nodes.length; i++) {
-    // We use the reverse child oreder to call providers of children first.
+    const nodeDef = nodes[i];
-    const nodeDef = view.def.reverseChildNodes[i];
+    let parent = nodeDef.parent;
-    const nodeIndex = nodeDef.index;
+    if (!parent && nodeDef.flags & lifecycles) {
-    if (nodeDef.flags & lifecycles) {
+      // matching root node (e.g. a pipe)
-      // a leaf
+      callProviderLifecycles(view, i, nodeDef.flags & lifecycles);
-      Services.setCurrentNode(view, nodeIndex);
+    }
-      callProviderLifecycles(asProviderData(view, nodeIndex).instance, nodeDef.flags & lifecycles);
+    if ((nodeDef.childFlags & lifecycles) === 0) {
-    } else if ((nodeDef.childFlags & lifecycles) === 0) {
+      // no child matches one of the lifecycles
-      // a parent with leafs
-      // no child matches one of the lifecycles,
-      // then skip the children
       i += nodeDef.childCount;
     }
+    while (parent && (parent.flags & NodeFlags.TypeElement) &&
+           i === parent.index + parent.childCount) {
+      // last child of an element
+      if (parent.directChildFlags & lifecycles) {
+        callElementProvidersLifecycles(view, parent, lifecycles);
+      }
+      parent = parent.parent;
+    }
   }
 }
 
-function callProviderLifecycles(provider: any, lifecycles: NodeFlags) {
+function callElementProvidersLifecycles(view: ViewData, elDef: NodeDef, lifecycles: NodeFlags) {
+  for (let i = elDef.index + 1; i <= elDef.index + elDef.childCount; i++) {
+    const nodeDef = view.def.nodes[i];
+    if (nodeDef.flags & lifecycles) {
+      callProviderLifecycles(view, i, nodeDef.flags & lifecycles);
+    }
+    // only visit direct children
+    i += nodeDef.childCount;
+  }
+}
+
+function callProviderLifecycles(view: ViewData, index: number, lifecycles: NodeFlags) {
+  const provider = asProviderData(view, index).instance;
+  Services.setCurrentNode(view, index);
   if (lifecycles & NodeFlags.AfterContentInit) {
     provider.ngAfterContentInit();
   }

modules/@angular/core/src/view/pure_expression.ts

@@ -38,7 +38,6 @@ function _pureExpressionDef(flags: NodeFlags, propertyNames: string[]): NodeDef
   return {
     // will bet set by the view definition
     index: undefined,
-    reverseChildIndex: undefined,
     parent: undefined,
     renderParent: undefined,
     bindingIndex: undefined,
@@ -46,6 +45,7 @@ function _pureExpressionDef(flags: NodeFlags, propertyNames: string[]): NodeDef
     // regular values
     flags,
     childFlags: 0,
+    directChildFlags: 0,
     childMatchedQueries: 0,
     matchedQueries: {},
     matchedQueryIds: 0,

modules/@angular/core/src/view/query.ts

@@ -26,7 +26,6 @@ export function queryDef(
   return {
     // will bet set by the view definition
     index: undefined,
-    reverseChildIndex: undefined,
     parent: undefined,
     renderParent: undefined,
     bindingIndex: undefined,
@@ -34,6 +33,7 @@ export function queryDef(
     // regular values
     flags,
     childFlags: 0,
+    directChildFlags: 0,
     childMatchedQueries: 0,
     ngContentIndex: undefined,
     matchedQueries: {},

modules/@angular/core/src/view/text.ts

@@ -30,7 +30,6 @@ export function textDef(ngContentIndex: number, constants: string[]): NodeDef {
   return {
     // will bet set by the view definition
     index: undefined,
-    reverseChildIndex: undefined,
     parent: undefined,
     renderParent: undefined,
     bindingIndex: undefined,
@@ -38,6 +37,7 @@ export function textDef(ngContentIndex: number, constants: string[]): NodeDef {
     // regular values
     flags,
     childFlags: 0,
+    directChildFlags: 0,
     childMatchedQueries: 0,
     matchedQueries: {},
     matchedQueryIds: 0,

modules/@angular/core/src/view/types.ts

@@ -33,11 +33,7 @@ export interface ViewDefinition {
   nodes: NodeDef[];
   /** aggregated NodeFlags for all nodes **/
   nodeFlags: NodeFlags;
-  /**
+  rootNodeFlags: NodeFlags;
-   * Order: parents before children, but children in reverse order.
-   * Especially providers are after elements / anchors.
-   */
-  reverseChildNodes: NodeDef[];
   lastRenderRootNode: NodeDef;
   bindingCount: number;
   outputCount: number;
@@ -83,15 +79,16 @@ export const enum ViewFlags {
 export interface NodeDef {
   flags: NodeFlags;
   index: number;
-  reverseChildIndex: number;
   parent: NodeDef;
   renderParent: NodeDef;
   /** this is checked against NgContentDef.index to find matched nodes */
   ngContentIndex: number;
   /** number of transitive children */
   childCount: number;
-  /** aggregated NodeFlags for all children (does not include self) **/
+  /** aggregated NodeFlags for all transitive children (does not include self) **/
   childFlags: NodeFlags;
+  /** aggregated NodeFlags for all direct children (does not include self) **/
+  directChildFlags: NodeFlags;
 
   bindingIndex: number;
   bindings: BindingDef[];

modules/@angular/core/src/view/view.ts

@@ -33,6 +33,7 @@ export function viewDef(
   let viewBindingCount = 0;
   let viewDisposableCount = 0;
   let viewNodeFlags = 0;
+  let viewRootNodeFlags = 0;
   let viewMatchedQueries = 0;
   let currentParent: NodeDef = null;
   let currentElementHasPublicProviders = false;
@@ -52,8 +53,6 @@ export function viewDef(
     node.parent = currentParent;
     node.bindingIndex = viewBindingCount;
     node.outputIndex = viewDisposableCount;
-    node.reverseChildIndex =
-        calculateReverseChildIndex(currentParent, i, node.childCount, nodes.length);
 
     // renderParent needs to account for ng-container!
     let currentRenderParent: NodeDef;
@@ -74,7 +73,6 @@ export function viewDef(
       currentElementHasPublicProviders = false;
       currentElementHasPrivateProviders = false;
     }
-    reverseChildNodes[node.reverseChildIndex] = node;
     validateNode(currentParent, node, nodes.length);
 
     viewNodeFlags |= node.flags;
@@ -84,10 +82,13 @@ export function viewDef(
     }
     if (currentParent) {
       currentParent.childFlags |= node.flags;
+      currentParent.directChildFlags |= node.flags;
       currentParent.childMatchedQueries |= node.matchedQueryIds;
       if (node.element && node.element.template) {
         currentParent.childMatchedQueries |= node.element.template.nodeMatchedQueries;
       }
+    } else {
+      viewRootNodeFlags |= node.flags;
     }
 
     viewBindingCount += node.bindings.length;
@@ -136,8 +137,9 @@ export function viewDef(
       nodes[nodeIndex].element.handleEvent(view, eventName, event);
   return {
     nodeFlags: viewNodeFlags,
+    rootNodeFlags: viewRootNodeFlags,
     nodeMatchedQueries: viewMatchedQueries, flags,
-    nodes: nodes, reverseChildNodes,
+    nodes: nodes,
     updateDirectives: updateDirectives || NOOP,
     updateRenderer: updateRenderer || NOOP,
     handleEvent: handleEvent || NOOP,
@@ -146,47 +148,6 @@ export function viewDef(
   };
 }
 
-
-
-function calculateReverseChildIndex(
-    currentParent: NodeDef, i: number, childCount: number, nodeCount: number) {
-  // Notes about reverse child order:
-  // - Every node is directly before its children, in dfs and reverse child order.
-  // - node.childCount contains all children, in dfs and reverse child order.
-  // - In dfs order, every node is before its first child
-  // - In reverse child order, every node is before its last child
-
-  // Algorithm, main idea:
-  // - In reverse child order, the ranges for each child + its transitive children are mirrored
-  //   regarding their position inside of their parent
-
-  // Visualization:
-  // Given the following tree:
-  // Nodes: n0
-  //             n1         n2
-  //                n11 n12    n21 n22
-  // dfs:    0   1   2   3  4   5   6
-  // result: 0   4   6   5  1   3   2
-  //
-  // Example:
-  // Current node = 1
-  // 1) lastChildIndex = 3
-  // 2) lastChildOffsetRelativeToParentInDfsOrder = 2
-  // 3) parentEndIndexInReverseChildOrder = 6
-  // 4) result = 4
-  let lastChildOffsetRelativeToParentInDfsOrder: number;
-  let parentEndIndexInReverseChildOrder: number;
-  if (currentParent) {
-    const lastChildIndex = i + childCount;
-    lastChildOffsetRelativeToParentInDfsOrder = lastChildIndex - currentParent.index - 1;
-    parentEndIndexInReverseChildOrder = currentParent.reverseChildIndex + currentParent.childCount;
-  } else {
-    lastChildOffsetRelativeToParentInDfsOrder = i + childCount;
-    parentEndIndexInReverseChildOrder = nodeCount - 1;
-  }
-  return parentEndIndexInReverseChildOrder - lastChildOffsetRelativeToParentInDfsOrder;
-}
-
 function validateNode(parent: NodeDef, node: NodeDef, nodeCount: number) {
   const template = node.element && node.element.template;
   if (template) {

modules/@angular/core/test/linker/change_detection_integration_spec.ts

@@ -906,12 +906,13 @@ function createTests({viewEngine}: {viewEngine: boolean}) {
         it('should be called in reverse order so the child is always notified before the parent',
            fakeAsync(() => {
              const ctx = createCompFixture(
-                 '<div testDirective="parent"><div testDirective="child"></div></div>');
+                 '<div testDirective="parent"><div testDirective="child"></div></div><div testDirective="sibling"></div>');
 
              ctx.detectChanges(false);
 
              expect(directiveLog.filter(['ngAfterContentChecked'])).toEqual([
-               'child.ngAfterContentChecked', 'parent.ngAfterContentChecked'
+               'child.ngAfterContentChecked', 'parent.ngAfterContentChecked',
+               'sibling.ngAfterContentChecked'
              ]);
            }));
       });
@@ -1018,12 +1019,12 @@ function createTests({viewEngine}: {viewEngine: boolean}) {
         it('should be called in reverse order so the child is always notified before the parent',
            fakeAsync(() => {
              const ctx = createCompFixture(
-                 '<div testDirective="parent"><div testDirective="child"></div></div>');
+                 '<div testDirective="parent"><div testDirective="child"></div></div><div testDirective="sibling"></div>');
 
              ctx.detectChanges(false);
 
              expect(directiveLog.filter(['ngAfterViewChecked'])).toEqual([
-               'child.ngAfterViewChecked', 'parent.ngAfterViewChecked'
+               'child.ngAfterViewChecked', 'parent.ngAfterViewChecked', 'sibling.ngAfterViewChecked'
              ]);
            }));
       });
@@ -1061,13 +1062,13 @@ function createTests({viewEngine}: {viewEngine: boolean}) {
         it('should be called in reverse order so the child is always notified before the parent',
            fakeAsync(() => {
              const ctx = createCompFixture(
-                 '<div testDirective="parent"><div testDirective="child"></div></div>');
+                 '<div testDirective="parent"><div testDirective="child"></div></div><div testDirective="sibling"></div>');
 
              ctx.detectChanges(false);
              ctx.destroy();
 
              expect(directiveLog.filter(['ngOnDestroy'])).toEqual([
-               'child.ngOnDestroy', 'parent.ngOnDestroy'
+               'child.ngOnDestroy', 'parent.ngOnDestroy', 'sibling.ngOnDestroy'
              ]);
            }));
 

modules/@angular/core/test/view/view_def_spec.ts

@@ -11,69 +11,6 @@ import {filterQueryId} from '@angular/core/src/view/util';
 
 export function main() {
   describe('viewDef', () => {
-    describe('reverseChild order', () => {
-      function reverseChildOrder(viewDef: ViewDefinition): number[] {
-        return viewDef.reverseChildNodes.map(node => node.index);
-      }
-
-      it('should reverse child order for root nodes', () => {
-        const vd = viewDef(ViewFlags.None, [
-          textDef(null, ['a']),  // level 0, index 0
-          textDef(null, ['a']),  // level 0, index 0
-        ]);
-
-        expect(reverseChildOrder(vd)).toEqual([1, 0]);
-      });
-
-      it('should reverse child order for one level, one root', () => {
-        const vd = viewDef(ViewFlags.None, [
-          elementDef(NodeFlags.None, null, null, 2, 'span'),  // level 0, index 0
-          textDef(null, ['a']),                               // level 1, index 1
-          textDef(null, ['a']),                               // level 1, index 2
-        ]);
-
-        expect(reverseChildOrder(vd)).toEqual([0, 2, 1]);
-      });
-
-      it('should reverse child order for 1 level, 2 roots', () => {
-        const vd = viewDef(ViewFlags.None, [
-          elementDef(NodeFlags.None, null, null, 2, 'span'),  // level 0, index 0
-          textDef(null, ['a']),                               // level 1, index 1
-          textDef(null, ['a']),                               // level 1, index 2
-          elementDef(NodeFlags.None, null, null, 1, 'span'),  // level 0, index 3
-          textDef(null, ['a']),                               // level 1, index 4
-        ]);
-
-        expect(reverseChildOrder(vd)).toEqual([3, 4, 0, 2, 1]);
-      });
-
-      it('should reverse child order for 2 levels', () => {
-        const vd = viewDef(ViewFlags.None, [
-          elementDef(NodeFlags.None, null, null, 4, 'span'),  // level 0, index 0
-          elementDef(NodeFlags.None, null, null, 1, 'span'),  // level 1, index 1
-          textDef(null, ['a']),                               // level 2, index 2
-          elementDef(NodeFlags.None, null, null, 1, 'span'),  // level 1, index 3
-          textDef(null, ['a']),                               // level 2, index 4
-        ]);
-
-        expect(reverseChildOrder(vd)).toEqual([0, 3, 4, 1, 2]);
-      });
-
-      it('should reverse child order for mixed levels', () => {
-        const vd = viewDef(ViewFlags.None, [
-          textDef(null, ['a']),                               // level 0, index 0
-          elementDef(NodeFlags.None, null, null, 5, 'span'),  // level 0, index 1
-          textDef(null, ['a']),                               // level 1, index 2
-          elementDef(NodeFlags.None, null, null, 1, 'span'),  // level 1, index 3
-          textDef(null, ['a']),                               // level 2, index 4
-          elementDef(NodeFlags.None, null, null, 1, 'span'),  // level 1, index 5
-          textDef(null, ['a']),                               // level 2, index 6
-          textDef(null, ['a']),                               // level 0, index 7
-        ]);
-
-        expect(reverseChildOrder(vd)).toEqual([7, 1, 5, 6, 3, 4, 2, 0]);
-      });
-    });
 
     describe('parent', () => {
       function parents(viewDef: ViewDefinition): number[] {
@@ -122,6 +59,10 @@ export function main() {
         return viewDef.nodes.map(node => node.childFlags);
       }
 
+      function directChildFlags(viewDef: ViewDefinition): number[] {
+        return viewDef.nodes.map(node => node.directChildFlags);
+      }
+
       it('should calculate childFlags for one level', () => {
         const vd = viewDef(ViewFlags.None, [
           elementDef(NodeFlags.None, null, null, 1, 'span'),
@@ -131,6 +72,10 @@ export function main() {
         expect(childFlags(vd)).toEqual([
           NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, NodeFlags.None
         ]);
+
+        expect(directChildFlags(vd)).toEqual([
+          NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, NodeFlags.None
+        ]);
       });
 
       it('should calculate childFlags for two levels', () => {
@@ -144,6 +89,11 @@ export function main() {
           NodeFlags.TypeElement | NodeFlags.TypeDirective | NodeFlags.AfterContentChecked,
           NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, NodeFlags.None
         ]);
+
+        expect(directChildFlags(vd)).toEqual([
+          NodeFlags.TypeElement, NodeFlags.TypeDirective | NodeFlags.AfterContentChecked,
+          NodeFlags.None
+        ]);
       });
 
       it('should calculate childFlags for one level, multiple roots', () => {
@@ -160,6 +110,12 @@ export function main() {
           NodeFlags.TypeDirective | NodeFlags.AfterContentInit | NodeFlags.AfterViewChecked,
           NodeFlags.None, NodeFlags.None
         ]);
+
+        expect(directChildFlags(vd)).toEqual([
+          NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, NodeFlags.None,
+          NodeFlags.TypeDirective | NodeFlags.AfterContentInit | NodeFlags.AfterViewChecked,
+          NodeFlags.None, NodeFlags.None
+        ]);
       });
 
       it('should calculate childFlags for multiple levels', () => {
@@ -178,6 +134,13 @@ export function main() {
           NodeFlags.TypeDirective | NodeFlags.AfterContentInit | NodeFlags.AfterViewInit,
           NodeFlags.None, NodeFlags.None
         ]);
+
+        expect(directChildFlags(vd)).toEqual([
+          NodeFlags.TypeElement, NodeFlags.TypeDirective | NodeFlags.AfterContentChecked,
+          NodeFlags.None,
+          NodeFlags.TypeDirective | NodeFlags.AfterContentInit | NodeFlags.AfterViewInit,
+          NodeFlags.None, NodeFlags.None
+        ]);
       });
     });