From 82c5313740ee39ff55b952e74bfb13295387b284 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Fri, 8 Jun 2018 15:25:39 -0700 Subject: [PATCH] feat(ivy): namespaced attributes added to output instructions (#24386) NOTE: This does NOT add parsing of namespaced attributes - Adds AttributeMarker for namespaced attributes - Adds test for namespaced attributes - Updates AttributeMarker enum to use CamelCase, and not UPPER_CASE names PR Close #24386 --- .../render3/r3_compiler_compliance_spec.ts | 47 ++++++++++++++++ packages/core/src/render3/STATUS.md | 1 + packages/core/src/render3/di.ts | 2 +- packages/core/src/render3/instructions.ts | 46 +++++++++++---- packages/core/src/render3/interfaces/node.ts | 11 +++- .../core/src/render3/node_selector_matcher.ts | 47 ++++++++++++---- .../compiler_canonical/elements_spec.ts | 56 +++++++++++++++++++ packages/core/test/render3/content_spec.ts | 2 +- packages/core/test/render3/di_spec.ts | 4 +- packages/core/test/render3/directive_spec.ts | 10 ++-- .../core/test/render3/instructions_spec.ts | 43 +++++++++++++- .../render3/node_selector_matcher_spec.ts | 12 +++- packages/core/test/render3/query_spec.ts | 6 +- 13 files changed, 247 insertions(+), 40 deletions(-) diff --git a/packages/compiler/test/render3/r3_compiler_compliance_spec.ts b/packages/compiler/test/render3/r3_compiler_compliance_spec.ts index a61016f656..ee6b53d123 100644 --- a/packages/compiler/test/render3/r3_compiler_compliance_spec.ts +++ b/packages/compiler/test/render3/r3_compiler_compliance_spec.ts @@ -163,6 +163,53 @@ describe('compiler compliance', () => { expectEmit(result.source, template, 'Incorrect template'); }); + // TODO(https://github.com/angular/angular/issues/24426): We need to support the parser actually + // building the proper attributes based off of xmlns atttribuates. + xit('should support namspaced attributes', () => { + const files = { + app: { + 'spec.ts': ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'my-component', + template: \`
Hello World!
\` + }) + export class MyComponent {} + + @NgModule({declarations: [MyComponent]}) + export class MyModule {} + ` + } + }; + + // The factory should look like this: + const factory = 'factory: function MyComponent_Factory() { return new MyComponent(); }'; + + // The template should look like this (where IDENT is a wild card for an identifier): + const template = ` + const $c1$ = ['class', 'my-app', 0, 'http://someuri/foo', 'foo:bar', 'baz', 'title', 'Hello', 0, 'http://someuri/foo', 'foo:qux', 'quacks']; + … + template: function MyComponent_Template(rf: IDENT, ctx: IDENT) { + if (rf & 1) { + $r3$.ɵE(0, 'div', $e0_attrs$); + $r3$.ɵT(1, 'Hello '); + $r3$.ɵE(2, 'b'); + $r3$.ɵT(3, 'World'); + $r3$.ɵe(); + $r3$.ɵT(4, '!'); + $r3$.ɵe(); + } + } + `; + + + const result = compile(files, angularFiles); + + expectEmit(result.source, factory, 'Incorrect factory'); + expectEmit(result.source, template, 'Incorrect template'); + }); + it('should bind to element properties', () => { const files = { app: { diff --git a/packages/core/src/render3/STATUS.md b/packages/core/src/render3/STATUS.md index 20f59c399a..bf1f5d0c10 100644 --- a/packages/core/src/render3/STATUS.md +++ b/packages/core/src/render3/STATUS.md @@ -147,6 +147,7 @@ The goal is for the `@Component` (and friends) to be the compiler of template. S | `
` | ✅ | ✅ | ✅ | | `
` | ✅ | ✅ | ✅ | | `
` | ✅ | ✅ | ✅ | +| `
`
Compiler still needs to be updated to process templates with namespaced attributes. ([see #24386](https://github.com/angular/angular/pull/24386)) | ✅ | ✅ | ❌ | | `{{ ['literal', exp ] }}` | ✅ | ✅ | ✅ | | `{{ { a: 'literal', b: exp } }}` | ✅ | ✅ | ✅ | | `{{ exp \| pipe: arg }}` | ✅ | ✅ | ✅ | diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index 3200038789..b5b9ad78d3 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -262,7 +262,7 @@ export function injectAttribute(attrNameToInject: string): string|undefined { if (attrs) { for (let i = 0; i < attrs.length; i = i + 2) { const attrName = attrs[i]; - if (attrName === AttributeMarker.SELECT_ONLY) break; + if (attrName === AttributeMarker.SelectOnly) break; if (attrName == attrNameToInject) { return attrs[i + 1] as string; } diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index d6debea1f8..c9c775a8fd 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -858,16 +858,34 @@ export function createTView( function setUpAttributes(native: RElement, attrs: TAttributes): void { const isProc = isProceduralRenderer(renderer); - for (let i = 0; i < attrs.length; i += 2) { + let i = 0; + + while (i < attrs.length) { const attrName = attrs[i]; - if (attrName === AttributeMarker.SELECT_ONLY) break; - if (attrName !== NG_PROJECT_AS_ATTR_NAME) { - const attrVal = attrs[i + 1]; + if (attrName === AttributeMarker.SelectOnly) break; + if (attrName === NG_PROJECT_AS_ATTR_NAME) { + i += 2; + } else { ngDevMode && ngDevMode.rendererSetAttribute++; - isProc ? - (renderer as ProceduralRenderer3) - .setAttribute(native, attrName as string, attrVal as string) : - native.setAttribute(attrName as string, attrVal as string); + if (attrName === AttributeMarker.NamespaceURI) { + // Namespaced attributes + const namespaceURI = attrs[i + 1] as string; + const attrName = attrs[i + 2] as string; + const attrVal = attrs[i + 3] as string; + isProc ? + (renderer as ProceduralRenderer3) + .setAttribute(native, attrName, attrVal, namespaceURI) : + native.setAttributeNS(namespaceURI, attrName, attrVal); + i += 4; + } else { + // Standard attributes + const attrVal = attrs[i + 1]; + isProc ? + (renderer as ProceduralRenderer3) + .setAttribute(native, attrName as string, attrVal as string) : + native.setAttribute(attrName as string, attrVal as string); + i += 2; + } } } } @@ -1509,17 +1527,25 @@ function generateInitialInputs( initialInputData[directiveIndex] = null; const attrs = tNode.attrs !; - for (let i = 0; i < attrs.length; i += 2) { + let i = 0; + while (i < attrs.length) { const attrName = attrs[i]; + if (attrName === AttributeMarker.SelectOnly) break; + if (attrName === AttributeMarker.NamespaceURI) { + // We do not allow inputs on namespaced attributes. + i += 4; + continue; + } const minifiedInputName = inputs[attrName]; const attrValue = attrs[i + 1]; - if (attrName === AttributeMarker.SELECT_ONLY) break; if (minifiedInputName !== undefined) { const inputsToStore: InitialInputs = initialInputData[directiveIndex] || (initialInputData[directiveIndex] = []); inputsToStore.push(minifiedInputName, attrValue as string); } + + i += 2; } return initialInputData; } diff --git a/packages/core/src/render3/interfaces/node.ts b/packages/core/src/render3/interfaces/node.ts index 5b0034bbb0..f4c1af3080 100644 --- a/packages/core/src/render3/interfaces/node.ts +++ b/packages/core/src/render3/interfaces/node.ts @@ -163,15 +163,20 @@ export interface LProjectionNode extends LNode { * items are not regular attributes and the processing should be adapted accordingly. */ export const enum AttributeMarker { - NS = 0, // namespace. Has to be repeated. + /** + * Marker indicates that the following 3 values in the attributes array are: + * namespaceUri, attributeName, attributeValue + * in that order. + */ + NamespaceURI = 0, /** * This marker indicates that the following attribute names were extracted from bindings (ex.: * [foo]="exp") and / or event handlers (ex. (bar)="doSth()"). * Taking the above bindings and outputs as an example an attributes array could look as follows: - * ['class', 'fade in', AttributeMarker.SELECT_ONLY, 'foo', 'bar'] + * ['class', 'fade in', AttributeMarker.SelectOnly, 'foo', 'bar'] */ - SELECT_ONLY = 1 + SelectOnly = 1 } /** diff --git a/packages/core/src/render3/node_selector_matcher.ts b/packages/core/src/render3/node_selector_matcher.ts index db94094e24..f4bd4cde68 100644 --- a/packages/core/src/render3/node_selector_matcher.ts +++ b/packages/core/src/render3/node_selector_matcher.ts @@ -8,7 +8,7 @@ import './ng_dev_mode'; -import {assertDefined} from './assert'; +import {assertDefined, assertNotEqual} from './assert'; import {AttributeMarker, TAttributes, TNode, unusedValueExportToPlacateAjd as unused1} from './interfaces/node'; import {CssSelector, CssSelectorList, NG_PROJECT_AS_ATTR_NAME, SelectorFlags, unusedValueExportToPlacateAjd as unused2} from './interfaces/projection'; @@ -40,7 +40,7 @@ export function isNodeMatchingSelector(tNode: TNode, selector: CssSelector): boo let mode: SelectorFlags = SelectorFlags.ELEMENT; const nodeAttrs = tNode.attrs !; - const selectOnlyMarkerIdx = nodeAttrs ? nodeAttrs.indexOf(AttributeMarker.SELECT_ONLY) : -1; + const selectOnlyMarkerIdx = nodeAttrs ? nodeAttrs.indexOf(AttributeMarker.SelectOnly) : -1; // When processing ":not" selectors, we skip to the next ":not" if the // current one doesn't match @@ -81,9 +81,16 @@ export function isNodeMatchingSelector(tNode: TNode, selector: CssSelector): boo const selectorAttrValue = mode & SelectorFlags.CLASS ? current : selector[++i]; if (selectorAttrValue !== '') { - const nodeAttrValue = selectOnlyMarkerIdx > -1 && attrIndexInNode > selectOnlyMarkerIdx ? - '' : - nodeAttrs[attrIndexInNode + 1]; + let nodeAttrValue: string; + const maybeAttrName = nodeAttrs[attrIndexInNode]; + if (selectOnlyMarkerIdx > -1 && attrIndexInNode > selectOnlyMarkerIdx) { + nodeAttrValue = ''; + } else { + ngDevMode && assertNotEqual( + maybeAttrName, AttributeMarker.NamespaceURI, + 'We do not match directives on namespaced attributes'); + nodeAttrValue = nodeAttrs[attrIndexInNode + 1] as string; + } if (mode & SelectorFlags.CLASS && !isCssClassMatching(nodeAttrValue as string, selectorAttrValue as string) || mode & SelectorFlags.ATTRIBUTE && selectorAttrValue !== nodeAttrValue) { @@ -101,16 +108,34 @@ function isPositive(mode: SelectorFlags): boolean { return (mode & SelectorFlags.NOT) === 0; } +/** + * Examines an attributes definition array from a node to find the index of the + * attribute with the specified name. + * + * NOTE: Will not find namespaced attributes. + * + * @param name the name of the attribute to find + * @param attrs the attribute array to examine + */ function findAttrIndexInNode(name: string, attrs: TAttributes | null): number { - let step = 2; if (attrs === null) return -1; - for (let i = 0; i < attrs.length; i += step) { - const attrName = attrs[i]; - if (attrName === name) return i; - if (attrName === AttributeMarker.SELECT_ONLY) { - step = 1; + let selectOnlyMode = false; + let i = 0; + while (i < attrs.length) { + const maybeAttrName = attrs[i]; + if (maybeAttrName === name) { + return i; + } else if (maybeAttrName === AttributeMarker.NamespaceURI) { + // NOTE(benlesh): will not find namespaced attributes. This is by design. + i += 4; + } else { + if (maybeAttrName === AttributeMarker.SelectOnly) { + selectOnlyMode = true; + } + i += selectOnlyMode ? 1 : 2; } } + return -1; } diff --git a/packages/core/test/render3/compiler_canonical/elements_spec.ts b/packages/core/test/render3/compiler_canonical/elements_spec.ts index 0f1dd08873..e82fbf5f04 100644 --- a/packages/core/test/render3/compiler_canonical/elements_spec.ts +++ b/packages/core/test/render3/compiler_canonical/elements_spec.ts @@ -10,10 +10,12 @@ import {browserDetection} from '@angular/platform-browser/testing/src/browser_ut import {ChangeDetectionStrategy, ChangeDetectorRef, Component, ContentChild, ContentChildren, Directive, HostBinding, HostListener, Injectable, Input, NgModule, OnDestroy, Optional, Pipe, PipeTransform, QueryList, SimpleChanges, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '../../../src/core'; import * as $r3$ from '../../../src/core_render3_private_export'; +import {AttributeMarker} from '../../../src/render3'; import {ComponentDef} from '../../../src/render3/interfaces/definition'; import {ComponentFixture, renderComponent, toHtml} from '../render_util'; + /// See: `normative.md` describe('elements', () => { // Saving type as $any$, etc to simplify testing for compiler, as types aren't saved @@ -150,6 +152,60 @@ describe('elements', () => { expect(toHtml(listenerComp)).toEqual(''); }); + it('should support namespaced attributes', () => { + type $MyComponent$ = MyComponent; + + // Important: keep arrays outside of function to not create new instances. + const $e0_attrs$ = [ + // class="my-app" + 'class', + 'my-app', + // foo:bar="baz" + AttributeMarker.NamespaceURI, + 'http://someuri/foo', + 'foo:bar', + 'baz', + // title="Hello" + 'title', + 'Hello', + // foo:qux="quacks" + AttributeMarker.NamespaceURI, + 'http://someuri/foo', + 'foo:qux', + 'quacks', + ]; + + @Component({ + selector: 'my-component', + template: + `
Hello World!
` + }) + class MyComponent { + // NORMATIVE + static ngComponentDef = $r3$.ɵdefineComponent({ + type: MyComponent, + selectors: [['my-component']], + factory: () => new MyComponent(), + template: function(rf: $RenderFlags$, ctx: $MyComponent$) { + if (rf & 1) { + $r3$.ɵE(0, 'div', $e0_attrs$); + $r3$.ɵT(1, 'Hello '); + $r3$.ɵE(2, 'b'); + $r3$.ɵT(3, 'World'); + $r3$.ɵe(); + $r3$.ɵT(4, '!'); + $r3$.ɵe(); + } + } + }); + // /NORMATIVE + } + + expect(toHtml(renderComponent(MyComponent))) + .toEqual( + '
Hello World!
'); + }); + describe('bindings', () => { it('should bind to property', () => { type $MyComponent$ = MyComponent; diff --git a/packages/core/test/render3/content_spec.ts b/packages/core/test/render3/content_spec.ts index 71d7dda90b..058be8ca4b 100644 --- a/packages/core/test/render3/content_spec.ts +++ b/packages/core/test/render3/content_spec.ts @@ -605,7 +605,7 @@ describe('content projection', () => { if (rf & RenderFlags.Create) { elementStart(0, 'child'); { - elementStart(1, 'span', [AttributeMarker.SELECT_ONLY, 'title']); + elementStart(1, 'span', [AttributeMarker.SelectOnly, 'title']); { text(2, 'Has title'); } elementEnd(); } diff --git a/packages/core/test/render3/di_spec.ts b/packages/core/test/render3/di_spec.ts index 66e0f67d09..31db90bae7 100644 --- a/packages/core/test/render3/di_spec.ts +++ b/packages/core/test/render3/di_spec.ts @@ -1225,7 +1225,7 @@ describe('di', () => { const MyApp = createComponent('my-app', function(rf: RenderFlags, ctx: any) { if (rf & RenderFlags.Create) { - elementStart(0, 'div', ['exist', 'existValue', AttributeMarker.SELECT_ONLY, 'nonExist']); + elementStart(0, 'div', ['exist', 'existValue', AttributeMarker.SelectOnly, 'nonExist']); exist = injectAttribute('exist'); nonExist = injectAttribute('nonExist'); } @@ -1243,7 +1243,7 @@ describe('di', () => { const MyApp = createComponent('my-app', function(rf: RenderFlags, ctx: any) { if (rf & RenderFlags.Create) { elementStart(0, 'div', [ - 'exist', 'existValue', AttributeMarker.SELECT_ONLY, 'binding1', 'nonExist', 'binding2' + 'exist', 'existValue', AttributeMarker.SelectOnly, 'binding1', 'nonExist', 'binding2' ]); exist = injectAttribute('exist'); nonExist = injectAttribute('nonExist'); diff --git a/packages/core/test/render3/directive_spec.ts b/packages/core/test/render3/directive_spec.ts index 58f0c22349..a84c9008b9 100644 --- a/packages/core/test/render3/directive_spec.ts +++ b/packages/core/test/render3/directive_spec.ts @@ -34,7 +34,7 @@ describe('directive', () => { } function Template() { - elementStart(0, 'span', [AttributeMarker.SELECT_ONLY, 'dir']); + elementStart(0, 'span', [AttributeMarker.SelectOnly, 'dir']); elementEnd(); } @@ -82,7 +82,7 @@ describe('directive', () => { */ function createTemplate() { // using 2 bindings to show example shape of attributes array - elementStart(0, 'span', ['class', 'fade', AttributeMarker.SELECT_ONLY, 'test', 'other']); + elementStart(0, 'span', ['class', 'fade', AttributeMarker.SelectOnly, 'test', 'other']); elementEnd(); } @@ -127,12 +127,12 @@ describe('directive', () => { } /** - * + * */ function createTemplate() { // putting name (test) in the "usual" value position elementStart( - 0, 'span', ['class', 'fade', AttributeMarker.SELECT_ONLY, 'prop1', 'test', 'prop2']); + 0, 'span', ['class', 'fade', AttributeMarker.SelectOnly, 'prop1', 'test', 'prop2']); elementEnd(); } @@ -168,7 +168,7 @@ describe('directive', () => { * */ function createTemplate() { - elementStart(0, 'span', [AttributeMarker.SELECT_ONLY, 'out']); + elementStart(0, 'span', [AttributeMarker.SelectOnly, 'out']); { listener('out', () => {}); } elementEnd(); } diff --git a/packages/core/test/render3/instructions_spec.ts b/packages/core/test/render3/instructions_spec.ts index d3302788eb..50d643dc28 100644 --- a/packages/core/test/render3/instructions_spec.ts +++ b/packages/core/test/render3/instructions_spec.ts @@ -11,7 +11,7 @@ import {NgForOfContext} from '@angular/common'; import {RenderFlags, directiveInject} from '../../src/render3'; import {defineComponent} from '../../src/render3/definition'; import {bind, container, element, elementAttribute, elementClass, elementEnd, elementProperty, elementStart, elementStyle, elementStyleNamed, interpolation1, renderTemplate, text, textBinding} from '../../src/render3/instructions'; -import {LElementNode, LNode} from '../../src/render3/interfaces/node'; +import {AttributeMarker, LElementNode, LNode} from '../../src/render3/interfaces/node'; import {RElement, domRendererFactory3} from '../../src/render3/interfaces/renderer'; import {TrustedString, bypassSanitizationTrustHtml, bypassSanitizationTrustResourceUrl, bypassSanitizationTrustScript, bypassSanitizationTrustStyle, bypassSanitizationTrustUrl, sanitizeHtml, sanitizeResourceUrl, sanitizeScript, sanitizeStyle, sanitizeUrl} from '../../src/sanitization/sanitization'; import {Sanitizer, SecurityContext} from '../../src/sanitization/security'; @@ -79,12 +79,53 @@ describe('instructions', () => { const div = (t.hostNode.native as HTMLElement).querySelector('div') !; expect(div.id).toEqual('test'); expect(div.title).toEqual('Hello'); + expect(ngDevMode).toHaveProperties({ + firstTemplatePass: 1, + tNode: 2, // 1 for div, 1 for host element + tView: 1, + rendererCreateElement: 1, + }); + }); + + it('should allow setting namespaced attributes', () => { + const t = new TemplateFixture(() => { + elementStart(0, 'div', [ + // id="test" + 'id', + 'test', + // test:foo="bar" + AttributeMarker.NamespaceURI, + 'http://someuri.com/2018/test', + 'test:foo', + 'bar', + // title="Hello" + 'title', + 'Hello', + ]); + elementEnd(); + }); + + const div = (t.hostNode.native as HTMLElement).querySelector('div') !; + const attrs: any = div.attributes; + + expect(attrs['id'].name).toEqual('id'); + expect(attrs['id'].namespaceURI).toEqual(null); + expect(attrs['id'].value).toEqual('test'); + + expect(attrs['test:foo'].name).toEqual('test:foo'); + expect(attrs['test:foo'].namespaceURI).toEqual('http://someuri.com/2018/test'); + expect(attrs['test:foo'].value).toEqual('bar'); + + expect(attrs['title'].name).toEqual('title'); + expect(attrs['title'].namespaceURI).toEqual(null); + expect(attrs['title'].value).toEqual('Hello'); expect(ngDevMode).toHaveProperties({ firstTemplatePass: 1, tNode: 2, // 1 for div, 1 for host element tView: 1, rendererCreateElement: 1, + rendererSetAttribute: 3 }); }); }); diff --git a/packages/core/test/render3/node_selector_matcher_spec.ts b/packages/core/test/render3/node_selector_matcher_spec.ts index 3a143bcadf..cf5c2aca36 100644 --- a/packages/core/test/render3/node_selector_matcher_spec.ts +++ b/packages/core/test/render3/node_selector_matcher_spec.ts @@ -86,6 +86,12 @@ describe('css selector matching', () => { ])).toBeFalsy(`Selector '[other]' should NOT match '`); }); + it('should match namespaced attributes', () => { + expect(isMatching( + 'span', [AttributeMarker.NamespaceURI, 'http://some/uri', 'title', 'name'], + ['', 'title', ''])); + }); + it('should match selector with one attribute without value when element has several attributes', () => { expect(isMatching('span', ['id', 'my_id', 'title', 'test_title'], [ @@ -179,14 +185,14 @@ describe('css selector matching', () => { }); it('should take optional binding attribute names into account', () => { - expect(isMatching('span', [AttributeMarker.SELECT_ONLY, 'directive'], [ + expect(isMatching('span', [AttributeMarker.SelectOnly, 'directive'], [ '', 'directive', '' ])).toBeTruthy(`Selector '[directive]' should match `); }); it('should not match optional binding attribute names if attribute selector has value', () => { - expect(isMatching('span', [AttributeMarker.SELECT_ONLY, 'directive'], [ + expect(isMatching('span', [AttributeMarker.SelectOnly, 'directive'], [ '', 'directive', 'value' ])).toBeFalsy(`Selector '[directive=value]' should not match `); }); @@ -194,7 +200,7 @@ describe('css selector matching', () => { it('should not match optional binding attribute names if attribute selector has value and next name equals to value', () => { expect(isMatching( - 'span', [AttributeMarker.SELECT_ONLY, 'directive', 'value'], + 'span', [AttributeMarker.SelectOnly, 'directive', 'value'], ['', 'directive', 'value'])) .toBeFalsy( `Selector '[directive=value]' should not match `); diff --git a/packages/core/test/render3/query_spec.ts b/packages/core/test/render3/query_spec.ts index 1c127d4b92..b94e8d26fc 100644 --- a/packages/core/test/render3/query_spec.ts +++ b/packages/core/test/render3/query_spec.ts @@ -859,7 +859,7 @@ describe('query', () => { } }, null, []); - container(5, undefined, null, [AttributeMarker.SELECT_ONLY, 'vc']); + container(5, undefined, null, [AttributeMarker.SelectOnly, 'vc']); } if (rf & RenderFlags.Update) { @@ -938,8 +938,8 @@ describe('query', () => { } }, null, []); - container(2, undefined, null, [AttributeMarker.SELECT_ONLY, 'vc']); - container(3, undefined, null, [AttributeMarker.SELECT_ONLY, 'vc']); + container(2, undefined, null, [AttributeMarker.SelectOnly, 'vc']); + container(3, undefined, null, [AttributeMarker.SelectOnly, 'vc']); } if (rf & RenderFlags.Update) {