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

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