From 7db93310f104ec92a7d8012b9849b7d266a92d59 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Mon, 20 Feb 2017 14:34:15 -0800 Subject: [PATCH] fix: view engine - fix some corner cases --- .../src/view_compiler_next/view_compiler.ts | 8 ++++- modules/@angular/core/src/view/refs.ts | 28 +++++++++++----- modules/@angular/core/src/view/types.ts | 2 +- modules/@angular/core/src/view/util.ts | 2 +- modules/@angular/core/src/view/view.ts | 14 +++++--- modules/@angular/core/src/view/view_attach.ts | 9 +++-- .../platform-browser/src/dom/dom_renderer.ts | 3 +- .../platform-server/src/server_renderer.ts | 33 +++++++++++++++---- .../platform-server/test/integration_spec.ts | 23 +++++++++++-- 9 files changed, 93 insertions(+), 29 deletions(-) diff --git a/modules/@angular/compiler/src/view_compiler_next/view_compiler.ts b/modules/@angular/compiler/src/view_compiler_next/view_compiler.ts index 0e2aad9c67..125f133e3c 100644 --- a/modules/@angular/compiler/src/view_compiler_next/view_compiler.ts +++ b/modules/@angular/compiler/src/view_compiler_next/view_compiler.ts @@ -294,9 +294,12 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver, BuiltinConverter if (hostBindings.length) { this._addUpdateExpressions(nodeIndex, hostBindings, this.updateRendererExpressions); } - inputDefs = elementBindingDefs(ast.inputs); + // Note: inputDefs have to be in the same order as hostBindings: + // - first the entries from the directives, then the ones from the element. ast.directives.forEach( (dirAst, dirIndex) => inputDefs.push(...elementBindingDefs(dirAst.hostProperties))); + inputDefs.push(...elementBindingDefs(ast.inputs)); + outputDefs = usedEvents.map(([target, eventName]) => { return target ? o.literalArr([o.literal(target), o.literal(eventName)]) : o.literal(eventName); @@ -847,6 +850,9 @@ function needsAdditionalRootNode(ast: TemplateAst): boolean { } if (ast instanceof ElementAst) { + if (ast.name === NG_CONTAINER_TAG && ast.children.length) { + return needsAdditionalRootNode(ast.children[ast.children.length - 1]); + } return ast.hasViewContainer; } diff --git a/modules/@angular/core/src/view/refs.ts b/modules/@angular/core/src/view/refs.ts index 01740a973d..7d77a2b740 100644 --- a/modules/@angular/core/src/view/refs.ts +++ b/modules/@angular/core/src/view/refs.ts @@ -103,10 +103,15 @@ class ViewContainerRef_ implements ViewContainerRef { } } - get(index: number): ViewRef { - const ref = new ViewRef_(this._data.embeddedViews[index]); - ref.attachToViewContainerRef(this); - return ref; + get(index: number): ViewRef { return this._getViewRef(this._data.embeddedViews[index]); } + + private _getViewRef(view: ViewData) { + if (view) { + const ref = new ViewRef_(view); + ref.attachToViewContainerRef(this); + return ref; + } + return null; } get length(): number { return this._data.embeddedViews.length; }; @@ -147,14 +152,19 @@ class ViewContainerRef_ implements ViewContainerRef { remove(index?: number): void { const viewData = Services.detachEmbeddedView(this._data, index); - Services.destroyView(viewData); + if (viewData) { + Services.destroyView(viewData); + } } detach(index?: number): ViewRef { - const view = this.get(index); - Services.detachEmbeddedView(this._data, index); - (view as ViewRef_).detachFromContainer(); - return view; + const view = Services.detachEmbeddedView(this._data, index); + if (view) { + const viewRef = this._getViewRef(view); + viewRef.detachFromContainer(); + return viewRef; + } + return null; } } diff --git a/modules/@angular/core/src/view/types.ts b/modules/@angular/core/src/view/types.ts index bda960afd9..92c6b25980 100644 --- a/modules/@angular/core/src/view/types.ts +++ b/modules/@angular/core/src/view/types.ts @@ -38,7 +38,7 @@ export interface ViewDefinition { * Especially providers are after elements / anchors. */ reverseChildNodes: NodeDef[]; - lastRootNode: NodeDef; + lastRenderRootNode: NodeDef; bindingCount: number; disposableCount: number; /** diff --git a/modules/@angular/core/src/view/util.ts b/modules/@angular/core/src/view/util.ts index c0d8b4a6be..d116269d4d 100644 --- a/modules/@angular/core/src/view/util.ts +++ b/modules/@angular/core/src/view/util.ts @@ -230,7 +230,7 @@ export function visitRootRenderNodes( view: ViewData, action: RenderNodeAction, parentNode: any, nextSibling: any, target: any[]) { // We need to re-compute the parent node in case the nodes have been moved around manually if (action === RenderNodeAction.RemoveChild) { - parentNode = view.renderer.parentNode(renderNode(view, view.def.lastRootNode)); + parentNode = view.renderer.parentNode(renderNode(view, view.def.lastRenderRootNode)); } visitSiblingRenderNodes( view, action, 0, view.def.nodes.length - 1, parentNode, nextSibling, target); diff --git a/modules/@angular/core/src/view/view.ts b/modules/@angular/core/src/view/view.ts index 9521339efc..61e6f32f7a 100644 --- a/modules/@angular/core/src/view/view.ts +++ b/modules/@angular/core/src/view/view.ts @@ -37,7 +37,7 @@ export function viewDef( let currentParent: NodeDef = null; let currentElementHasPublicProviders = false; let currentElementHasPrivateProviders = false; - let lastRootNode: NodeDef = null; + let lastRenderRootNode: NodeDef = null; for (let i = 0; i < nodes.length; i++) { while (currentParent && i > currentParent.index + currentParent.childCount) { const newParent = currentParent.parent; @@ -92,8 +92,8 @@ export function viewDef( viewBindingCount += node.bindings.length; viewDisposableCount += node.disposableCount; - if (!currentRenderParent) { - lastRootNode = node; + if (!currentRenderParent && (node.type === NodeType.Element || node.type === NodeType.Text)) { + lastRenderRootNode = node; } if (node.type === NodeType.Provider || node.type === NodeType.Directive) { if (!currentElementHasPublicProviders) { @@ -141,7 +141,7 @@ export function viewDef( updateRenderer: updateRenderer || NOOP, handleEvent: handleEvent || NOOP, bindingCount: viewBindingCount, - disposableCount: viewDisposableCount, lastRootNode + disposableCount: viewDisposableCount, lastRenderRootNode }; } @@ -189,7 +189,8 @@ function calculateReverseChildIndex( function validateNode(parent: NodeDef, node: NodeDef, nodeCount: number) { const template = node.element && node.element.template; if (template) { - if (template.lastRootNode && template.lastRootNode.flags & NodeFlags.HasEmbeddedViews) { + if (template.lastRenderRootNode && + template.lastRenderRootNode.flags & NodeFlags.HasEmbeddedViews) { throw new Error( `Illegal State: Last root node of a template can't have embedded views, at index ${node.index}!`); } @@ -476,6 +477,9 @@ function checkNoChangesQuery(view: ViewData, nodeDef: NodeDef) { } export function destroyView(view: ViewData) { + if (view.state & ViewState.Destroyed) { + return; + } execEmbeddedViewsAction(view, ViewAction.Destroy); execComponentViewsAction(view, ViewAction.Destroy); callLifecycleHooksChildrenFirst(view, NodeFlags.OnDestroy); diff --git a/modules/@angular/core/src/view/view_attach.ts b/modules/@angular/core/src/view/view_attach.ts index 2cb7239df6..c7e1e01ed3 100644 --- a/modules/@angular/core/src/view/view_attach.ts +++ b/modules/@angular/core/src/view/view_attach.ts @@ -33,8 +33,11 @@ export function attachEmbeddedView(elementData: ElementData, viewIndex: number, export function detachEmbeddedView(elementData: ElementData, viewIndex: number): ViewData { const embeddedViews = elementData.embeddedViews; - if (viewIndex == null) { - viewIndex = embeddedViews.length; + if (viewIndex == null || viewIndex >= embeddedViews.length) { + viewIndex = embeddedViews.length - 1; + } + if (viewIndex < 0) { + return null; } const view = embeddedViews[viewIndex]; removeFromArray(embeddedViews, viewIndex); @@ -76,7 +79,7 @@ export function moveEmbeddedView( function renderAttachEmbeddedView(elementData: ElementData, prevView: ViewData, view: ViewData) { const prevRenderNode = - prevView ? renderNode(prevView, prevView.def.lastRootNode) : elementData.renderElement; + prevView ? renderNode(prevView, prevView.def.lastRenderRootNode) : elementData.renderElement; const parentNode = view.renderer.parentNode(prevRenderNode); const nextSibling = view.renderer.nextSibling(prevRenderNode); // Note: We can't check if `nextSibling` is present, as on WebWorkers it will always be! diff --git a/modules/@angular/platform-browser/src/dom/dom_renderer.ts b/modules/@angular/platform-browser/src/dom/dom_renderer.ts index b140c3c00d..32d4eb9b25 100644 --- a/modules/@angular/platform-browser/src/dom/dom_renderer.ts +++ b/modules/@angular/platform-browser/src/dom/dom_renderer.ts @@ -19,7 +19,8 @@ import {DomSharedStylesHost} from './shared_styles_host'; export const NAMESPACE_URIS: {[ns: string]: string} = { 'xlink': 'http://www.w3.org/1999/xlink', 'svg': 'http://www.w3.org/2000/svg', - 'xhtml': 'http://www.w3.org/1999/xhtml' + 'xhtml': 'http://www.w3.org/1999/xhtml', + 'xml': 'http://www.w3.org/XML/1998/namespace' }; const TEMPLATE_COMMENT_TEXT = 'template bindings={}'; const TEMPLATE_BINDINGS_EXP = /^template bindings=(.*)$/; diff --git a/modules/@angular/platform-server/src/server_renderer.ts b/modules/@angular/platform-server/src/server_renderer.ts index 5364d1bfcf..a014bc91e9 100644 --- a/modules/@angular/platform-server/src/server_renderer.ts +++ b/modules/@angular/platform-server/src/server_renderer.ts @@ -266,11 +266,12 @@ function appendNodes(parent: any, nodes: any) { export class ServerRendererFactoryV2 implements RendererFactoryV2 { private rendererByCompId = new Map(); private defaultRenderer: RendererV2; + private schema = new DomElementSchemaRegistry(); constructor( private ngZone: NgZone, @Inject(DOCUMENT) private document: any, private sharedStylesHost: SharedStylesHost) { - this.defaultRenderer = new DefaultServerRendererV2(document, ngZone); + this.defaultRenderer = new DefaultServerRendererV2(document, ngZone, this.schema); }; createRenderer(element: any, type: RendererTypeV2): RendererV2 { @@ -282,7 +283,7 @@ export class ServerRendererFactoryV2 implements RendererFactoryV2 { let renderer = this.rendererByCompId.get(type.id); if (!renderer) { renderer = new EmulatedEncapsulationServerRendererV2( - this.document, this.ngZone, this.sharedStylesHost, type); + this.document, this.ngZone, this.sharedStylesHost, this.schema, type); this.rendererByCompId.set(type.id, renderer); } (renderer).applyToHost(element); @@ -303,7 +304,8 @@ export class ServerRendererFactoryV2 implements RendererFactoryV2 { } class DefaultServerRendererV2 implements RendererV2 { - constructor(private document: any, private ngZone: NgZone) {} + constructor( + private document: any, private ngZone: NgZone, private schema: DomElementSchemaRegistry) {} destroy(): void {} @@ -382,7 +384,26 @@ class DefaultServerRendererV2 implements RendererV2 { getDOM().removeStyle(el, style); } - setProperty(el: any, name: string, value: any): void { getDOM().setProperty(el, name, value); } + // The value was validated already as a property binding, against the property name. + // To know this value is safe to use as an attribute, the security context of the + // attribute with the given name is checked against that security context of the + // property. + private _isSafeToReflectProperty(tagName: string, propertyName: string): boolean { + return this.schema.securityContext(tagName, propertyName, true) === + this.schema.securityContext(tagName, propertyName, false); + } + + setProperty(el: any, name: string, value: any): void { + getDOM().setProperty(el, name, value); + // Mirror property values for known HTML element properties in the attributes. + const tagName = (el.tagName as string).toLowerCase(); + if (isPresent(value) && (typeof value === 'number' || typeof value == 'string') && + this.schema.hasElement(tagName, EMPTY_ARRAY) && + this.schema.hasProperty(tagName, name, EMPTY_ARRAY) && + this._isSafeToReflectProperty(tagName, name)) { + this.setAttribute(el, name, value.toString()); + } + } setValue(node: any, value: string): void { getDOM().setText(node, value); } @@ -404,8 +425,8 @@ class EmulatedEncapsulationServerRendererV2 extends DefaultServerRendererV2 { constructor( document: any, ngZone: NgZone, sharedStylesHost: SharedStylesHost, - private component: RendererTypeV2) { - super(document, ngZone); + schema: DomElementSchemaRegistry, private component: RendererTypeV2) { + super(document, ngZone, schema); const styles = flattenStyles(component.id, component.styles, []); sharedStylesHost.addStyles(styles); diff --git a/modules/@angular/platform-server/test/integration_spec.ts b/modules/@angular/platform-server/test/integration_spec.ts index 57219e142b..de85bda9f2 100644 --- a/modules/@angular/platform-server/test/integration_spec.ts +++ b/modules/@angular/platform-server/test/integration_spec.ts @@ -7,14 +7,14 @@ */ import {PlatformLocation} from '@angular/common'; +import {USE_VIEW_ENGINE} from '@angular/compiler/src/config'; import {ApplicationRef, CompilerFactory, Component, NgModule, NgModuleRef, NgZone, PlatformRef, destroyPlatform, getPlatform} from '@angular/core'; -import {async, inject} from '@angular/core/testing'; +import {TestBed, async, inject} from '@angular/core/testing'; import {Http, HttpModule, Response, ResponseOptions, XHRBackend} from '@angular/http'; import {MockBackend, MockConnection} from '@angular/http/testing'; import {DOCUMENT} from '@angular/platform-browser'; import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; import {INITIAL_CONFIG, PlatformState, ServerModule, platformDynamicServer, renderModule, renderModuleFactory} from '@angular/platform-server'; - import {Subscription} from 'rxjs/Subscription'; import {filter} from 'rxjs/operator/filter'; import {first} from 'rxjs/operator/first'; @@ -99,6 +99,25 @@ class ImageExampleModule { } export function main() { + describe('regular', () => { declareTests({viewEngine: false}); }); + + describe('view engine', () => { + beforeEach(() => { + TestBed.configureCompiler({ + useJit: true, + providers: [{ + provide: USE_VIEW_ENGINE, + useValue: true, + }], + }); + }); + + declareTests({viewEngine: true}); + }); +} + + +function declareTests({viewEngine}: {viewEngine: boolean}) { if (getDOM().supportsDOMEvents()) return; // NODE only describe('platform-server integration', () => {