fix(ivy): ensure errors are thrown during checkNoChanges for style/class bindings (#33103)

Prior to this fix, all style/class bindings (e.g. `[style]` and
`[class.foo]`) would quietly update a binding value if and when the
current binding value changes during checkNoChanges.

With this patch, all styling instructions will properly check to see
if the value has changed during the second pass of detectChanges()
if checkNoChanges is active.

PR Close #33103

aio/src/app/app.component.spec.ts

@@ -1,5 +1,5 @@
 import { NO_ERRORS_SCHEMA, DebugElement } from '@angular/core';
-import { inject, ComponentFixture, TestBed, fakeAsync, tick } from '@angular/core/testing';
+import { inject, ComponentFixture, TestBed, fakeAsync, flushMicrotasks, tick } from '@angular/core/testing';
 import { Title } from '@angular/platform-browser';
 import { APP_BASE_HREF } from '@angular/common';
 import { HttpClient } from '@angular/common/http';
@@ -529,7 +529,8 @@ describe('AppComponent', () => {
       it('should call `scrollAfterRender` (via `onDocInserted`) when navigate to a new Doc', fakeAsync(() => {
         locationService.go('guide/pipes');
         tick(1); // triggers the HTTP response for the document
-        fixture.detectChanges(); // triggers the event that calls `onDocInserted`
+        fixture.detectChanges();  // passes the new doc to the `DocViewer`
+        flushMicrotasks();  // triggers the `DocViewer` event that calls `onDocInserted`
 
         expect(scrollAfterRenderSpy).toHaveBeenCalledWith(scrollDelay);
 

aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts

@@ -1,7 +1,7 @@
 import { ComponentFixture, TestBed } from '@angular/core/testing';
 import { Meta, Title } from '@angular/platform-browser';
 
-import { Observable, of } from 'rxjs';
+import { Observable, asapScheduler, of } from 'rxjs';
 
 import { FILE_NOT_FOUND_ID, FETCHING_ERROR_ID } from 'app/documents/document.service';
 import { Logger } from 'app/shared/logger.service';
@@ -21,6 +21,8 @@ describe('DocViewerComponent', () => {
   let docViewerEl: HTMLElement;
   let docViewer: TestDocViewerComponent;
 
+  const safeFlushAsapScheduler = () => asapScheduler.actions.length && asapScheduler.flush();
+
   beforeEach(() => {
     TestBed.configureTestingModule({
       imports: [CustomElementsModule, TestModule],
@@ -42,19 +44,20 @@ describe('DocViewerComponent', () => {
   describe('#doc', () => {
     let renderSpy: jasmine.Spy;
 
-    const setCurrentDoc = (contents: string|null, id = 'fizz/buzz') => {
-      parentComponent.currentDoc = {contents, id};
-      parentFixture.detectChanges();
+    const setCurrentDoc = (newDoc: TestParentComponent['currentDoc']) => {
+      parentComponent.currentDoc = newDoc && {id: 'fizz/buzz', ...newDoc};
+      parentFixture.detectChanges();  // Run change detection to propagate the new doc to `DocViewer`.
+      safeFlushAsapScheduler();  // Flush `asapScheduler` to trigger `DocViewer#render()`.
     };
 
     beforeEach(() => renderSpy = spyOn(docViewer, 'render').and.callFake(() => of(undefined)));
 
     it('should render the new document', () => {
-      setCurrentDoc('foo', 'bar');
+      setCurrentDoc({contents: 'foo', id: 'bar'});
       expect(renderSpy).toHaveBeenCalledTimes(1);
       expect(renderSpy.calls.mostRecent().args).toEqual([{id: 'bar', contents: 'foo'}]);
 
-      setCurrentDoc(null, 'baz');
+      setCurrentDoc({contents: null, id: 'baz'});
       expect(renderSpy).toHaveBeenCalledTimes(2);
       expect(renderSpy.calls.mostRecent().args).toEqual([{id: 'baz', contents: null}]);
     });
@@ -63,24 +66,20 @@ describe('DocViewerComponent', () => {
       const obs = new ObservableWithSubscriptionSpies();
       renderSpy.and.returnValue(obs);
 
-      setCurrentDoc('foo', 'bar');
+      setCurrentDoc({contents: 'foo', id: 'bar'});
       expect(obs.subscribeSpy).toHaveBeenCalledTimes(1);
       expect(obs.unsubscribeSpies[0]).not.toHaveBeenCalled();
 
-      setCurrentDoc('baz', 'qux');
+      setCurrentDoc({contents: 'baz', id: 'qux'});
       expect(obs.subscribeSpy).toHaveBeenCalledTimes(2);
       expect(obs.unsubscribeSpies[0]).toHaveBeenCalledTimes(1);
     });
 
     it('should ignore falsy document values', () => {
-      parentComponent.currentDoc = null;
-      parentFixture.detectChanges();
-
+      setCurrentDoc(null);
       expect(renderSpy).not.toHaveBeenCalled();
 
-      parentComponent.currentDoc = undefined;
-      parentFixture.detectChanges();
-
+      setCurrentDoc(undefined);
       expect(renderSpy).not.toHaveBeenCalled();
     });
   });
@@ -92,14 +91,17 @@ describe('DocViewerComponent', () => {
       expect(renderSpy).not.toHaveBeenCalled();
 
       docViewer.doc = {contents: 'Some content', id: 'some-id'};
+      safeFlushAsapScheduler();
       expect(renderSpy).toHaveBeenCalledTimes(1);
 
       docViewer.ngOnDestroy();
 
       docViewer.doc = {contents: 'Other content', id: 'other-id'};
+      safeFlushAsapScheduler();
       expect(renderSpy).toHaveBeenCalledTimes(1);
 
       docViewer.doc = {contents: 'More content', id: 'more-id'};
+      safeFlushAsapScheduler();
       expect(renderSpy).toHaveBeenCalledTimes(1);
     });
   });

aio/src/app/layout/doc-viewer/doc-viewer.component.ts

@@ -1,8 +1,8 @@
 import { Component, ElementRef, EventEmitter, Input, OnDestroy, Output } from '@angular/core';
 import { Title, Meta } from '@angular/platform-browser';
 
-import { Observable, of, timer } from 'rxjs';
-import { catchError, switchMap, takeUntil, tap } from 'rxjs/operators';
+import { asapScheduler, Observable, of, timer } from 'rxjs';
+import { catchError, observeOn, switchMap, takeUntil, tap } from 'rxjs/operators';
 
 import { DocumentContents, FILE_NOT_FOUND_ID, FETCHING_ERROR_ID } from 'app/documents/document.service';
 import { Logger } from 'app/shared/logger.service';
@@ -78,6 +78,7 @@ export class DocViewerComponent implements OnDestroy {
 
     this.docContents$
         .pipe(
+            observeOn(asapScheduler),
             switchMap(newDoc => this.render(newDoc)),
             takeUntil(this.onDestroy$),
         )

packages/core/src/render3/instructions/styling.ts

@@ -7,19 +7,20 @@
 */
 import {SafeValue} from '../../sanitization/bypass';
 import {StyleSanitizeFn} from '../../sanitization/style_sanitizer';
+import {throwErrorIfNoChangesMode} from '../errors';
 import {setInputsForProperty} from '../instructions/shared';
 import {AttributeMarker, TAttributes, TNode, TNodeFlags, TNodeType} from '../interfaces/node';
 import {RElement} from '../interfaces/renderer';
 import {StylingMapArray, StylingMapArrayIndex, TStylingConfig, TStylingContext} from '../interfaces/styling';
 import {isDirectiveHost} from '../interfaces/type_checks';
 import {BINDING_INDEX, LView, RENDERER} from '../interfaces/view';
-import {getActiveDirectiveId, getCurrentStyleSanitizer, getLView, getSelectedIndex, resetCurrentStyleSanitizer, setCurrentStyleSanitizer, setElementExitFn} from '../state';
+import {getActiveDirectiveId, getCheckNoChangesMode, getCurrentStyleSanitizer, getLView, getSelectedIndex, resetCurrentStyleSanitizer, setCurrentStyleSanitizer, setElementExitFn} from '../state';
 import {applyStylingMapDirectly, applyStylingValueDirectly, flushStyling, setClass, setStyle, updateClassViaContext, updateStyleViaContext} from '../styling/bindings';
 import {activateStylingMapFeature} from '../styling/map_based_bindings';
 import {attachStylingDebugObject} from '../styling/styling_debug';
 import {NO_CHANGE} from '../tokens';
 import {renderStringify} from '../util/misc_utils';
-import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, hasClassInput, hasStyleInput, hasValueChanged, isContextLocked, isHostStylingActive, isStylingContext, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils';
+import {addItemToStylingMap, allocStylingMapArray, allocTStylingContext, allowDirectStyling, concatString, forceClassesAsString, forceStylesAsString, getInitialStylingValue, getStylingMapArray, getValue, hasClassInput, hasStyleInput, hasValueChanged, isContextLocked, isHostStylingActive, isStylingContext, normalizeIntoStylingMap, patchConfig, selectClassBasedInputName, setValue, stylingMapToString} from '../util/styling_utils';
 import {getNativeByTNode, getTNode} from '../util/view_utils';
 
 
@@ -171,6 +172,17 @@ function stylingProp(
     patchConfig(context, TStylingConfig.HasPropBindings);
   }
 
+  // [style.prop] and [class.name] bindings do not use `bind()` and will
+  // therefore manage accessing and updating the new value in the lView directly.
+  // For this reason, the checkNoChanges situation must also be handled here
+  // as well.
+  if (ngDevMode && getCheckNoChangesMode()) {
+    const oldValue = getValue(lView, bindingIndex);
+    if (hasValueChanged(oldValue, value)) {
+      throwErrorIfNoChangesMode(false, oldValue, value);
+    }
+  }
+
   // Direct Apply Case: bypass context resolution and apply the
   // style/class value directly to the element
   if (allowDirectStyling(context, hostBindingsMode)) {
@@ -246,7 +258,7 @@ export function ɵɵstyleMap(styles: {[styleName: string]: any} | NO_CHANGE | nu
     styles = NO_CHANGE;
   }
 
-  const updated = _stylingMap(index, context, bindingIndex, styles, false);
+  const updated = stylingMap(index, context, bindingIndex, styles, false);
   if (ngDevMode) {
     ngDevMode.styleMap++;
     if (updated) {
@@ -303,7 +315,7 @@ export function classMapInternal(
     classes = NO_CHANGE;
   }
 
-  const updated = _stylingMap(elementIndex, context, bindingIndex, classes, true);
+  const updated = stylingMap(elementIndex, context, bindingIndex, classes, true);
   if (ngDevMode) {
     ngDevMode.classMap++;
     if (updated) {
@@ -318,7 +330,7 @@ export function classMapInternal(
  * When this function is called it will activate support for `[style]` and
  * `[class]` bindings in Angular.
  */
-function _stylingMap(
+function stylingMap(
     elementIndex: number, context: TStylingContext, bindingIndex: number,
     value: {[key: string]: any} | string | null, isClassBased: boolean): boolean {
   let updated = false;
@@ -327,9 +339,18 @@ function _stylingMap(
   const directiveIndex = getActiveDirectiveId();
   const tNode = getTNode(elementIndex, lView);
   const native = getNativeByTNode(tNode, lView) as RElement;
-  const oldValue = lView[bindingIndex] as StylingMapArray | null;
+  const oldValue = getValue(lView, bindingIndex);
   const hostBindingsMode = isHostStyling();
   const sanitizer = getCurrentStyleSanitizer();
+  const valueHasChanged = hasValueChanged(oldValue, value);
+
+  // [style] and [class] bindings do not use `bind()` and will therefore
+  // manage accessing and updating the new value in the lView directly.
+  // For this reason, the checkNoChanges situation must also be handled here
+  // as well.
+  if (ngDevMode && valueHasChanged && getCheckNoChangesMode()) {
+    throwErrorIfNoChangesMode(false, oldValue, value);
+  }
 
   // we check for this in the instruction code so that the context can be notified
   // about prop or map bindings so that the direct apply check can decide earlier
@@ -338,7 +359,6 @@ function _stylingMap(
     patchConfig(context, TStylingConfig.HasMapBindings);
   }
 
-  const valueHasChanged = hasValueChanged(oldValue, value);
   const stylingMapArr =
       value === NO_CHANGE ? NO_CHANGE : normalizeIntoStylingMap(oldValue, value, !isClassBased);
 
@@ -479,12 +499,12 @@ export function registerInitialStylingOnTNode(
     if (typeof attr == 'number') {
       mode = attr;
     } else if (mode == AttributeMarker.Classes) {
-      classes = classes || allocStylingMapArray();
+      classes = classes || allocStylingMapArray(null);
       addItemToStylingMap(classes, attr, true);
       hasAdditionalInitialStyling = true;
     } else if (mode == AttributeMarker.Styles) {
       const value = attrs[++i] as string | null;
-      styles = styles || allocStylingMapArray();
+      styles = styles || allocStylingMapArray(null);
       addItemToStylingMap(styles, attr, value);
       hasAdditionalInitialStyling = true;
     }

packages/core/src/render3/interfaces/styling.ts

@@ -522,8 +522,11 @@ export type LStylingData = LView | (string | number | boolean | null)[];
  * of the key/value array that was used to populate the property/
  * value entries that take place in the remainder of the array.
  */
-export interface StylingMapArray extends Array<{}|string|number|null> {
-  [StylingMapArrayIndex.RawValuePosition]: {}|string|null;
+export interface StylingMapArray extends Array<{}|string|number|null|undefined> {
+  /**
+   * The last raw value used to generate the entries in the map.
+   */
+  [StylingMapArrayIndex.RawValuePosition]: {}|string|number|null|undefined;
 }
 
 /**

packages/core/src/render3/util/styling_utils.ts

@@ -43,7 +43,7 @@ export const DEFAULT_GUARD_MASK_VALUE = 0b1;
  */
 export function allocTStylingContext(
     initialStyling: StylingMapArray | null, hasDirectives: boolean): TStylingContext {
-  initialStyling = initialStyling || allocStylingMapArray();
+  initialStyling = initialStyling || allocStylingMapArray(null);
   let config = TStylingConfig.Initial;
   if (hasDirectives) {
     config |= TStylingConfig.HasDirectives;
@@ -58,8 +58,8 @@ export function allocTStylingContext(
   ];
 }
 
-export function allocStylingMapArray(): StylingMapArray {
-  return [''];
+export function allocStylingMapArray(value: {} | string | null): StylingMapArray {
+  return [value];
 }
 
 export function getConfig(context: TStylingContext) {
@@ -169,7 +169,7 @@ export function setValue(data: LStylingData, bindingIndex: number, value: any) {
 }
 
 export function getValue<T = any>(data: LStylingData, bindingIndex: number): T|null {
-  return bindingIndex > 0 ? data[bindingIndex] as T : null;
+  return bindingIndex !== 0 ? data[bindingIndex] as T : null;
 }
 
 export function lockContext(context: TStylingContext, hostBindingsMode: boolean): void {
@@ -207,7 +207,7 @@ export function hasValueChanged(
 /**
  * Determines whether the provided styling value is truthy or falsy.
  */
-export function isStylingValueDefined<T extends string|number|{}|null>(value: T):
+export function isStylingValueDefined<T extends string|number|{}|null|undefined>(value: T):
     value is NonNullable<T> {
   // the reason why null is compared against is because
   // a CSS class value that is set to `false` must be
@@ -396,8 +396,9 @@ export function normalizeIntoStylingMap(
     bindingValue: null | StylingMapArray,
     newValues: {[key: string]: any} | string | null | undefined,
     normalizeProps?: boolean): StylingMapArray {
-  const stylingMapArr: StylingMapArray = Array.isArray(bindingValue) ? bindingValue : [null];
-  stylingMapArr[StylingMapArrayIndex.RawValuePosition] = newValues || null;
+  const stylingMapArr: StylingMapArray =
+      Array.isArray(bindingValue) ? bindingValue : allocStylingMapArray(null);
+  stylingMapArr[StylingMapArrayIndex.RawValuePosition] = newValues;
 
   // because the new values may not include all the properties
   // that the old ones had, all values are set to `null` before

packages/core/test/acceptance/styling_spec.ts

@@ -2241,6 +2241,57 @@ describe('styling', () => {
        fixture.detectChanges();
        expect(div.classList.contains('disabled')).toBe(false);
      });
+
+  it('should throw an error if a prop-based style/class binding value is changed during checkNoChanges',
+     () => {
+       @Component({
+         template: `
+        <div [style.color]="color" [class.foo]="fooClass"></div>
+      `
+       })
+       class Cmp {
+         color = 'red';
+         fooClass = true;
+
+         ngAfterViewInit() {
+           this.color = 'blue';
+           this.fooClass = false;
+         }
+       }
+
+       TestBed.configureTestingModule({declarations: [Cmp]});
+       const fixture = TestBed.createComponent(Cmp);
+
+       expect(() => {
+         fixture.detectChanges();
+       }).toThrowError(/ExpressionChangedAfterItHasBeenCheckedError/);
+     });
+
+  onlyInIvy('only ivy allows for map-based style AND class bindings')
+      .it('should throw an error if a map-based style/class binding value is changed during checkNoChanges',
+          () => {
+            @Component({
+              template: `
+                <div [style]="style" [class]="klass"></div>
+              `
+            })
+            class Cmp {
+              style: any = {width: '100px'};
+              klass: any = {foo: true, bar: false};
+
+              ngAfterViewInit() {
+                this.style = {height: '200px'};
+                this.klass = {foo: false};
+              }
+            }
+
+            TestBed.configureTestingModule({declarations: [Cmp]});
+            const fixture = TestBed.createComponent(Cmp);
+
+            expect(() => {
+              fixture.detectChanges();
+            }).toThrowError(/ExpressionChangedAfterItHasBeenCheckedError/);
+          });
 });
 
 function assertStyleCounters(countForSet: number, countForRemove: number) {

packages/core/test/render3/styling_next/map_based_bindings_spec.ts

@@ -81,5 +81,5 @@ describe('map-based bindings', () => {
 
 function createAndAssertValues(newValue: any, entries: any[]) {
   const result = createMap(null, newValue);
-  expect(result).toEqual([newValue || null, ...entries]);
+  expect(result).toEqual([newValue, ...entries]);
 }