Skip to content

Commit

Permalink
fix(core): Host classes should not be fed back into @Input (#35889)
Browse files Browse the repository at this point in the history
Previously all static styling information (including the ones from component/directive host bindings) would get merged into a single value before it would be written into the `@Input('class'/'style')`. The new behavior specifically excludes host values from the `@Input` bindings.

Fix #35383

PR Close #35889
  • Loading branch information
mhevery authored and kara committed May 14, 2020
1 parent 9cca82e commit f872b69
Show file tree
Hide file tree
Showing 10 changed files with 154 additions and 34 deletions.
2 changes: 1 addition & 1 deletion packages/core/src/render3/component.ts
Expand Up @@ -177,7 +177,7 @@ export function createRootComponentView(
const tNode: TElementNode = getOrCreateTNode(tView, null, 0, TNodeType.Element, null, null);
const mergedAttrs = tNode.mergedAttrs = def.hostAttrs;
if (mergedAttrs !== null) {
computeStaticStyling(tNode, mergedAttrs);
computeStaticStyling(tNode, mergedAttrs, true);
if (rNode !== null) {
setUpAttributes(hostRenderer, rNode, mergedAttrs);
if (tNode.classes !== null) {
Expand Down
14 changes: 9 additions & 5 deletions packages/core/src/render3/instructions/element.ts
Expand Up @@ -39,8 +39,12 @@ function elementStartFirstCreatePass(
resolveDirectives(tView, lView, tNode, getConstant<string[]>(tViewConsts, localRefsIndex));
ngDevMode && warnAboutUnknownElement(tView, lView, native, tNode, hasDirectives);

if (tNode.attrs !== null) {
computeStaticStyling(tNode, tNode.attrs, false);
}

if (tNode.mergedAttrs !== null) {
computeStaticStyling(tNode, tNode.mergedAttrs);
computeStaticStyling(tNode, tNode.mergedAttrs, true);
}

if (tView.queries !== null) {
Expand Down Expand Up @@ -148,12 +152,12 @@ export function ɵɵelementEnd(): void {
}
}

if (tNode.classes !== null && hasClassInput(tNode)) {
setDirectiveInputsWhichShadowsStyling(tView, tNode, getLView(), tNode.classes, true);
if (tNode.classesWithoutHost != null && hasClassInput(tNode)) {
setDirectiveInputsWhichShadowsStyling(tView, tNode, getLView(), tNode.classesWithoutHost, true);
}

if (tNode.styles !== null && hasStyleInput(tNode)) {
setDirectiveInputsWhichShadowsStyling(tView, tNode, getLView(), tNode.styles, false);
if (tNode.stylesWithoutHost != null && hasStyleInput(tNode)) {
setDirectiveInputsWhichShadowsStyling(tView, tNode, getLView(), tNode.stylesWithoutHost, false);
}
}

Expand Down
Expand Up @@ -33,7 +33,7 @@ function elementContainerStartFirstCreatePass(
// While ng-container doesn't necessarily support styling, we use the style context to identify
// and execute directives on the ng-container.
if (attrs !== null) {
computeStaticStyling(tNode, attrs);
computeStaticStyling(tNode, attrs, true);
}

const localRefs = getConstant<string[]>(tViewConsts, localRefsIndex);
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/render3/instructions/lview_debug.ts
Expand Up @@ -179,8 +179,10 @@ class TNode implements ITNode {
public parent: TElementNode|TContainerNode|null, //
public projection: number|(ITNode|RNode[])[]|null, //
public styles: string|null, //
public stylesWithoutHost: string|null, //
public residualStyles: KeyValueArray<any>|undefined|null, //
public classes: string|null, //
public classesWithoutHost: string|null, //
public residualClasses: KeyValueArray<any>|undefined|null, //
public classBindings: TStylingRange, //
public styleBindings: TStylingRange, //
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/render3/instructions/shared.ts
Expand Up @@ -851,8 +851,10 @@ export function createTNode(
tParent, // parent: TElementNode|TContainerNode|null
null, // projection: number|(ITNode|RNode[])[]|null
null, // styles: string|null
null, // stylesWithoutHost: string|null
undefined, // residualStyles: string|null
null, // classes: string|null
null, // classesWithoutHost: string|null
undefined, // residualClasses: string|null
0 as any, // classBindings: TStylingRange;
0 as any, // styleBindings: TStylingRange;
Expand Down Expand Up @@ -881,8 +883,10 @@ export function createTNode(
parent: tParent,
projection: null,
styles: null,
stylesWithoutHost: null,
residualStyles: undefined,
classes: null,
classesWithoutHost: null,
residualClasses: undefined,
classBindings: 0 as any,
styleBindings: 0 as any,
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/render3/instructions/styling.ts
Expand Up @@ -257,7 +257,7 @@ export function checkStylingMap(
// the binding has removed it. This would confuse `[ngStyle]`/`[ngClass]` to do the wrong
// thing as it would think that the static portion was removed. For this reason we
// concatenate it so that `[ngStyle]`/`[ngClass]` can continue to work on changed.
let staticPrefix = isClassBased ? tNode.classes : tNode.styles;
let staticPrefix = isClassBased ? tNode.classesWithoutHost : tNode.stylesWithoutHost;
ngDevMode && isClassBased === false && staticPrefix !== null &&
assertEqual(
staticPrefix.endsWith(';'), true, 'Expecting static portion to end with \';\'');
Expand Down
37 changes: 34 additions & 3 deletions packages/core/src/render3/interfaces/node.ts
Expand Up @@ -502,14 +502,30 @@ export interface TNode {
projection: (TNode|RNode[])[]|number|null;

/**
* A collection of all style static values for an element.
* A collection of all `style` static values for an element (including from host).
*
* This field will be populated if and when:
*
* - There are one or more initial styles on an element (e.g. `<div style="width:200px">`)
* - There are one or more initial `style`s on an element (e.g. `<div style="width:200px;">`)
* - There are one or more initial `style`s on a directive/component host
* (e.g. `@Directive({host: {style: "width:200px;" } }`)
*/
styles: string|null;


/**
* A collection of all `style` static values for an element excluding host sources.
*
* Populated when there are one or more initial `style`s on an element
* (e.g. `<div style="width:200px;">`)
* Must be stored separately from `tNode.styles` to facilitate setting directive
* inputs that shadow the `style` property. If we used `tNode.styles` as is for shadowed inputs,
* we would feed host styles back into directives as "inputs". If we used `tNode.attrs`, we would
* have to concatenate the attributes on every template pass. Instead, we process once on first
* create pass and store here.
*/
stylesWithoutHost: string|null;

/**
* A `KeyValueArray` version of residual `styles`.
*
Expand Down Expand Up @@ -540,14 +556,29 @@ export interface TNode {
residualStyles: KeyValueArray<any>|undefined|null;

/**
* A collection of all class static values for an element.
* A collection of all class static values for an element (including from host).
*
* This field will be populated if and when:
*
* - There are one or more initial classes on an element (e.g. `<div class="one two three">`)
* - There are one or more initial classes on an directive/component host
* (e.g. `@Directive({host: {class: "SOME_CLASS" } }`)
*/
classes: string|null;

/**
* A collection of all class static values for an element excluding host sources.
*
* Populated when there are one or more initial classes on an element
* (e.g. `<div class="SOME_CLASS">`)
* Must be stored separately from `tNode.classes` to facilitate setting directive
* inputs that shadow the `class` property. If we used `tNode.classes` as is for shadowed inputs,
* we would feed host classes back into directives as "inputs". If we used `tNode.attrs`, we would
* have to concatenate the attributes on every template pass. Instead, we process once on first
* create pass and store here.
*/
classesWithoutHost: string|null;

/**
* A `KeyValueArray` version of residual `classes`.
*
Expand Down
36 changes: 21 additions & 15 deletions packages/core/src/render3/styling/static_styling.ts
Expand Up @@ -18,25 +18,31 @@ import {getTView} from '../state';
*
* @param tNode The `TNode` into which the styling information should be loaded.
* @param attrs `TAttributes` containing the styling information.
* @param writeToHost Where should the resulting static styles be written?
* - `false` Write to `TNode.stylesWithoutHost` / `TNode.classesWithoutHost`
* - `true` Write to `TNode.styles` / `TNode.classes`
*/
export function computeStaticStyling(tNode: TNode, attrs: TAttributes): void {
export function computeStaticStyling(
tNode: TNode, attrs: TAttributes|null, writeToHost: boolean): void {
ngDevMode &&
assertFirstCreatePass(getTView(), 'Expecting to be called in first template pass only');
let styles: string|null = tNode.styles;
let classes: string|null = tNode.classes;
let styles: string|null = writeToHost ? tNode.styles : null;
let classes: string|null = writeToHost ? tNode.classes : null;
let mode: AttributeMarker|0 = 0;
for (let i = 0; i < attrs.length; i++) {
const value = attrs[i];
if (typeof value === 'number') {
mode = value;
} else if (mode == AttributeMarker.Classes) {
classes = concatStringsWithSpace(classes, value as string);
} else if (mode == AttributeMarker.Styles) {
const style = value as string;
const styleValue = attrs[++i] as string;
styles = concatStringsWithSpace(styles, style + ': ' + styleValue + ';');
if (attrs !== null) {
for (let i = 0; i < attrs.length; i++) {
const value = attrs[i];
if (typeof value === 'number') {
mode = value;
} else if (mode == AttributeMarker.Classes) {
classes = concatStringsWithSpace(classes, value as string);
} else if (mode == AttributeMarker.Styles) {
const style = value as string;
const styleValue = attrs[++i] as string;
styles = concatStringsWithSpace(styles, style + ': ' + styleValue + ';');
}
}
}
styles !== null && (tNode.styles = styles);
classes !== null && (tNode.classes = classes);
writeToHost ? tNode.styles = styles : tNode.stylesWithoutHost = styles;
writeToHost ? tNode.classes = classes : tNode.classesWithoutHost = classes;
}
83 changes: 78 additions & 5 deletions packages/core/test/acceptance/styling_spec.ts
Expand Up @@ -308,14 +308,87 @@ describe('styling', () => {
// VE has weird behavior where it calls the @Input('class') with either `class="static` or
// `[class]="dynamic"` but never both. This is determined at compile time. Due to locality
// we don't know if `[class]` is coming if we see `class` only. So we need to combine the
// static and dynamic parte. This results in slightly different calling sequence, but should
// static and dynamic parts. This results in slightly different calling sequence, but should
// result in the same final DOM.
expect(div1.getAttribute('shadow-class')).toEqual('s1 d1');

expect(div2.className).toEqual('');
expect(div2.getAttribute('shadow-class')).toEqual('s2 d2');
});

it('should not feed host classes back into shadow input', () => {
@Component({
template: `
<div class="s1" dir-shadows-class-input></div>
<div class="s1" [class]=" 'd1' " dir-shadows-class-input></div>
`
})
class Cmp {
}

@Directive({selector: '[dir-shadows-class-input]', host: {'class': 'DIRECTIVE'}})
class DirectiveShadowsClassInput {
constructor(private elementRef: ElementRef) {}
@Input('class')
set klass(value: string) {
this.elementRef.nativeElement.setAttribute('shadow-class', value);
}
}

TestBed.configureTestingModule({declarations: [Cmp, DirectiveShadowsClassInput]});
const fixture = TestBed.createComponent(Cmp);
fixture.detectChanges();

const [divStatic, divBinding] = fixture.nativeElement.querySelectorAll('div');
expectClass(divStatic).toEqual({'DIRECTIVE': true, 's1': true});
expect(divStatic.getAttribute('shadow-class')).toEqual('s1');

expectClass(divBinding).toEqual({'DIRECTIVE': true, 's1': true});
// VE has weird behavior where it calls the @Input('class') with either `class="static` or
// `[class]="dynamic"` but never both. This is determined at compile time. Due to locality
// we don't know if `[class]` is coming if we see `class` only. So we need to combine the
// static and dynamic parts. This results in slightly different calling sequence, but should
// result in the same final DOM.
expect(divBinding.getAttribute('shadow-class')).toEqual(ivyEnabled ? 's1 d1' : 'd1');
});

it('should not feed host style back into shadow input', () => {
@Component({
template: `
<div style="width: 1px;" dir-shadows-class-input></div>
<div style="width: 1px;" [style]=" 'height:1px;' " dir-shadows-class-input></div>
`
})
class Cmp {
}

@Directive({selector: '[dir-shadows-class-input]', host: {'style': 'color: red;'}})
class DirectiveShadowsStyleInput {
constructor(private elementRef: ElementRef) {}
@Input('style')
set style(value: string) {
this.elementRef.nativeElement.setAttribute('shadow-style', value);
}
}

TestBed.configureTestingModule({declarations: [Cmp, DirectiveShadowsStyleInput]});
const fixture = TestBed.createComponent(Cmp);
fixture.detectChanges();

const [divStatic, divBinding] = fixture.nativeElement.querySelectorAll('div');
expectStyle(divStatic).toEqual({'color': 'red', 'width': '1px'});
expect(divStatic.getAttribute('shadow-style')).toEqual('width: 1px;');

expectStyle(divBinding).toEqual({'color': 'red', 'width': '1px'});
// VE has weird behavior where it calls the @Input('style') with either `style="static` or
// `[style]="dynamic"` but never both. This is determined at compile time. Due to locality
// we don't know if `[style]` is coming if we see `style` only. So we need to combine the
// static and dynamic parts. This results in slightly different calling sequence, but should
// result in the same final DOM.
expect(divBinding.getAttribute('shadow-style'))
.toEqual(ivyEnabled ? 'width: 1px; height:1px;' : 'height:1px;');
});

onlyInIvy('shadow bindings include static portion')
.it('should bind [class] as input to directive when both static and falsy dynamic values are present',
() => {
Expand Down Expand Up @@ -375,15 +448,15 @@ describe('styling', () => {
}

@Directive({selector: '[dir-shadows-style-input]'})
class DirectiveShadowsClassInput {
class DirectiveShadowsStyleInput {
constructor(private elementRef: ElementRef) {}
@Input('style')
set style(value: string) {
this.elementRef.nativeElement.setAttribute('shadow-style', value);
}
}

TestBed.configureTestingModule({declarations: [Cmp, DirectiveShadowsClassInput]});
TestBed.configureTestingModule({declarations: [Cmp, DirectiveShadowsStyleInput]});
const fixture = TestBed.createComponent(Cmp);
fixture.detectChanges();

Expand All @@ -410,15 +483,15 @@ describe('styling', () => {
}

@Directive({selector: '[dir-shadows-style-input]'})
class DirectiveShadowsClassInput {
class DirectiveShadowsStyleInput {
constructor(private elementRef: ElementRef) {}
@Input('style')
set style(value: string) {
this.elementRef.nativeElement.setAttribute('shadow-style', value);
}
}

TestBed.configureTestingModule({declarations: [Cmp, DirectiveShadowsClassInput]});
TestBed.configureTestingModule({declarations: [Cmp, DirectiveShadowsStyleInput]});
const fixture = TestBed.createComponent(Cmp);
fixture.detectChanges();

Expand Down
Expand Up @@ -20,7 +20,7 @@ describe('static styling', () => {
tNode = createTNode(null!, null!, TNodeType.Element, 0, '', null);
});
it('should initialize when no attrs', () => {
computeStaticStyling(tNode, []);
computeStaticStyling(tNode, [], true);
expect(tNode.classes).toEqual(null);
expect(tNode.styles).toEqual(null);
});
Expand All @@ -31,7 +31,7 @@ describe('static styling', () => {
AttributeMarker.Classes, 'my-class', //
AttributeMarker.Styles, 'color', 'red' //
];
computeStaticStyling(tNode, tAttrs);
computeStaticStyling(tNode, tAttrs, true);
expect(tNode.classes).toEqual('my-class');
expect(tNode.styles).toEqual('color: red;');
});
Expand All @@ -42,7 +42,7 @@ describe('static styling', () => {
AttributeMarker.Classes, 'my-class', 'other', //
AttributeMarker.Styles, 'color', 'red', 'width', '100px' //
];
computeStaticStyling(tNode, tAttrs);
computeStaticStyling(tNode, tAttrs, true);
expect(tNode.classes).toEqual('my-class other');
expect(tNode.styles).toEqual('color: red; width: 100px;');
});
Expand Down

0 comments on commit f872b69

Please sign in to comment.