Fw 1559 #33542

mhevery · 2019-11-02T04:13:52Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

https://angular-team.atlassian.net/browse/FW-1559

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Under some circumstances inserting `LView` into `LContainer` would get inserted in a wrong DOM location resulting in incorrect DOM node order. These two use cases were corrected: - Inserting in front of empty `LView` ``` <ng-template></ng-template>` ``` - Inserting in front of nested `Lview` ``` <ng-template> <ng-template [ngIf]="true">Nested</ng-template> </ng-template>` ```

pkozlowski-opensource · 2019-11-02T07:45:06Z

packages/core/src/render3/node_manipulation.ts

      }
-      return getNativeByTNodeOrNull(tViewNodeChild, lView);
+      return getNativeByTNode(tNode, lView);


I think that it should be recursive in nature - what might happen is that a child of ElementContainer / IcuContainer is a container which might have views etc.

pkozlowski-opensource · 2019-11-02T07:46:31Z

packages/core/src/render3/node_manipulation.ts

+      const childLContainer = lView[tNode.index] as LContainer;
+      ngDevMode && assertLContainer(childLContainer);
+      return getFirstRNodeFromLViewInLContainer(childLContainer, 0);
+    } else {


I don't see the projection handled here, but it might show up at the root of a view, right? Plus, projection might have a container at the root so it is recursive in nature.

pkozlowski-opensource

@mhevery I did the first, quick pass on this PR and I think that there are more cases to cover here. Essentially the function you are modifying is almost the same as collecting root nodes (

angular/packages/core/src/render3/view_ref.ts

Lines 305 to 348 in ac9d044

    
           function collectNativeNodes(lView: LView, tNode: TNode | null, result: any[]): any[] { 
        
             while (tNode !== null) { 
        
               ngDevMode && assertNodeOfPossibleTypes( 
        
                                tNode, TNodeType.Element, TNodeType.Container, TNodeType.Projection, 
        
                                TNodeType.ElementContainer, TNodeType.IcuContainer); 
        
               const lNode = lView[tNode.index]; 
        
               if (lNode !== null) { 
        
                 result.push(unwrapRNode(lNode)); 
        
               } 
        
               // A given lNode can represent either a native node or a LContainer (when it is a host of a 
        
               // ViewContainerRef). When we find a LContainer we need to descend into it to collect root nodes 
        
               // from the views in this container. 
        
               if (isLContainer(lNode)) { 
        
                 for (let i = CONTAINER_HEADER_OFFSET; i < lNode.length; i++) { 
        
                   const lViewInAContainer = lNode[i]; 
        
                   const lViewFirstChildTNode = lViewInAContainer[TVIEW].firstChild; 
        
                   if (lViewFirstChildTNode !== null) { 
        
                     collectNativeNodes(lViewInAContainer, lViewFirstChildTNode, result); 
        
                   } 
        
                 } 
        
               } 
        
               const tNodeType = tNode.type; 
        
               if (tNodeType === TNodeType.ElementContainer || tNodeType === TNodeType.IcuContainer) { 
        
                 collectNativeNodes(lView, tNode.child, result); 
        
               } else if (tNodeType === TNodeType.Projection) { 
        
                 const componentView = findComponentView(lView); 
        
                 const componentHost = componentView[T_HOST] as TElementNode; 
        
                 const parentView = getLViewParent(componentView); 
        
                 let currentProjectedNode: TNode|null = 
        
                     (componentHost.projection as(TNode | null)[])[tNode.projection as number]; 
        
                 while (currentProjectedNode !== null && parentView !== null) { 
        
                   result.push(getNativeByTNode(currentProjectedNode, parentView)); 
        
                   currentProjectedNode = currentProjectedNode.next; 
        
                 } 
        
               } 
        
               tNode = tNode.next; 
        
             } 
        
             return result; 
        
           }

) - the way I understand it is that only difference is that we can stop at the first result.

More generally, I think that we can fold all those root node traversal routines we've got into one - we've got already applyView - we would have to "just" add a new tree walk action (collect).

I think that, in this PR, we should:

add more tests / asserts for all the node types (need to cover the same cases as in collectNativeNodes linked above);
add / modify implementation that covers all the cases.

Then, with tests in place and in a separate PR, refactor code to have the root nodes tree walking in one place only (should drop quite a bit of code duplication!). I've spent quite a bit of time poking around the code so happy to discuss this in more details!

mhevery · 2019-11-05T06:42:45Z

@pkozlowski-opensource I have refactored it to use existing code in 9465a38, but as you can see the aesthetics of it are not great. :-( My current thinking is that we should refactor applyView and friends into a cursor pattern. We could than just iterate over the cursor as many times as we need to to either add/remove or to retrieve the first item from the list. What do you think?

pkozlowski-opensource · 2019-11-05T07:01:57Z

packages/core/test/acceptance/view_insertion_spec.ts

@@ -249,4 +277,150 @@ describe('view insertion', () => {
      expect(fixture.debugElement.queryAll(By.css('div.dynamic')).length).toBe(4);
    });
  });
