From 64ad74acbea7b4c54e1de5ef3872f76a4a26da5c Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Fri, 17 Apr 2015 20:37:23 -0700 Subject: [PATCH] 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 #1416 --- .../emulated_unscoped_shadow_dom_strategy.js | 1 - .../src/render/dom/shadow_dom/light_dom.js | 15 +-- ...mulated_scoped_shadow_dom_strategy_spec.js | 7 +- ...lated_unscoped_shadow_dom_strategy_spec.js | 7 +- .../render/dom/shadow_dom/light_dom_spec.js | 26 ++++- .../shadow_dom_emulation_integration_spec.js | 97 ++++++++++++++++++- 6 files changed, 135 insertions(+), 18 deletions(-) diff --git a/modules/angular2/src/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy.js b/modules/angular2/src/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy.js index 63cf1f5319..a3dc2e43ec 100644 --- a/modules/angular2/src/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy.js +++ b/modules/angular2/src/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy.js @@ -34,7 +34,6 @@ export class EmulatedUnscopedShadowDomStrategy extends ShadowDomStrategy { } attachTemplate(el, view:viewModule.RenderView) { - DOM.clearNodes(el); moveViewNodesIntoParent(el, view); } diff --git a/modules/angular2/src/render/dom/shadow_dom/light_dom.js b/modules/angular2/src/render/dom/shadow_dom/light_dom.js index 584ef37e42..1ba7fa6afa 100644 --- a/modules/angular2/src/render/dom/shadow_dom/light_dom.js +++ b/modules/angular2/src/render/dom/shadow_dom/light_dom.js @@ -37,10 +37,7 @@ export class LightDom { } redistribute() { - var tags = this.contentTags(); - if (tags.length > 0) { - redistributeNodes(tags, this.expandedDomNodes()); - } + redistributeNodes(this.contentTags(), this.expandedDomNodes()); } contentTags(): List { @@ -122,16 +119,22 @@ function redistributeNodes(contents:List, nodes:List) { for (var i = 0; i < contents.length; ++i) { var content = contents[i]; var select = content.select; - var matchSelector = (n) => DOM.elementMatches(n, select); // Empty selector is identical to if (select.length === 0) { - content.insert(nodes); + content.insert(ListWrapper.clone(nodes)); ListWrapper.clear(nodes); } else { + var matchSelector = (n) => DOM.elementMatches(n, select); var matchingNodes = ListWrapper.filter(nodes, matchSelector); content.insert(matchingNodes); ListWrapper.removeAll(nodes, matchingNodes); } } + for (var i = 0; i < nodes.length; i++) { + var node = nodes[i]; + if (isPresent(node.parentNode)) { + DOM.remove(nodes[i]); + } + } } diff --git a/modules/angular2/test/render/dom/shadow_dom/emulated_scoped_shadow_dom_strategy_spec.js b/modules/angular2/test/render/dom/shadow_dom/emulated_scoped_shadow_dom_strategy_spec.js index 298fcf0586..0541d37eda 100644 --- a/modules/angular2/test/render/dom/shadow_dom/emulated_scoped_shadow_dom_strategy_spec.js +++ b/modules/angular2/test/render/dom/shadow_dom/emulated_scoped_shadow_dom_strategy_spec.js @@ -46,14 +46,13 @@ export function main() { it('should attach the view nodes as child of the host element', () => { var host = el('
original content
'); + var originalChild = DOM.childNodes(host)[0]; var nodes = el('
view
'); var view = new RenderView(null, [nodes], [], [], []); strategy.attachTemplate(host, view); - var firstChild = DOM.firstChild(host); - expect(DOM.tagName(firstChild).toLowerCase()).toEqual('div'); - expect(firstChild).toHaveText('view'); - expect(host).toHaveText('view'); + expect(DOM.childNodes(host)[0]).toBe(originalChild); + expect(DOM.childNodes(host)[1]).toBe(nodes); }); it('should rewrite style urls', () => { diff --git a/modules/angular2/test/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy_spec.js b/modules/angular2/test/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy_spec.js index e6875dbb56..aec3d76b0a 100644 --- a/modules/angular2/test/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy_spec.js +++ b/modules/angular2/test/render/dom/shadow_dom/emulated_unscoped_shadow_dom_strategy_spec.js @@ -41,14 +41,13 @@ export function main() { it('should attach the view nodes as child of the host element', () => { var host = el('
original content
'); + var originalChild = DOM.childNodes(host)[0]; var nodes = el('
view
'); var view = new RenderView(null, [nodes], [], [], []); strategy.attachTemplate(host, view); - var firstChild = DOM.firstChild(host); - expect(DOM.tagName(firstChild).toLowerCase()).toEqual('div'); - expect(firstChild).toHaveText('view'); - expect(host).toHaveText('view'); + expect(DOM.childNodes(host)[0]).toBe(originalChild); + expect(DOM.childNodes(host)[1]).toBe(nodes); }); it('should rewrite style urls', () => { diff --git a/modules/angular2/test/render/dom/shadow_dom/light_dom_spec.js b/modules/angular2/test/render/dom/shadow_dom/light_dom_spec.js index 5af2843cda..c1928bb68d 100644 --- a/modules/angular2/test/render/dom/shadow_dom/light_dom_spec.js +++ b/modules/angular2/test/render/dom/shadow_dom/light_dom_spec.js @@ -84,7 +84,7 @@ class FakeContentTag { } insert(nodes){ - this._nodes = ListWrapper.clone(nodes); + this._nodes = nodes; } nodes() { @@ -215,6 +215,30 @@ export function main() { expect(toHtml(wildcard.nodes())).toEqual(["1", "2", "3"]); expect(toHtml(contentB.nodes())).toEqual([]); }); + + it("should remove all nodes if there are no content tags", () => { + var lightDomEl = el("
123
") + + var lightDom = new LightDom(lightDomView, new FakeView([]), lightDomEl); + + lightDom.redistribute(); + + expect(DOM.childNodes(lightDomEl).length).toBe(0); + }); + + it("should remove all not projected nodes", () => { + var lightDomEl = el("
123
"); + var bNode = DOM.childNodes(lightDomEl)[1]; + + var lightDom = new LightDom(lightDomView, new FakeView([ + new FakeContentTag(null, "a") + ]), lightDomEl); + + lightDom.redistribute(); + + expect(bNode.parentNode).toBe(null); + }); + }); }); } diff --git a/modules/angular2/test/render/dom/shadow_dom_emulation_integration_spec.js b/modules/angular2/test/render/dom/shadow_dom_emulation_integration_spec.js index 4c1e139903..a9bfb4d7dd 100644 --- a/modules/angular2/test/render/dom/shadow_dom_emulation_integration_spec.js +++ b/modules/angular2/test/render/dom/shadow_dom_emulation_integration_spec.js @@ -54,10 +54,11 @@ export function main() { var testbed, renderer, rootEl, compile, compileRoot; - function createRenderer({templates}) { + function createRenderer({templates, viewCacheCapacity}) { testbed = new IntegrationTestbed({ shadowDomStrategy: strategyFactory(), - templates: ListWrapper.concat(templates, componentTemplates) + templates: ListWrapper.concat(templates, componentTemplates), + viewCacheCapacity: viewCacheCapacity }); renderer = testbed.renderer; compileRoot = (rootEl) => testbed.compileRoot(rootEl); @@ -87,6 +88,25 @@ export function main() { }); })); + it('should not show the light dom event if there is not content tag', inject([AsyncTestCompleter], (async) => { + createRenderer({ + templates: [new ViewDefinition({ + componentId: 'main', + template: '' + + '
A
' + + '', + directives: [empty] + })] + }); + compileRoot('main').then( (pv) => { + renderer.createInPlaceHostView(null, rootEl, pv.render); + + expect(rootEl).toHaveText(''); + + async.done(); + }); + })); + it('should support dynamic components', inject([AsyncTestCompleter], (async) => { createRenderer({ templates: [new ViewDefinition({ @@ -289,6 +309,46 @@ export function main() { }); })); + it("should support tabs with view caching", inject([AsyncTestCompleter], (async) => { + createRenderer({ + templates: [new ViewDefinition({ + componentId: 'main', + template: + '(0'+ + '1'+ + '2)', + directives: [tabComponent] + })], + viewCacheCapacity: 5 + }); + compileRoot('main').then( (pv) => { + var viewRefs = renderer.createInPlaceHostView(null, rootEl, pv.render); + var vcRef0 = new ViewContainerRef(viewRefs[2], 0); + var vcRef1 = new ViewContainerRef(viewRefs[3], 0); + var vcRef2 = new ViewContainerRef(viewRefs[4], 0); + var mainPv = pv.elementBinders[0].nestedProtoView; + var pvRef = mainPv.elementBinders[0].nestedProtoView.elementBinders[0].nestedProtoView.render; + + expect(rootEl).toHaveText('()'); + + renderer.createViewInContainer(vcRef0, 0, pvRef); + + expect(rootEl).toHaveText('(TAB(0))'); + + renderer.destroyViewInContainer(vcRef0, 0); + renderer.createViewInContainer(vcRef1, 0, pvRef); + + expect(rootEl).toHaveText('(TAB(1))'); + + renderer.destroyViewInContainer(vcRef1, 0); + renderer.createViewInContainer(vcRef2, 0, pvRef); + + expect(rootEl).toHaveText('(TAB(2))'); + + async.done(); + }); + })); + //Implement once NgElement support changing a class //it("should redistribute when a class has been added or removed"); //it('should not lose focus', () => { @@ -318,6 +378,12 @@ var simple = new DirectiveMetadata({ type: DirectiveMetadata.COMPONENT_TYPE }); +var empty = new DirectiveMetadata({ + selector: 'empty', + id: 'empty', + type: DirectiveMetadata.COMPONENT_TYPE +}); + var dynamicComponent = new DirectiveMetadata({ selector: 'dynamic', id: 'dynamic', @@ -372,12 +438,29 @@ var autoViewportDirective = new DirectiveMetadata({ type: DirectiveMetadata.VIEWPORT_TYPE }); +var tabGroupComponent = new DirectiveMetadata({ + selector: 'tab-group', + id: 'tab-group', + type: DirectiveMetadata.COMPONENT_TYPE +}); + +var tabComponent = new DirectiveMetadata({ + selector: 'tab', + id: 'tab', + type: DirectiveMetadata.COMPONENT_TYPE +}); + var componentTemplates = [ new ViewDefinition({ componentId: 'simple', template: 'SIMPLE()', directives: [] }), + new ViewDefinition({ + componentId: 'empty', + template: '', + directives: [] + }), new ViewDefinition({ componentId: 'multiple-content-tags', template: '(, )', @@ -407,5 +490,15 @@ var componentTemplates = [ componentId: 'conditional-content', template: '
(
, )
', directives: [autoViewportDirective] + }), + new ViewDefinition({ + componentId: 'tab-group', + template: 'GROUP()', + directives: [] + }), + new ViewDefinition({ + componentId: 'tab', + template: '
TAB()
', + directives: [autoViewportDirective] }) ];