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 (angular#27849)

Angular allows for `<ng-content>` elements to include a selector which
filters which content-projected entries are inserted into the container
depending on whether or not the selector is matched.

With Ivy this feature has not fully worked due to the massive changes
that took place inside of Ivy's styling algorithm code (which is
responsible for assigning classes and styles to an element). This
fix ensures that content-projection can correctly identify which slot
an element should be placed into when class-based selectors are used.

PR Close angular#27849
  • Loading branch information
@matsko @wKoza
matsko authored and wKoza committed Jan 18, 2019
1 parent b877c7f commit b73e75d
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 '../util/ng_dev_mode';
import {assertDefined, assertNotEqual} from '../util/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 @@ -1022,6 +1022,9 @@
{
"name": "queueComponentIndexForCheck"
},
{
"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);
it('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 b73e75d

Please sign in to comment.