+
+  describe('regression', () => {
+    it('should allow inserting of all different types', () => {


Could we split this test into several ones (one test for each case, similarly to what I did in #33493? )? It is super-hard to follow-as is.

pkozlowski-opensource · 2019-11-05T07:05:12Z

packages/core/test/acceptance/view_insertion_spec.ts

@@ -249,4 +277,150 @@ describe('view insertion', () => {
      expect(fixture.debugElement.queryAll(By.css('div.dynamic')).length).toBe(4);
    });
  });
+
+  describe('regression', () => {


nits regarding naming:

regression -> non-regression? but more generally, I would have sth more descriptive, like "should allow inserting views before views with different node types";

"should allow inserting of all different types" - the essence of those tests is that we want to insert before a view with specific nodes layout and it should be reflected in the name.

Generally speaking, I would love to have this test split into multiple ones (see below)

mhevery · 2019-11-06T22:37:45Z

@pkozlowski-opensource took over this one here #33627

angular-automatic-lock-bot · 2019-12-07T16:04:46Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mhevery requested a review from a team as a code owner November 2, 2019 04:13

googlebot added the cla: yes label Nov 2, 2019

mhevery force-pushed the fw-1559 branch from b00708e to c4738a5 Compare November 2, 2019 04:14

pkozlowski-opensource reviewed Nov 2, 2019

View reviewed changes

mhevery added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Nov 2, 2019

pkozlowski-opensource added the comp: ivy label Nov 2, 2019

ngbot bot added this to the needsTriage milestone Nov 2, 2019

pkozlowski-opensource requested changes Nov 2, 2019

View reviewed changes

mhevery added 3 commits November 4, 2019 13:26

fixup! fix(ivy): handle cases of inserting LView into LContainer

1739c55

fixup! fix(ivy): handle cases of inserting LView into LContainer

6cbd41a

fixup! fix(ivy): handle cases of inserting LView into LContainer

9465a38

mhevery force-pushed the fw-1559 branch from 06c0811 to 9465a38 Compare November 5, 2019 06:40

pkozlowski-opensource reviewed Nov 5, 2019

View reviewed changes

mhevery closed this Nov 6, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fw 1559 #33542

Fw 1559 #33542

mhevery commented Nov 2, 2019

pkozlowski-opensource Nov 2, 2019

pkozlowski-opensource Nov 2, 2019 •

edited

pkozlowski-opensource left a comment

mhevery commented Nov 5, 2019

pkozlowski-opensource Nov 5, 2019

pkozlowski-opensource Nov 5, 2019

mhevery commented Nov 6, 2019

angular-automatic-lock-bot bot commented Dec 7, 2019

	function collectNativeNodes(lView: LView, tNode: TNode \| null, result: any[]): any[] {
	while (tNode !== null) {
	ngDevMode && assertNodeOfPossibleTypes(
	tNode, TNodeType.Element, TNodeType.Container, TNodeType.Projection,
	TNodeType.ElementContainer, TNodeType.IcuContainer);

	const lNode = lView[tNode.index];
	if (lNode !== null) {
	result.push(unwrapRNode(lNode));
	}

	// A given lNode can represent either a native node or a LContainer (when it is a host of a
	// ViewContainerRef). When we find a LContainer we need to descend into it to collect root nodes
	// from the views in this container.
	if (isLContainer(lNode)) {
	for (let i = CONTAINER_HEADER_OFFSET; i < lNode.length; i++) {
	const lViewInAContainer = lNode[i];
	const lViewFirstChildTNode = lViewInAContainer[TVIEW].firstChild;
	if (lViewFirstChildTNode !== null) {
	collectNativeNodes(lViewInAContainer, lViewFirstChildTNode, result);
	}
	}
	}

	const tNodeType = tNode.type;
	if (tNodeType === TNodeType.ElementContainer \|\| tNodeType === TNodeType.IcuContainer) {
	collectNativeNodes(lView, tNode.child, result);
	} else if (tNodeType === TNodeType.Projection) {
	const componentView = findComponentView(lView);
	const componentHost = componentView[T_HOST] as TElementNode;
	const parentView = getLViewParent(componentView);
	let currentProjectedNode: TNode\|null =
	(componentHost.projection as(TNode \| null)[])[tNode.projection as number];

	while (currentProjectedNode !== null && parentView !== null) {
	result.push(getNativeByTNode(currentProjectedNode, parentView));
	currentProjectedNode = currentProjectedNode.next;
	}
	}
	tNode = tNode.next;
	}

	return result;
	}

Fw 1559 #33542

Fw 1559 #33542

Conversation

mhevery commented Nov 2, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

pkozlowski-opensource Nov 2, 2019

Choose a reason for hiding this comment

pkozlowski-opensource Nov 2, 2019 • edited

Choose a reason for hiding this comment

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

mhevery commented Nov 5, 2019

pkozlowski-opensource Nov 5, 2019

Choose a reason for hiding this comment

pkozlowski-opensource Nov 5, 2019

Choose a reason for hiding this comment

mhevery commented Nov 6, 2019

angular-automatic-lock-bot bot commented Dec 7, 2019

pkozlowski-opensource Nov 2, 2019 •

edited