From bca437617fc3ffa4d18bf45a7179c6d2e30e83c8 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 3 Nov 2019 12:12:35 +0100 Subject: [PATCH] fix(ivy): match directives on namespaced elements (#33555) Prior to this change, namespaced elements such as SVG elements would not participate correctly in directive matching as their namespace was not ignored, which was the case with the View Engine compiler. This led to incorrect behavior at runtime and template type checking. This commit resolved the issue by ignoring the namespace of elements and attributes like they were in View Engine. Fixes #32061 PR Close #33555 --- .../compiler/src/render3/view/t2_binder.ts | 20 ++------ .../compiler/src/render3/view/template.ts | 13 +++-- .../test/render3/view/binding_spec.ts | 20 ++++++++ .../core/test/acceptance/directive_spec.ts | 48 ++++++++++++++++++- 4 files changed, 78 insertions(+), 23 deletions(-) diff --git a/packages/compiler/src/render3/view/t2_binder.ts b/packages/compiler/src/render3/view/t2_binder.ts index e684b29e0a..9bb9f0630a 100644 --- a/packages/compiler/src/render3/view/t2_binder.ts +++ b/packages/compiler/src/render3/view/t2_binder.ts @@ -11,6 +11,7 @@ import {CssSelector, SelectorMatcher} from '../../selector'; import {BoundAttribute, BoundEvent, BoundText, Content, Element, Icu, Node, Reference, Template, Text, TextAttribute, Variable, Visitor} from '../r3_ast'; import {BoundTarget, DirectiveMeta, Target, TargetBinder} from './t2_api'; +import {createCssSelector} from './template'; import {getAttrsForDirectiveMatching} from './util'; @@ -222,25 +223,10 @@ class DirectiveBinder implements Visitor { visitTemplate(template: Template): void { this.visitElementOrTemplate('ng-template', template); } - visitElementOrTemplate(tag: string, node: Element|Template): void { + visitElementOrTemplate(elementName: string, node: Element|Template): void { // First, determine the HTML shape of the node for the purpose of directive matching. // Do this by building up a `CssSelector` for the node. - const cssSelector = new CssSelector(); - cssSelector.setElement(tag); - - // Add attributes to the CSS selector. - const attrs = getAttrsForDirectiveMatching(node); - Object.getOwnPropertyNames(attrs).forEach((name) => { - const value = attrs[name]; - - cssSelector.addAttribute(name, value); - - // Treat the 'class' attribute specially. - if (name.toLowerCase() === 'class') { - const classes = value.trim().split(/\s+/g); - classes.forEach(className => cssSelector.addClassName(className)); - } - }); + const cssSelector = createCssSelector(elementName, getAttrsForDirectiveMatching(node)); // Next, use the `SelectorMatcher` to get the list of directives on the node. const directives: DirectiveT[] = []; diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 71e4c7078b..de51e3cc42 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -1189,9 +1189,9 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver return args; } - private matchDirectives(tagName: string, elOrTpl: t.Element|t.Template) { + private matchDirectives(elementName: string, elOrTpl: t.Element|t.Template) { if (this.directiveMatcher) { - const selector = createCssSelector(tagName, getAttrsForDirectiveMatching(elOrTpl)); + const selector = createCssSelector(elementName, getAttrsForDirectiveMatching(elOrTpl)); this.directiveMatcher.match( selector, (cssSelector, staticType) => { this.directives.add(staticType); }); } @@ -1762,15 +1762,18 @@ export class BindingScope implements LocalResolver { /** * Creates a `CssSelector` given a tag name and a map of attributes */ -function createCssSelector(tag: string, attributes: {[name: string]: string}): CssSelector { +export function createCssSelector( + elementName: string, attributes: {[name: string]: string}): CssSelector { const cssSelector = new CssSelector(); + const elementNameNoNs = splitNsName(elementName)[1]; - cssSelector.setElement(tag); + cssSelector.setElement(elementNameNoNs); Object.getOwnPropertyNames(attributes).forEach((name) => { + const nameNoNs = splitNsName(name)[1]; const value = attributes[name]; - cssSelector.addAttribute(name, value); + cssSelector.addAttribute(nameNoNs, value); if (name.toLowerCase() === 'class') { const classes = value.trim().split(/\s+/); classes.forEach(className => cssSelector.addClassName(className)); diff --git a/packages/compiler/test/render3/view/binding_spec.ts b/packages/compiler/test/render3/view/binding_spec.ts index b7e6fcd310..8eb08599f1 100644 --- a/packages/compiler/test/render3/view/binding_spec.ts +++ b/packages/compiler/test/render3/view/binding_spec.ts @@ -64,6 +64,26 @@ describe('t2 binding', () => { expect(directives[0].name).toBe('NgFor'); }); + it('should match directives on namespaced elements', () => { + const template = parseTemplate('SVG', '', {}); + const matcher = new SelectorMatcher(); + matcher.addSelectables(CssSelector.parse('text[dir]'), { + name: 'Dir', + exportAs: null, + inputs: {}, + outputs: {}, + isComponent: false, + }); + const binder = new R3TargetBinder(matcher); + const res = binder.bind({template: template.nodes}); + const svgNode = template.nodes[0] as a.Element; + const textNode = svgNode.children[0] as a.Element; + const directives = res.getDirectivesOfNode(textNode) !; + expect(directives).not.toBeNull(); + expect(directives.length).toBe(1); + expect(directives[0].name).toBe('Dir'); + }); + it('should not match directives intended for an element on a microsyntax template', () => { const template = parseTemplate('
', '', {}); const binder = new R3TargetBinder(makeSelectorMatcher()); diff --git a/packages/core/test/acceptance/directive_spec.ts b/packages/core/test/acceptance/directive_spec.ts index d0fee53328..41ba900960 100644 --- a/packages/core/test/acceptance/directive_spec.ts +++ b/packages/core/test/acceptance/directive_spec.ts @@ -7,7 +7,7 @@ */ import {CommonModule} from '@angular/common'; -import {Component, Directive, EventEmitter, Output, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core'; +import {Component, Directive, ElementRef, EventEmitter, Output, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core'; import {Input} from '@angular/core/src/metadata'; import {TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; @@ -260,6 +260,52 @@ describe('directives', () => { expect(calls).toEqual(['MyDir.ngOnInit']); }); + it('should match directives on elements with namespace', () => { + const calls: string[] = []; + + @Directive({selector: 'svg[dir]'}) + class MyDir { + constructor(private el: ElementRef) {} + ngOnInit() { calls.push(`MyDir.ngOnInit: ${this.el.nativeElement.tagName}`); } + } + + @Component({ + selector: `my-comp`, + template: ``, + }) + class MyComp { + } + + TestBed.configureTestingModule({declarations: [MyDir, MyComp]}); + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + + expect(calls).toEqual(['MyDir.ngOnInit: svg']); + }); + + it('should match directives on descendant elements with namespace', () => { + const calls: string[] = []; + + @Directive({selector: 'text[dir]'}) + class MyDir { + constructor(private el: ElementRef) {} + ngOnInit() { calls.push(`MyDir.ngOnInit: ${this.el.nativeElement.tagName}`); } + } + + @Component({ + selector: `my-comp`, + template: ``, + }) + class MyComp { + } + + TestBed.configureTestingModule({declarations: [MyDir, MyComp]}); + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + + expect(calls).toEqual(['MyDir.ngOnInit: text']); + }); + it('should match directives when the node has "class", "style" and a binding', () => { const logs: string[] = [];