fix(core): handle invalid classes in class array bindings (#49924)

crisbeto · thePunderWoman · commit 2fff8fadbeff · 2023-04-19T16:28:26.000Z
When binding an array to `class` like `[class]="['foo', 'bar']"`, the runtime treats it the same as a literal binding with all the values being `true`, e.g. `{foo: true, bar: true}`. While object literals can only have string keys, arrays can have any value which can lead to errors if the array contains non-string values. These changes add some logic to stringify the keys and ignore invalid ones. Fixes #48473. PR Close #49924
diff --git a/packages/core/src/render3/instructions/styling.ts b/packages/core/src/render3/instructions/styling.ts
@@ -136,7 +136,7 @@ export function styleStringParser(keyValueArray: KeyValueArray<any>, text: strin
  */
 export function ɵɵclassMap(classes: {[className: string]: boolean|undefined|null}|string|undefined|
                            null): void {
-  checkStylingMap(keyValueArraySet, classStringParser, classes, true);
+  checkStylingMap(classKeyValueArraySet, classStringParser, classes, true);
 }
 
 /**
@@ -616,6 +616,27 @@ export function styleKeyValueArraySet(keyValueArray: KeyValueArray<any>, key: st
   keyValueArraySet(keyValueArray, key, unwrapSafeValue(value));
 }
 
+/**
+ * Class-binding-specific function for setting the `value` for a `key`.
+ *
+ * See: `keyValueArraySet` for details
+ *
+ * @param keyValueArray KeyValueArray to add to.
+ * @param key Style key to add.
+ * @param value The value to set.
+ */
+export function classKeyValueArraySet(keyValueArray: KeyValueArray<any>, key: unknown, value: any) {
+  // We use `classList.add` to eventually add the CSS classes to the DOM node. Any value passed into
+  // `add` is stringified and added to the `class` attribute, e.g. even null, undefined or numbers
+  // will be added. Stringify the key here so that our internal data structure matches the value in
+  // the DOM. The only exceptions are empty strings and strings that contain spaces for which
+  // the browser throws an error. We ignore such values, because the error is somewhat cryptic.
+  const stringKey = String(key);
+  if (stringKey !== '' && !stringKey.includes(' ')) {
+    keyValueArraySet(keyValueArray, stringKey, value);
+  }
+}
+
 /**
  * Update map based styling.
  *
diff --git a/packages/core/test/acceptance/styling_spec.ts b/packages/core/test/acceptance/styling_spec.ts
@@ -300,6 +300,135 @@ describe('styling', () => {
     });
   });
 
+  describe('non-string class keys', () => {
+    it('should allow null in a class array binding', () => {
+      @Component({
+        template: `<div [class]="['a', null, 'c']" [class.extra]="true"></div>`,
+        standalone: true,
+      })
+      class Cmp {
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      const div = fixture.nativeElement.querySelector('div');
+      expect(div.getAttribute('class')).toBe('a c null extra');
+    });
+
+    it('should allow undefined in a class array binding', () => {
+      @Component({
+        template: `<div [class]="['a', undefined, 'c']" [class.extra]="true"></div>`,
+        standalone: true,
+      })
+      class Cmp {
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      const div = fixture.nativeElement.querySelector('div');
+      expect(div.getAttribute('class')).toBe('a c undefined extra');
+    });
+
+    it('should allow zero in a class array binding', () => {
+      @Component({
+        template: `<div [class]="['a', 0, 'c']" [class.extra]="true"></div>`,
+        standalone: true,
+      })
+      class Cmp {
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      const div = fixture.nativeElement.querySelector('div');
+      expect(div.getAttribute('class')).toBe('0 a c extra');
+    });
+
+    it('should allow false in a class array binding', () => {
+      @Component({
+        template: `<div [class]="['a', false, 'c']" [class.extra]="true"></div>`,
+        standalone: true,
+      })
+      class Cmp {
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      const div = fixture.nativeElement.querySelector('div');
+      expect(div.getAttribute('class')).toBe('a c false extra');
+    });
+
+    it('should ignore an empty string in a class array binding', () => {
+      @Component({
+        template: `<div [class]="['a', '', 'c']" [class.extra]="true"></div>`,
+        standalone: true,
+      })
+      class Cmp {
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      const div = fixture.nativeElement.querySelector('div');
+      expect(div.getAttribute('class')).toBe('a c extra');
+    });
+
+    it('should ignore a string containing spaces in a class array binding', () => {
+      @Component({
+        template: `<div [class]="['a', 'hello there', 'c']" [class.extra]="true"></div>`,
+        standalone: true,
+      })
+      class Cmp {
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      const div = fixture.nativeElement.querySelector('div');
+      expect(div.getAttribute('class')).toBe('a c extra');
+    });
+
+    it('should ignore a string containing spaces in a class object literal binding', () => {
+      @Component({
+        template:
+            `<div [class]="{a: true, 'hello there': true, c: true}" [class.extra]="true"></div>`,
+        standalone: true,
+      })
+      class Cmp {
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      const div = fixture.nativeElement.querySelector('div');
+      expect(div.getAttribute('class')).toBe('a c extra');
+    });
+
+    it('should ignore an object literal in a class array binding', () => {
+      @Component({
+        template: `<div [class]="['a', {foo: true}, 'c']" [class.extra]="true"></div>`,
+        standalone: true,
+      })
+      class Cmp {
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      const div = fixture.nativeElement.querySelector('div');
+      expect(div.getAttribute('class')).toBe('a c extra');
+    });
+
+    it('should handle a string array in a class array binding', () => {
+      @Component({
+        template: `<div [class]="['a', ['foo', 'bar'], 'c']" [class.extra]="true"></div>`,
+        standalone: true,
+      })
+      class Cmp {
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      const div = fixture.nativeElement.querySelector('div');
+      expect(div.getAttribute('class')).toBe('a c foo,bar extra');
+    });
+  });
+
   it('should bind [class] as input to directive', () => {
     @Component({
       template: `
