fix(common): ensure diffing in ngStyle/ngClass correctly emits value …

…changes (angular#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 angular#34336, angular#34444

PR Close angular#34307

packages/common/src/directives/styling_differ.ts

@@ -60,8 +60,20 @@
 export class StylingDiffer<T> {
   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<T> {
    * @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<T> {
    */
   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<T> {
       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<T> {
 }
 
 /**
- * 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,  //
 }
 
 

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(`<div [ngClass]="{'color-red': true}"></div>`);
+        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('');
+      });
+
+    });
   });
 }
 

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 = `<div [ngStyle]="expr"></div>`;
+    it('should not write to native node when a bound expression doesnt change', () => {
 
-         fixture = createTestComponent(template);
+      const template = `<div [ngStyle]="{'color': 'red'}"></div>`;
 
-         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'});
+
+    });
 
   });
 }

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<string>();
+    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();
+  });
+});