Skip to content

Commit

Permalink
fix(ivy): ensure component/directive class selectors are properly u…
Browse files Browse the repository at this point in the history 
…nderstood
  • Loading branch information
@matsko
matsko committed Jan 11, 2019
1 parent c4f7727 commit 552b267
Show file tree
Hide file tree
Showing 4 changed files with 181 additions and 115 deletions.
33 changes: 28 additions & 5 deletions packages/core/src/render3/node_selector_matcher.ts
View file
Expand Up @@ -11,6 +11,7 @@ import './ng_dev_mode';
import {assertDefined, assertNotEqual} from './assert';
import {AttributeMarker, TAttributes, TNode, TNodeType, unusedValueExportToPlacateAjd as unused1} from './interfaces/node';
import {CssSelector, CssSelectorList, NG_PROJECT_AS_ATTR_NAME, SelectorFlags, unusedValueExportToPlacateAjd as unused2} from './interfaces/projection';
import {getInitialClassNameValue} from './styling/class_and_style_bindings';

const unusedValueToPlacateAjd = unused1 + unused2;

Expand Down Expand Up @@ -58,7 +59,6 @@ function hasTagAndTypeMatch(
export function isNodeMatchingSelector(
tNode: TNode, selector: CssSelector, isProjectionMode: boolean): boolean {
ngDevMode && assertDefined(selector[0], 'Selector should have a tag name');

let mode: SelectorFlags = SelectorFlags.ELEMENT;
const nodeAttrs = tNode.attrs !;
const selectOnlyMarkerIdx = nodeAttrs ? nodeAttrs.indexOf(AttributeMarker.SelectOnly) : -1;
Expand Down Expand Up @@ -92,7 +92,19 @@ export function isNodeMatchingSelector(
skipToNextSelector = true;
}
} else {
const attrName = mode & SelectorFlags.CLASS ? 'class' : current;
const selectorAttrValue = mode & SelectorFlags.CLASS ? current : selector[++i];

// special case for matching against classes when a tNode has been instantiated with
// class and style values as separate attribute values (e.g. ['title', CLASS, 'foo'])
if ((mode & SelectorFlags.CLASS) && tNode.stylingTemplate) {
if (!isCssClassMatching(readClassValueFromTNode(tNode), selectorAttrValue as string)) {
if (isPositive(mode)) return false;
skipToNextSelector = true;
}
continue;
}

const attrName = (mode & SelectorFlags.CLASS) ? 'class' : current;
const attrIndexInNode = findAttrIndexInNode(attrName, nodeAttrs);

if (attrIndexInNode === -1) {
Expand All @@ -101,7 +113,6 @@ export function isNodeMatchingSelector(
continue;
}

const selectorAttrValue = mode & SelectorFlags.CLASS ? current : selector[++i];
if (selectorAttrValue !== '') {
let nodeAttrValue: string;
const maybeAttrName = nodeAttrs[attrIndexInNode];
Expand All @@ -113,8 +124,10 @@ export function isNodeMatchingSelector(
'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) ||

const compareAgainstClassName = mode & SelectorFlags.CLASS ? nodeAttrValue : null;
if (compareAgainstClassName &&
!isCssClassMatching(compareAgainstClassName, selectorAttrValue as string) ||
mode & SelectorFlags.ATTRIBUTE && selectorAttrValue !== nodeAttrValue) {
if (isPositive(mode)) return false;
skipToNextSelector = true;
Expand All @@ -130,6 +143,16 @@ function isPositive(mode: SelectorFlags): boolean {
return (mode & SelectorFlags.NOT) === 0;
}

function readClassValueFromTNode(tNode: TNode): string {
// comparing against CSS class values is complex because the compiler doesn't place them as
// regular attributes when an element is created. Instead, the classes (and styles for
// that matter) are placed in a special styling context that is used for resolving all
// class/style values across static attributes, [style]/[class] and [style.prop]/[class.name]
// bindings. Therefore if and when the styling context exists then the class values are to be
// extracted by the context helper code below...
return tNode.stylingTemplate ? getInitialClassNameValue(tNode.stylingTemplate) : '';
}

/**
* Examines an attributes definition array from a node to find the index of the
* attribute with the specified name.
Expand Down
3 changes: 3 additions & 0 deletions packages/core/test/bundling/todo/bundle.golden_symbols.json
View file
Expand Up @@ -1058,6 +1058,9 @@
{
"name": "queueViewHooks"
},
{
"name": "readClassValueFromTNode"
},
{
"name": "readElementValue"
},
Expand Down
230 changes: 123 additions & 107 deletions packages/core/test/linker/projection_integration_spec.ts
View file
Expand Up @@ -81,22 +81,35 @@ describe('projection', () => {
expect(main.nativeElement).toHaveText('');
});

fixmeIvy('FW-833: Directive / projected node matching against class name')
.it('should support multiple content tags', () => {
TestBed.configureTestingModule({declarations: [MultipleContentTagsComponent]});
TestBed.overrideComponent(MainComp, {
set: {
template: '<multiple-content-tags>' +
'<div>B</div>' +
'<div>C</div>' +
'<div class="left">A</div>' +
'</multiple-content-tags>'
}
});
const main = TestBed.createComponent(MainComp);
fit('should project a single class-based tag', () => {
TestBed.configureTestingModule({declarations: [SingleContentTagComponent]});
TestBed.overrideComponent(MainComp, {
set: {
template: '<single-content-tag>' +
'<div class="target">I AM PROJECTED</div>' +
'</single-content-tag>'
}
});
const main = TestBed.createComponent(MainComp);

expect(main.nativeElement).toHaveText('(A, BC)');
});
expect(main.nativeElement).toHaveText('I AM PROJECTED');
});

fixmeIvy('unknown').it('should support multiple content tags', () => {
TestBed.configureTestingModule({declarations: [MultipleContentTagsComponent]});
TestBed.overrideComponent(MainComp, {
set: {
template: '<multiple-content-tags>' +
'<div>B</div>' +
'<div>C</div>' +
'<div class="left">A</div>' +
'</multiple-content-tags>'
}
});
const main = TestBed.createComponent(MainComp);

expect(main.nativeElement).toHaveText('(A, BC)');
});

it('should redistribute only direct children', () => {
TestBed.configureTestingModule({declarations: [MultipleContentTagsComponent]});
Expand Down Expand Up @@ -182,35 +195,34 @@ describe('projection', () => {
expect(main.nativeElement).toHaveText('OUTER(INNER(INNERINNER(A,BC)))');
});

fixmeIvy('FW-833: Directive / projected node matching against class name')
.it('should redistribute when the shadow dom changes', () => {
TestBed.configureTestingModule(
{declarations: [ConditionalContentComponent, ManualViewportDirective]});
TestBed.overrideComponent(MainComp, {
set: {
template: '<conditional-content>' +
'<div class="left">A</div>' +
'<div>B</div>' +
'<div>C</div>' +
'</conditional-content>'
}
});
const main = TestBed.createComponent(MainComp);
fixmeIvy('unknown').it('should redistribute when the shadow dom changes', () => {
TestBed.configureTestingModule(
{declarations: [ConditionalContentComponent, ManualViewportDirective]});
TestBed.overrideComponent(MainComp, {
set: {
template: '<conditional-content>' +
'<div class="left">A</div>' +
'<div>B</div>' +
'<div>C</div>' +
'</conditional-content>'
}
});
const main = TestBed.createComponent(MainComp);

const viewportDirective =
main.debugElement.queryAllNodes(By.directive(ManualViewportDirective))[0].injector.get(
ManualViewportDirective);
const viewportDirective =
main.debugElement.queryAllNodes(By.directive(ManualViewportDirective))[0].injector.get(
ManualViewportDirective);

expect(main.nativeElement).toHaveText('(, BC)');
expect(main.nativeElement).toHaveText('(, BC)');

viewportDirective.show();
main.detectChanges();
expect(main.nativeElement).toHaveText('(A, BC)');
viewportDirective.show();
main.detectChanges();
expect(main.nativeElement).toHaveText('(A, BC)');

viewportDirective.hide();
main.detectChanges();
expect(main.nativeElement).toHaveText('(, BC)');
});
viewportDirective.hide();
main.detectChanges();
expect(main.nativeElement).toHaveText('(, BC)');
});

// GH-2095 - https://github.com/angular/angular/issues/2095
// important as we are removing the ng-content element during compilation,
Expand Down Expand Up @@ -290,38 +302,36 @@ describe('projection', () => {
expect(main.nativeElement).toHaveText('SIMPLE()START(A)END');
});

fixmeIvy('FW-833: Directive / projected node matching against class name')
.it('should support moving ng-content around', () => {
TestBed.configureTestingModule({
declarations: [ConditionalContentComponent, ProjectDirective, ManualViewportDirective]
});
TestBed.overrideComponent(MainComp, {
set: {
template: '<conditional-content>' +
'<div class="left">A</div>' +
'<div>B</div>' +
'</conditional-content>' +
'START(<div project></div>)END'
}
});
const main = TestBed.createComponent(MainComp);
fixmeIvy('unknown').it('should support moving ng-content around', () => {
TestBed.configureTestingModule(
{declarations: [ConditionalContentComponent, ProjectDirective, ManualViewportDirective]});
TestBed.overrideComponent(MainComp, {
set: {
template: '<conditional-content>' +
'<div class="left">A</div>' +
'<div>B</div>' +
'</conditional-content>' +
'START(<div project></div>)END'
}
});
const main = TestBed.createComponent(MainComp);

const sourceDirective: ManualViewportDirective =
main.debugElement.queryAllNodes(By.directive(ManualViewportDirective))[0].injector.get(
ManualViewportDirective);
const projectDirective: ProjectDirective =
main.debugElement.queryAllNodes(By.directive(ProjectDirective))[0].injector.get(
ProjectDirective);
expect(main.nativeElement).toHaveText('(, B)START()END');

projectDirective.show(sourceDirective.templateRef);
expect(main.nativeElement).toHaveText('(, B)START(A)END');

// Stamping ng-content multiple times should not produce the content multiple
// times...
projectDirective.show(sourceDirective.templateRef);
expect(main.nativeElement).toHaveText('(, B)START(A)END');
});
const sourceDirective: ManualViewportDirective =
main.debugElement.queryAllNodes(By.directive(ManualViewportDirective))[0].injector.get(
ManualViewportDirective);
const projectDirective: ProjectDirective =
main.debugElement.queryAllNodes(By.directive(ProjectDirective))[0].injector.get(
ProjectDirective);
expect(main.nativeElement).toHaveText('(, B)START()END');

projectDirective.show(sourceDirective.templateRef);
expect(main.nativeElement).toHaveText('(, B)START(A)END');

// Stamping ng-content multiple times should not produce the content multiple
// times...
projectDirective.show(sourceDirective.templateRef);
expect(main.nativeElement).toHaveText('(, B)START(A)END');
});

// Note: This does not use a ng-content element, but
// is still important as we are merging proto views independent of
Expand Down Expand Up @@ -533,49 +543,48 @@ describe('projection', () => {
expect(main.nativeElement).toHaveText('B(A)');
});

fixmeIvy('FW-833: Directive / projected node matching against class name')
.it('should project filled view containers into a view container', () => {
TestBed.configureTestingModule(
{declarations: [ConditionalContentComponent, ManualViewportDirective]});
TestBed.overrideComponent(MainComp, {
set: {
template: '<conditional-content>' +
'<div class="left">A</div>' +
'<ng-template manual class="left">B</ng-template>' +
'<div class="left">C</div>' +
'<div>D</div>' +
'</conditional-content>'
}
});
const main = TestBed.createComponent(MainComp);
fixmeIvy('unknown').it('should project filled view containers into a view container', () => {
TestBed.configureTestingModule(
{declarations: [ConditionalContentComponent, ManualViewportDirective]});
TestBed.overrideComponent(MainComp, {
set: {
template: '<conditional-content>' +
'<div class="left">A</div>' +
'<ng-template manual class="left">B</ng-template>' +
'<div class="left">C</div>' +
'<div>D</div>' +
'</conditional-content>'
}
});
const main = TestBed.createComponent(MainComp);

const conditionalComp = main.debugElement.query(By.directive(ConditionalContentComponent));
const conditionalComp = main.debugElement.query(By.directive(ConditionalContentComponent));

const viewViewportDir =
conditionalComp.queryAllNodes(By.directive(ManualViewportDirective))[0].injector.get(
ManualViewportDirective);
const viewViewportDir =
conditionalComp.queryAllNodes(By.directive(ManualViewportDirective))[0].injector.get(
ManualViewportDirective);

expect(main.nativeElement).toHaveText('(, D)');
expect(main.nativeElement).toHaveText('(, D)');
expect(main.nativeElement).toHaveText('(, D)');
expect(main.nativeElement).toHaveText('(, D)');

viewViewportDir.show();
main.detectChanges();
expect(main.nativeElement).toHaveText('(AC, D)');
viewViewportDir.show();
main.detectChanges();
expect(main.nativeElement).toHaveText('(AC, D)');

const contentViewportDir =
conditionalComp.queryAllNodes(By.directive(ManualViewportDirective))[1].injector.get(
ManualViewportDirective);
const contentViewportDir =
conditionalComp.queryAllNodes(By.directive(ManualViewportDirective))[1].injector.get(
ManualViewportDirective);

contentViewportDir.show();
main.detectChanges();
expect(main.nativeElement).toHaveText('(ABC, D)');
contentViewportDir.show();
main.detectChanges();
expect(main.nativeElement).toHaveText('(ABC, D)');

// hide view viewport, and test that it also hides
// the content viewport's views
viewViewportDir.hide();
main.detectChanges();
expect(main.nativeElement).toHaveText('(, D)');
});
// hide view viewport, and test that it also hides
// the content viewport's views
viewViewportDir.hide();
main.detectChanges();
expect(main.nativeElement).toHaveText('(, D)');
});
});

@Component({selector: 'main', template: ''})
Expand Down Expand Up @@ -626,6 +635,13 @@ class Empty {
class MultipleContentTagsComponent {
}

@Component({
selector: 'single-content-tag',
template: '<ng-content SELECT=".target"></ng-content>',
})
class SingleContentTagComponent {
}

@Directive({selector: '[manual]'})
class ManualViewportDirective {
constructor(public vc: ViewContainerRef, public templateRef: TemplateRef<Object>) {}
Expand Down

0 comments on commit 552b267

Please sign in to comment.