Skip to content

Commit

Permalink
fix(ivy): match directives on namespaced elements (angular#33555)
Browse files Browse the repository at this point in the history
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 angular#32061

PR Close angular#33555
  • Loading branch information
JoostK authored and mohaxspb committed Nov 7, 2019
1 parent 6520259 commit 4942bfe
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 23 deletions.
20 changes: 3 additions & 17 deletions packages/compiler/src/render3/view/t2_binder.ts
Expand Up @@ -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';


Expand Down Expand Up @@ -222,25 +223,10 @@ class DirectiveBinder<DirectiveT extends DirectiveMeta> 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[] = [];
Expand Down
13 changes: 8 additions & 5 deletions packages/compiler/src/render3/view/template.ts
Expand Up @@ -1189,9 +1189,9 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, 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); });
}
Expand Down Expand Up @@ -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));
Expand Down
20 changes: 20 additions & 0 deletions packages/compiler/test/render3/view/binding_spec.ts
Expand Up @@ -64,6 +64,26 @@ describe('t2 binding', () => {
expect(directives[0].name).toBe('NgFor');
});

it('should match directives on namespaced elements', () => {
const template = parseTemplate('<svg><text dir>SVG</text></svg>', '', {});
const matcher = new SelectorMatcher<DirectiveMeta>();
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('<div *ngFor="let item of items" dir></div>', '', {});
const binder = new R3TargetBinder(makeSelectorMatcher());
Expand Down
48 changes: 47 additions & 1 deletion packages/core/test/acceptance/directive_spec.ts
Expand Up @@ -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';
Expand Down Expand Up @@ -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: `<svg dir><text dir></text></svg>`,
})
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: `<svg dir><text dir></text></svg>`,
})
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[] = [];

Expand Down

0 comments on commit 4942bfe

Please sign in to comment.