From e62eeed7d4e1ead64eaee023632e177e17d5964e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Fri, 11 Jan 2019 08:37:51 -0800 Subject: [PATCH] fix(ivy): ensure component/directive `class` selectors are properly understood (#27849) Angular allows for `` 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 #27849 --- .../core/src/render3/node_selector_matcher.ts | 33 ++- .../bundling/todo/bundle.golden_symbols.json | 3 + .../linker/projection_integration_spec.ts | 226 ++++++++++-------- .../render3/node_selector_matcher_spec.ts | 30 ++- 4 files changed, 179 insertions(+), 113 deletions(-) diff --git a/packages/core/src/render3/node_selector_matcher.ts b/packages/core/src/render3/node_selector_matcher.ts index 065233674c..9fcc32cfa7 100644 --- a/packages/core/src/render3/node_selector_matcher.ts +++ b/packages/core/src/render3/node_selector_matcher.ts @@ -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; @@ -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; @@ -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) { @@ -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]; @@ -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; @@ -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. diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 3fa175255b..0de16a9c3f 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -1022,6 +1022,9 @@ { "name": "queueComponentIndexForCheck" }, + { + "name": "readClassValueFromTNode" + }, { "name": "readElementValue" }, diff --git a/packages/core/test/linker/projection_integration_spec.ts b/packages/core/test/linker/projection_integration_spec.ts index d41a092e41..23bbb76eb9 100644 --- a/packages/core/test/linker/projection_integration_spec.ts +++ b/packages/core/test/linker/projection_integration_spec.ts @@ -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: '' + - '
B
' + - '
C
' + - '
A
' + - '' - } - }); - const main = TestBed.createComponent(MainComp); + it('should project a single class-based tag', () => { + TestBed.configureTestingModule({declarations: [SingleContentTagComponent]}); + TestBed.overrideComponent(MainComp, { + set: { + template: '' + + '
I AM PROJECTED
' + + '' + } + }); + 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: '' + + '
B
' + + '
C
' + + '
A
' + + '' + } + }); + const main = TestBed.createComponent(MainComp); + + expect(main.nativeElement).toHaveText('(A, BC)'); + }); it('should redistribute only direct children', () => { TestBed.configureTestingModule({declarations: [MultipleContentTagsComponent]}); @@ -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: '' + - '
A
' + - '
B
' + - '
C
' + - '' - } - }); - 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: '' + + '
A
' + + '
B
' + + '
C
' + + '' + } + }); + 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, @@ -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: '' + - '
A
' + - '
B
' + - '' + - 'START(
)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: '' + + '
A
' + + '
B
' + + '' + + 'START(
)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'); + 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'); + 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'); - }); + // 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 @@ -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: '' + - '
A
' + - 'B' + - '
C
' + - '
D
' + - '' - } - }); - 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: '' + + '
A
' + + 'B' + + '
C
' + + '
D
' + + '' + } + }); + 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: ''}) @@ -626,6 +635,13 @@ class Empty { class MultipleContentTagsComponent { } +@Component({ + selector: 'single-content-tag', + template: '', +}) +class SingleContentTagComponent { +} + @Directive({selector: '[manual]'}) class ManualViewportDirective { constructor(public vc: ViewContainerRef, public templateRef: TemplateRef) {} diff --git a/packages/core/test/render3/node_selector_matcher_spec.ts b/packages/core/test/render3/node_selector_matcher_spec.ts index 5974dd4391..822dcca3ac 100644 --- a/packages/core/test/render3/node_selector_matcher_spec.ts +++ b/packages/core/test/render3/node_selector_matcher_spec.ts @@ -10,6 +10,7 @@ import {AttributeMarker, TAttributes, TNode, TNodeType} from '../../src/render3/ import {CssSelector, CssSelectorList, NG_PROJECT_AS_ATTR_NAME, SelectorFlags,} from '../../src/render3/interfaces/projection'; import {getProjectAsAttrValue, isNodeMatchingSelectorList, isNodeMatchingSelector} from '../../src/render3/node_selector_matcher'; +import {initializeStaticContext} from '../../src/render3/styling/class_and_style_bindings'; import {createTNode} from '@angular/core/src/render3/instructions'; import {getLView} from '@angular/core/src/render3/state'; @@ -18,9 +19,12 @@ function testLStaticData(tagName: string, attrs: TAttributes | null): TNode { } describe('css selector matching', () => { - function isMatching(tagName: string, attrs: TAttributes | null, selector: CssSelector): boolean { - return isNodeMatchingSelector( - createTNode(getLView(), TNodeType.Element, 0, tagName, attrs, null), selector, false); + function isMatching( + tagName: string, attrsOrTNode: TAttributes | TNode | null, selector: CssSelector): boolean { + const tNode = (!attrsOrTNode || Array.isArray(attrsOrTNode)) ? + createTNode(getLView(), TNodeType.Element, 0, tagName, attrsOrTNode as TAttributes, null) : + (attrsOrTNode as TNode); + return isNodeMatchingSelector(tNode, selector, false); } describe('isNodeMatchingSimpleSelector', () => { @@ -298,6 +302,26 @@ describe('css selector matching', () => { //
expect(isMatching('div', ['class', 'foo'], selector)).toBeFalsy(); }); + + it('should match against a class value before and after the styling context is created', + () => { + // selector: 'div.abc' + const selector = ['div', SelectorFlags.CLASS, 'abc']; + const tNode = createTNode(getLView(), TNodeType.Element, 0, 'div', [], null); + + //
(without attrs or styling context) + expect(isMatching('div', tNode, selector)).toBeFalsy(); + + //
(with attrs but without styling context) + tNode.attrs = ['class', 'abc']; + tNode.stylingTemplate = null; + expect(isMatching('div', tNode, selector)).toBeTruthy(); + + //
(with styling context but without attrs) + tNode.stylingTemplate = initializeStaticContext([AttributeMarker.Classes, 'abc']); + tNode.attrs = null; + expect(isMatching('div', tNode, selector)).toBeTruthy(); + }); }); });