From abd462858732141aedbc12644e93fa725103b660 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Sun, 8 Dec 2019 20:19:28 -0800 Subject: [PATCH] fix(common): ensure diffing in ngStyle/ngClass correctly emits value changes (#34307) Prior to this change, in Ivy mode ngStyle/ngClass would accidentally emit value changes for static (string-based) values even if the value itself had not changed. This patch ensures that the style/class diffing code is more strict and when it signals ngClass/ngStyle that there has been a value change. Fixes #34336, #34444 PR Close #34307 --- .../common/src/directives/styling_differ.ts | 122 ++++++++++++------ .../common/test/directives/ng_class_spec.ts | 15 +++ .../common/test/directives/ng_style_spec.ts | 29 ++--- .../test/directives/styling_differ_spec.ts | 113 ++++++++++++++++ 4 files changed, 222 insertions(+), 57 deletions(-) create mode 100644 packages/common/test/directives/styling_differ_spec.ts diff --git a/packages/common/src/directives/styling_differ.ts b/packages/common/src/directives/styling_differ.ts index 972c15337d..792ab1a44a 100644 --- a/packages/common/src/directives/styling_differ.ts +++ b/packages/common/src/directives/styling_differ.ts @@ -60,8 +60,20 @@ export class StylingDiffer { public readonly value: T|null = null; - private _lastSetValue: {[key: string]: any}|string|string[]|null = null; + /** + * The last set value that was applied via `setValue()` + */ + private _lastSetValue: {[key: string]: any}|string|string[]|undefined|null = undefined; + + /** + * The type of value that the `_lastSetValue` variable is + */ private _lastSetValueType: StylingDifferValueTypes = StylingDifferValueTypes.Null; + + /** + * Whether or not the last value change occurred because the variable itself changed reference + * (identity) + */ private _lastSetValueIdentityChange = false; constructor(private _name: string, private _options: StylingDifferOptions) {} @@ -75,21 +87,29 @@ export class StylingDiffer { * @param value the new styling value provided from the ngClass/ngStyle binding */ setValue(value: {[key: string]: any}|string[]|string|null) { - if (Array.isArray(value)) { - this._lastSetValueType = StylingDifferValueTypes.Array; - } else if (value instanceof Set) { - this._lastSetValueType = StylingDifferValueTypes.Set; - } else if (value && typeof value === 'string') { - if (!(this._options & StylingDifferOptions.AllowStringValue)) { - throw new Error(this._name + ' string values are not allowed'); + if (value !== this._lastSetValue) { + let type: StylingDifferValueTypes; + if (!value) { // matches empty strings, null, false and undefined + type = StylingDifferValueTypes.Null; + value = null; + } else if (Array.isArray(value)) { + type = StylingDifferValueTypes.Array; + } else if (value instanceof Set) { + type = StylingDifferValueTypes.Set; + } else if (typeof value === 'string') { + if (!(this._options & StylingDifferOptions.AllowStringValue)) { + throw new Error(this._name + ' string values are not allowed'); + } + type = StylingDifferValueTypes.String; + } else { + type = StylingDifferValueTypes.StringMap; } - this._lastSetValueType = StylingDifferValueTypes.String; - } else { - this._lastSetValueType = value ? StylingDifferValueTypes.Map : StylingDifferValueTypes.Null; - } - this._lastSetValueIdentityChange = true; - this._lastSetValue = value || null; + this._lastSetValueType = type; + this._lastSetValueIdentityChange = true; + this._lastSetValue = value; + this._processValueChange(true); + } } /** @@ -104,8 +124,28 @@ export class StylingDiffer { */ hasValueChanged(): boolean { let valueHasChanged = this._lastSetValueIdentityChange; - if (!valueHasChanged && !(this._lastSetValueType & StylingDifferValueTypes.Collection)) - return false; + if (!valueHasChanged && (this._lastSetValueType & StylingDifferValueTypes.Collection)) { + valueHasChanged = this._processValueChange(false); + } else { + // this is set to false in the event that the value is a collection. + // This way (if the identity hasn't changed), then the algorithm can + // diff the collection value to see if the contents have mutated + // (otherwise the value change was processed during the time when + // the variable changed). + this._lastSetValueIdentityChange = false; + } + return valueHasChanged; + } + + /** + * Examines the last set value to see if there was a change in data. + * + * @param hasIdentityChange whether or not the last set value changed in identity or not + * + * @returns true when the value has changed (either by identity or by shape if its a collection). + */ + private _processValueChange(hasIdentityChange: boolean) { + let valueHasChanged = hasIdentityChange; let finalValue: {[key: string]: any}|string|null = null; const trimValues = (this._options & StylingDifferOptions.TrimProperties) ? true : false; @@ -117,25 +157,27 @@ export class StylingDiffer { case StylingDifferValueTypes.String: const tokens = (this._lastSetValue as string).split(/\s+/g); if (this._options & StylingDifferOptions.ForceAsMap) { - finalValue = {}; - tokens.forEach((token, i) => (finalValue as{[key: string]: any})[token] = true); + finalValue = {} as{[key: string]: any}; + for (let i = 0; i < tokens.length; i++) { + finalValue[tokens[i]] = true; + } } else { - finalValue = tokens.reduce((str, token, i) => str + (i ? ' ' : '') + token); + finalValue = ''; + for (let i = 0; i < tokens.length; i++) { + finalValue += (i !== 0 ? ' ' : '') + tokens[i]; + } } break; // case 2: [input]="{key:value}" - case StylingDifferValueTypes.Map: + case StylingDifferValueTypes.StringMap: const map: {[key: string]: any} = this._lastSetValue as{[key: string]: any}; const keys = Object.keys(map); if (!valueHasChanged) { - if (this.value) { - // we know that the classExp value exists and that it is - // a map (otherwise an identity change would have occurred) - valueHasChanged = mapHasChanged(keys, this.value as{[key: string]: any}, map); - } else { - valueHasChanged = true; - } + // we know that the classExp value exists and that it is + // a map (otherwise an identity change would have occurred) + valueHasChanged = + this.value ? mapHasChanged(keys, this.value as{[key: string]: any}, map) : true; } if (valueHasChanged) { @@ -174,27 +216,27 @@ export class StylingDiffer { } /** - * Various options that are consumed by the [StylingDiffer] class. + * Various options that are consumed by the [StylingDiffer] class */ export const enum StylingDifferOptions { - None = 0b00000, - TrimProperties = 0b00001, - AllowSubKeys = 0b00010, - AllowStringValue = 0b00100, - AllowUnits = 0b01000, - ForceAsMap = 0b10000, + None = 0b00000, // + TrimProperties = 0b00001, // + AllowSubKeys = 0b00010, // + AllowStringValue = 0b00100, // + AllowUnits = 0b01000, // + ForceAsMap = 0b10000, // } /** * The different types of inputs that the [StylingDiffer] can deal with */ const enum StylingDifferValueTypes { - Null = 0b0000, - String = 0b0001, - Map = 0b0010, - Array = 0b0100, - Set = 0b1000, - Collection = 0b1110, + Null = 0b0000, // + String = 0b0001, // + StringMap = 0b0010, // + Array = 0b0100, // + Set = 0b1000, // + Collection = 0b1110, // } diff --git a/packages/common/test/directives/ng_class_spec.ts b/packages/common/test/directives/ng_class_spec.ts index 36706aa884..4d83a6ee21 100644 --- a/packages/common/test/directives/ng_class_spec.ts +++ b/packages/common/test/directives/ng_class_spec.ts @@ -352,6 +352,21 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing'; detectChangesAndExpectClassName('init baz'); })); }); + + describe('non-regression', () => { + + // https://github.com/angular/angular/issues/34336 + it('should not write to native node when a bound expression doesnt change', () => { + fixture = createTestComponent(`
`); + detectChangesAndExpectClassName('color-red'); + + // Overwrite CSS classes to make sure that ngClass is not doing any DOM manipulation (as + // there was no change to the expression bound to [ngClass]). + fixture.debugElement.children[0].nativeElement.className = ''; + detectChangesAndExpectClassName(''); + }); + + }); }); } diff --git a/packages/common/test/directives/ng_style_spec.ts b/packages/common/test/directives/ng_style_spec.ts index 3978fd7dac..2af393888e 100644 --- a/packages/common/test/directives/ng_style_spec.ts +++ b/packages/common/test/directives/ng_style_spec.ts @@ -158,27 +158,22 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing'; expectNativeEl(fixture).toHaveCssStyle({'font-size': '12px'}); })); - it('should skip keys that are set to undefined values', async(() => { - const template = `
`; + it('should not write to native node when a bound expression doesnt change', () => { - fixture = createTestComponent(template); + const template = `
`; - getComponent().expr = { - 'border-top-color': undefined, - 'border-top-style': undefined, - 'border-color': 'red', - 'border-style': 'solid', - 'border-width': '1rem', - }; + fixture = createTestComponent(template); - fixture.detectChanges(); + fixture.detectChanges(); + expectNativeEl(fixture).toHaveCssStyle({'color': 'red'}); - expectNativeEl(fixture).toHaveCssStyle({ - 'border-color': 'red', - 'border-style': 'solid', - 'border-width': '1rem', - }); - })); + // Overwrite native styles to make sure that ngClass is not doing any DOM manipulation (as + // there was no change to the expression bound to [ngStyle]). + fixture.debugElement.children[0].nativeElement.style.color = 'blue'; + fixture.detectChanges(); + expectNativeEl(fixture).toHaveCssStyle({'color': 'blue'}); + + }); }); } diff --git a/packages/common/test/directives/styling_differ_spec.ts b/packages/common/test/directives/styling_differ_spec.ts new file mode 100644 index 0000000000..0a9c9da53f --- /dev/null +++ b/packages/common/test/directives/styling_differ_spec.ts @@ -0,0 +1,113 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import {StylingDiffer, StylingDifferOptions} from '@angular/common/src/directives/styling_differ'; + +describe('StylingDiffer', () => { + it('should create a key/value object of values from a string', () => { + const d = new StylingDiffer( + 'ngClass', StylingDifferOptions.ForceAsMap | StylingDifferOptions.AllowStringValue); + expect(d.value).toEqual(null); + + d.setValue('one two'); + expect(d.value).toEqual({one: true, two: true}); + + d.setValue('three'); + expect(d.value).toEqual({three: true}); + }); + + it('should not emit that a value has changed if a new non-collection value was not set', () => { + const d = new StylingDiffer( + 'ngClass', StylingDifferOptions.ForceAsMap | StylingDifferOptions.AllowStringValue); + expect(d.value).toEqual(null); + + d.setValue('one two'); + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({one: true, two: true}); + expect(d.hasValueChanged()).toBeFalsy(); + expect(d.value).toEqual({one: true, two: true}); + + d.setValue('three'); + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({three: true}); + expect(d.hasValueChanged()).toBeFalsy(); + expect(d.value).toEqual({three: true}); + + d.setValue(null); + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual(null); + expect(d.hasValueChanged()).toBeFalsy(); + expect(d.value).toEqual(null); + }); + + it('should watch the contents of a StringMap value and emit new values if they change', () => { + const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap); + + const myMap: {[key: string]: any} = {}; + myMap['abc'] = true; + + d.setValue(myMap); + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({abc: true}); + expect(d.hasValueChanged()).toBeFalsy(); + + myMap['def'] = true; + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({abc: true, def: true}); + expect(d.hasValueChanged()).toBeFalsy(); + + delete myMap['abc']; + delete myMap['def']; + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({}); + expect(d.hasValueChanged()).toBeFalsy(); + }); + + it('should watch the contents of an Array value and emit new values if they change', () => { + const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap); + + const myArray: string[] = []; + myArray.push('abc'); + + d.setValue(myArray); + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({abc: true}); + expect(d.hasValueChanged()).toBeFalsy(); + + myArray.push('def'); + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({abc: true, def: true}); + expect(d.hasValueChanged()).toBeFalsy(); + + myArray.length = 0; + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({}); + expect(d.hasValueChanged()).toBeFalsy(); + }); + + it('should watch the contents of a Set value and emit new values if they change', () => { + const d = new StylingDiffer('ngClass', StylingDifferOptions.ForceAsMap); + + const mySet = new Set(); + mySet.add('abc'); + + d.setValue(mySet); + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({abc: true}); + expect(d.hasValueChanged()).toBeFalsy(); + + mySet.add('def'); + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({abc: true, def: true}); + expect(d.hasValueChanged()).toBeFalsy(); + + mySet.clear(); + expect(d.hasValueChanged()).toBeTruthy(); + expect(d.value).toEqual({}); + expect(d.hasValueChanged()).toBeFalsy(); + }); +});