Skip to content

Commit 64ad74a

Browse files
tboschtbosch
committed
fix(shadowdom): remove unused nodes on redistribute
Previously, light dom nodes that were not used by any content tag were not removed from a view on redistribute. This lead to a bug when reusing a view from the view pool, as it still contained stale reprojected nodes. Fixes angular#1416
1 parent 02997f4 commit 64ad74a

File tree

6 files changed

+135
-18
lines changed

6 files changed

+135
-18
lines changed

‎modules/angular2/src/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ export class EmulatedUnscopedShadowDomStrategy extends ShadowDomStrategy {
3434
}
3535

3636
attachTemplate(el, view:viewModule.RenderView) {
37-
DOM.clearNodes(el);
3837
moveViewNodesIntoParent(el, view);
3938
}
4039

‎modules/angular2/src/render/dom/shadow_dom/light_dom.js‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,7 @@ export class LightDom {
3737
}
3838

3939
redistribute() {
40-
var tags = this.contentTags();
41-
if (tags.length > 0) {
42-
redistributeNodes(tags, this.expandedDomNodes());
43-
}
40+
redistributeNodes(this.contentTags(), this.expandedDomNodes());
4441
}
4542

4643
contentTags(): List<Content> {
@@ -122,16 +119,22 @@ function redistributeNodes(contents:List<Content>, nodes:List) {
122119
for (var i = 0; i < contents.length; ++i) {
123120
var content = contents[i];
124121
var select = content.select;
125-
var matchSelector = (n) => DOM.elementMatches(n, select);
126122

127123
// Empty selector is identical to <content/>
128124
if (select.length === 0) {
129-
content.insert(nodes);
125+
content.insert(ListWrapper.clone(nodes));
130126
ListWrapper.clear(nodes);
131127
} else {
128+
var matchSelector = (n) => DOM.elementMatches(n, select);
132129
var matchingNodes = ListWrapper.filter(nodes, matchSelector);
133130
content.insert(matchingNodes);
134131
ListWrapper.removeAll(nodes, matchingNodes);
135132
}
136133
}
134+
for (var i = 0; i < nodes.length; i++) {
135+
var node = nodes[i];
136+
if (isPresent(node.parentNode)) {
137+
DOM.remove(nodes[i]);
138+
}
139+
}
137140
}

‎modules/angular2/test/render/dom/shadow_dom/emulated_scoped_shadow_dom_strategy_spec.js‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,14 +46,13 @@ export function main() {
4646

4747
it('should attach the view nodes as child of the host element', () => {
4848
var host = el('<div><span>original content</span></div>');
49+
var originalChild = DOM.childNodes(host)[0];
4950
var nodes = el('<div>view</div>');
5051
var view = new RenderView(null, [nodes], [], [], []);
5152

5253
strategy.attachTemplate(host, view);
53-
var firstChild = DOM.firstChild(host);
54-
expect(DOM.tagName(firstChild).toLowerCase()).toEqual('div');
55-
expect(firstChild).toHaveText('view');
56-
expect(host).toHaveText('view');
54+
expect(DOM.childNodes(host)[0]).toBe(originalChild);
55+
expect(DOM.childNodes(host)[1]).toBe(nodes);
5756
});
5857

5958
it('should rewrite style urls', () => {

‎modules/angular2/test/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy_spec.js‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,14 +41,13 @@ export function main() {
4141

4242
it('should attach the view nodes as child of the host element', () => {
4343
var host = el('<div><span>original content</span></div>');
44+
var originalChild = DOM.childNodes(host)[0];
4445
var nodes = el('<div>view</div>');
4546
var view = new RenderView(null, [nodes], [], [], []);
4647

4748
strategy.attachTemplate(host, view);
48-
var firstChild = DOM.firstChild(host);
49-
expect(DOM.tagName(firstChild).toLowerCase()).toEqual('div');
50-
expect(firstChild).toHaveText('view');
51-
expect(host).toHaveText('view');
49+
expect(DOM.childNodes(host)[0]).toBe(originalChild);
50+
expect(DOM.childNodes(host)[1]).toBe(nodes);
5251
});
5352

5453
it('should rewrite style urls', () => {

‎modules/angular2/test/render/dom/shadow_dom/light_dom_spec.js‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ class FakeContentTag {
8484
}
8585

8686
insert(nodes){
87-
this._nodes = ListWrapper.clone(nodes);
87+
this._nodes = nodes;
8888
}
8989

9090
nodes() {
@@ -215,6 +215,30 @@ export function main() {
215215
expect(toHtml(wildcard.nodes())).toEqual(["<a>1</a>", "<b>2</b>", "<a>3</a>"]);
216216
expect(toHtml(contentB.nodes())).toEqual([]);
217217
});
218+
219+
it("should remove all nodes if there are no content tags", () => {
220+
var lightDomEl = el("<div><a>1</a><b>2</b><a>3</a></div>")
221+
222+
var lightDom = new LightDom(lightDomView, new FakeView([]), lightDomEl);
223+
224+
lightDom.redistribute();
225+
226+
expect(DOM.childNodes(lightDomEl).length).toBe(0);
227+
});
228+
229+
it("should remove all not projected nodes", () => {
230+
var lightDomEl = el("<div><a>1</a><b>2</b><a>3</a></div>");
231+
var bNode = DOM.childNodes(lightDomEl)[1];
232+
233+
var lightDom = new LightDom(lightDomView, new FakeView([
234+
new FakeContentTag(null, "a")
235+
]), lightDomEl);
236+
237+
lightDom.redistribute();
238+
239+
expect(bNode.parentNode).toBe(null);
240+
});
241+
218242
});
219243
});
220244
}

‎modules/angular2/test/render/dom/shadow_dom_emulation_integration_spec.js‎

Lines changed: 95 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,11 @@ export function main() {
5454

5555
var testbed, renderer, rootEl, compile, compileRoot;
5656

57-
function createRenderer({templates}) {
57+
function createRenderer({templates, viewCacheCapacity}) {
5858
testbed = new IntegrationTestbed({
5959
shadowDomStrategy: strategyFactory(),
60-
templates: ListWrapper.concat(templates, componentTemplates)
60+
templates: ListWrapper.concat(templates, componentTemplates),
61+
viewCacheCapacity: viewCacheCapacity
6162
});
6263
renderer = testbed.renderer;
6364
compileRoot = (rootEl) => testbed.compileRoot(rootEl);
@@ -87,6 +88,25 @@ export function main() {
8788
});
8889
}));
8990

91+
it('should not show the light dom event if there is not content tag', inject([AsyncTestCompleter], (async) => {
92+
createRenderer({
93+
templates: [new ViewDefinition({
94+
componentId: 'main',
95+
template: '<empty>' +
96+
'<div>A</div>' +
97+
'</empty>',
98+
directives: [empty]
99+
})]
100+
});
101+
compileRoot('main').then( (pv) => {
102+
renderer.createInPlaceHostView(null, rootEl, pv.render);
103+
104+
expect(rootEl).toHaveText('');
105+
106+
async.done();
107+
});
108+
}));
109+
90110
it('should support dynamic components', inject([AsyncTestCompleter], (async) => {
91111
createRenderer({
92112
templates: [new ViewDefinition({
@@ -289,6 +309,46 @@ export function main() {
289309
});
290310
}));
291311

312+
it("should support tabs with view caching", inject([AsyncTestCompleter], (async) => {
313+
createRenderer({
314+
templates: [new ViewDefinition({
315+
componentId: 'main',
316+
template:
317+
'(<tab><span>0</span></tab>'+
318+
'<tab><span>1</span></tab>'+
319+
'<tab><span>2</span></tab>)',
320+
directives: [tabComponent]
321+
})],
322+
viewCacheCapacity: 5
323+
});
324+
compileRoot('main').then( (pv) => {
325+
var viewRefs = renderer.createInPlaceHostView(null, rootEl, pv.render);
326+
var vcRef0 = new ViewContainerRef(viewRefs[2], 0);
327+
var vcRef1 = new ViewContainerRef(viewRefs[3], 0);
328+
var vcRef2 = new ViewContainerRef(viewRefs[4], 0);
329+
var mainPv = pv.elementBinders[0].nestedProtoView;
330+
var pvRef = mainPv.elementBinders[0].nestedProtoView.elementBinders[0].nestedProtoView.render;
331+
332+
expect(rootEl).toHaveText('()');
333+
334+
renderer.createViewInContainer(vcRef0, 0, pvRef);
335+
336+
expect(rootEl).toHaveText('(TAB(0))');
337+
338+
renderer.destroyViewInContainer(vcRef0, 0);
339+
renderer.createViewInContainer(vcRef1, 0, pvRef);
340+
341+
expect(rootEl).toHaveText('(TAB(1))');
342+
343+
renderer.destroyViewInContainer(vcRef1, 0);
344+
renderer.createViewInContainer(vcRef2, 0, pvRef);
345+
346+
expect(rootEl).toHaveText('(TAB(2))');
347+
348+
async.done();
349+
});
350+
}));
351+
292352
//Implement once NgElement support changing a class
293353
//it("should redistribute when a class has been added or removed");
294354
//it('should not lose focus', () => {
@@ -318,6 +378,12 @@ var simple = new DirectiveMetadata({
318378
type: DirectiveMetadata.COMPONENT_TYPE
319379
});
320380

381+
var empty = new DirectiveMetadata({
382+
selector: 'empty',
383+
id: 'empty',
384+
type: DirectiveMetadata.COMPONENT_TYPE
385+
});
386+
321387
var dynamicComponent = new DirectiveMetadata({
322388
selector: 'dynamic',
323389
id: 'dynamic',
@@ -372,12 +438,29 @@ var autoViewportDirective = new DirectiveMetadata({
372438
type: DirectiveMetadata.VIEWPORT_TYPE
373439
});
374440

441+
var tabGroupComponent = new DirectiveMetadata({
442+
selector: 'tab-group',
443+
id: 'tab-group',
444+
type: DirectiveMetadata.COMPONENT_TYPE
445+
});
446+
447+
var tabComponent = new DirectiveMetadata({
448+
selector: 'tab',
449+
id: 'tab',
450+
type: DirectiveMetadata.COMPONENT_TYPE
451+
});
452+
375453
var componentTemplates = [
376454
new ViewDefinition({
377455
componentId: 'simple',
378456
template: 'SIMPLE(<content></content>)',
379457
directives: []
380458
}),
459+
new ViewDefinition({
460+
componentId: 'empty',
461+
template: '',
462+
directives: []
463+
}),
381464
new ViewDefinition({
382465
componentId: 'multiple-content-tags',
383466
template: '(<content select=".left"></content>, <content></content>)',
@@ -407,5 +490,15 @@ var componentTemplates = [
407490
componentId: 'conditional-content',
408491
template: '<div>(<div *auto="cond"><content select=".left"></content></div>, <content></content>)</div>',
409492
directives: [autoViewportDirective]
493+
}),
494+
new ViewDefinition({
495+
componentId: 'tab-group',
496+
template: 'GROUP(<content></content>)',
497+
directives: []
498+
}),
499+
new ViewDefinition({
500+
componentId: 'tab',
501+
template: '<div><div *auto="cond">TAB(<content></content>)</div></div>',
502+
directives: [autoViewportDirective]
410503
})
411504
];

0 commit comments

Comments
 (0)