refactor(ivy): get rid of styling cleanup functions outside of styling code (#32591)

Prior to this patch, each time `advance()` would run (or when a
templateFn or hostBindings code exits) then the core change detection
code would check to see whether the styling data needs to be reset. This
patch removes that functionality and places everything inside of the
scheduled styling exit function. This means that each time one or more
styling bindings run (even if the value hasn't changed) then an exit
function will be scheduled and that will do all the cleanup.

PR Close #32591

integration/_payload-limits.json

@@ -12,7 +12,7 @@
     "master": {
       "uncompressed": {
         "runtime": 1440,
-        "main": 13604,
+        "main": 13415,
         "polyfills": 45340
       }
     }

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

@@ -9,7 +9,6 @@ import {assertDataInRange, assertGreaterThan} from '../../util/assert';
 import {executeCheckHooks, executeInitAndCheckHooks} from '../hooks';
 import {FLAGS, HEADER_OFFSET, InitPhaseState, LView, LViewFlags, TVIEW} from '../interfaces/view';
 import {ActiveElementFlags, executeElementExitFn, getCheckNoChangesMode, getLView, getSelectedIndex, hasActiveElementFlag, setSelectedIndex} from '../state';
-import {resetStylingState} from '../styling_next/state';
 
 
 
@@ -76,10 +75,6 @@ export function selectIndexInternal(lView: LView, index: number, checkNoChangesM
     }
   }
 
-  if (hasActiveElementFlag(ActiveElementFlags.ResetStylesOnExit)) {
-    resetStylingState();
-  }
-
   // We must set the selected index *after* running the hooks, because hooks may have side-effects
   // that cause other template functions to run, thus updating the selected index, which is global
   // state. If we run `setSelectedIndex` *before* we run the hooks, in some cases the selected index

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

@@ -32,7 +32,6 @@ import {assertNodeOfPossibleTypes} from '../node_assert';
 import {isNodeMatchingSelectorList} from '../node_selector_matcher';
 import {ActiveElementFlags, executeElementExitFn, getBindingsEnabled, getCheckNoChangesMode, getIsParent, getPreviousOrParentTNode, getSelectedIndex, hasActiveElementFlag, incrementActiveDirectiveId, namespaceHTMLInternal, selectView, setActiveHostElement, setBindingRoot, setCheckNoChangesMode, setCurrentDirectiveDef, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state';
 import {renderStylingMap} from '../styling_next/bindings';
-import {resetStylingState} from '../styling_next/state';
 import {NO_CHANGE} from '../tokens';
 import {ANIMATION_PROP_PREFIX, isAnimationProp} from '../util/attrs_utils';
 import {INTERPOLATION_DELIMITER, renderStringify, stringifyForError} from '../util/misc_utils';
@@ -505,9 +504,6 @@ function executeTemplate<T>(
     if (hasActiveElementFlag(ActiveElementFlags.RunExitFn)) {
       executeElementExitFn();
     }
-    if (hasActiveElementFlag(ActiveElementFlags.ResetStylesOnExit)) {
-      resetStylingState();
-    }
     setSelectedIndex(prevSelectedIndex);
   }
 }

packages/core/src/render3/state.ts

@@ -12,8 +12,7 @@ import {assertDefined} from '../util/assert';
 import {assertLViewOrUndefined} from './assert';
 import {ComponentDef, DirectiveDef} from './interfaces/definition';
 import {TElementNode, TNode, TViewNode} from './interfaces/node';
-import {CONTEXT, DECLARATION_VIEW, LView, OpaqueViewState, TVIEW} from './interfaces/view';
+import {CONTEXT, DECLARATION_VIEW, LView, OpaqueViewState} from './interfaces/view';
-import {resetStylingState} from './styling_next/state';
 
 
 /**
@@ -144,8 +143,7 @@ let activeDirectiveId = 0;
 export const enum ActiveElementFlags {
   Initial = 0b00,
   RunExitFn = 0b01,
-  ResetStylesOnExit = 0b10,
+  Size = 1,
-  Size = 2,
 }
 
 /**
@@ -174,9 +172,6 @@ export function setActiveHostElement(elementIndex: number | null = null) {
     if (hasActiveElementFlag(ActiveElementFlags.RunExitFn)) {
       executeElementExitFn();
     }
-    if (hasActiveElementFlag(ActiveElementFlags.ResetStylesOnExit)) {
-      resetStylingState();
-    }
     setSelectedIndex(elementIndex === null ? -1 : elementIndex);
     activeDirectiveId = 0;
   }
@@ -390,6 +385,10 @@ export function setCurrentQueryIndex(value: number): void {
  * @returns the previously active lView;
  */
 export function selectView(newView: LView, hostTNode: TElementNode | TViewNode | null): LView {
+  if (hasActiveElementFlag(ActiveElementFlags.RunExitFn)) {
+    executeElementExitFn();
+  }
+
   ngDevMode && assertLViewOrUndefined(newView);
   const oldView = lView;
 

packages/core/src/render3/styling_next/bindings.ts

@@ -405,19 +405,23 @@ export function flushStyling(
     if (!isContextLocked(stylesContext, hostBindingsMode)) {
       lockAndFinalizeContext(stylesContext, hostBindingsMode);
     }
+    if (state.stylesBitMask !== 0) {
       applyStylingViaContext(
           stylesContext, renderer, element, data, state.stylesBitMask, setStyle, styleSanitizer,
           hostBindingsMode);
     }
+  }
 
   if (classesContext) {
     if (!isContextLocked(classesContext, hostBindingsMode)) {
       lockAndFinalizeContext(classesContext, hostBindingsMode);
     }
+    if (state.classesBitMask !== 0) {
       applyStylingViaContext(
           classesContext, renderer, element, data, state.classesBitMask, setClass, null,
           hostBindingsMode);
     }
+  }
 
   resetStylingState();
 }

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

@@ -11,7 +11,7 @@ import {setInputsForProperty} from '../instructions/shared';
 import {AttributeMarker, TAttributes, TNode, TNodeType} from '../interfaces/node';
 import {RElement} from '../interfaces/renderer';
 import {BINDING_INDEX, LView, RENDERER} from '../interfaces/view';
-import {ActiveElementFlags, getActiveDirectiveId, getCurrentStyleSanitizer, getLView, getSelectedIndex, setActiveElementFlag, setCurrentStyleSanitizer, setElementExitFn} from '../state';
+import {getActiveDirectiveId, getCurrentStyleSanitizer, getLView, getSelectedIndex, setCurrentStyleSanitizer, setElementExitFn} from '../state';
 import {NO_CHANGE} from '../tokens';
 import {renderStringify} from '../util/misc_utils';
 import {getNativeByTNode, getTNode} from '../util/view_utils';
@@ -186,10 +186,7 @@ function stylingProp(
           value as string | SafeValue | null, sanitizer);
     }
 
-    if (updated) {
+    setElementExitFn(stylingApply);
-      setElementExitFn(applyStyling);
-    }
-    markStylingStateAsDirty();
   }
 
   return updated;
@@ -331,6 +328,9 @@ function _stylingMap(
         renderer, context, native, lView, bindingIndex, stylingMapArr as StylingMapArray,
         isClassBased ? setClass : setStyle, sanitizer, valueHasChanged);
   } else {
+    updated = valueHasChanged;
+    activateStylingMapFeature();
+
     // Context Resolution (or first update) Case: save the map value
     // and defer to the context to flush and apply the style/class binding
     // value to the element.
@@ -344,15 +344,9 @@ function _stylingMap(
           valueHasChanged);
     }
 
-    if (valueHasChanged) {
+    setElementExitFn(stylingApply);
-      updated = true;
-      setElementExitFn(applyStyling);
   }
 
-    activateStylingMapFeature();
-  }
-
-  markStylingStateAsDirty();
   return updated;
 }
 
@@ -384,9 +378,9 @@ function updateDirectiveInputValue(
       const initialValue = getInitialStylingValue(context);
       const value = normalizeStylingDirectiveInputValue(initialValue, newValue, isClassBased);
       setInputsForProperty(lView, inputs, value);
+      setElementExitFn(stylingApply);
     }
     setValue(lView, bindingIndex, newValue);
-    setElementExitFn(applyStyling);
   }
 }
 
@@ -423,17 +417,17 @@ function normalizeStylingDirectiveInputValue(
  * in this file. When called it will flush all style and class bindings to the element
  * via the context resolution algorithm.
  */
-function applyStyling(): void {
+function stylingApply(): void {
-  const elementIndex = getSelectedIndex();
   const lView = getLView();
+  const elementIndex = getSelectedIndex();
   const tNode = getTNode(elementIndex, lView);
-  const renderer = getRenderer(tNode, lView);
   const native = getNativeByTNode(tNode, lView) as RElement;
+  const directiveIndex = getActiveDirectiveId();
+  const renderer = getRenderer(tNode, lView);
   const sanitizer = getCurrentStyleSanitizer();
   const classesContext = isStylingContext(tNode.classes) ? tNode.classes as TStylingContext : null;
   const stylesContext = isStylingContext(tNode.styles) ? tNode.styles as TStylingContext : null;
-  flushStyling(
+  flushStyling(renderer, lView, classesContext, stylesContext, native, directiveIndex, sanitizer);
-      renderer, lView, classesContext, stylesContext, native, getActiveDirectiveId(), sanitizer);
   setCurrentStyleSanitizer(null);
 }
 
@@ -538,10 +532,6 @@ function resolveStylePropValue(
   return resolvedValue;
 }
 
-function markStylingStateAsDirty(): void {
-  setActiveElementFlag(ActiveElementFlags.ResetStylesOnExit);
-}
-
 /**
  * Whether or not the style/class binding being applied was executed within a host bindings
  * function.

packages/core/test/acceptance/styling_next_spec.ts

@@ -5,7 +5,7 @@
  * 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 {Component, Directive, HostBinding, Input, ViewChild} from '@angular/core';
+import {Component, ComponentFactoryResolver, ComponentRef, Directive, HostBinding, Input, NgModule, ViewChild, ViewContainerRef} from '@angular/core';
 import {DebugNode, LViewDebug, toDebug} from '@angular/core/src/render3/instructions/lview_debug';
 import {loadLContextFromNode} from '@angular/core/src/render3/util/discovery_utils';
 import {ngDevModeResetPerfCounters as resetStylingCounters} from '@angular/core/src/util/ng_dev_mode';
@@ -1168,6 +1168,175 @@ describe('new styling integration', () => {
     expect(div.classList.contains('foo')).toBeTruthy();
     expect(div.classList.contains('bar')).toBeTruthy();
   });
+
+  it('should allow detectChanges to be run in a property change that causes additional styling to be rendered',
+     () => {
+       @Component({
+         selector: 'child',
+         template: `
+          <div [class.ready-child]="readyTpl"></div>
+        `,
+       })
+       class ChildCmp {
+         readyTpl = false;
+
+         @HostBinding('class.ready-host')
+         readyHost = false;
+       }
+
+       @Component({
+         selector: 'parent',
+         template: `
+        <div>
+          <div #template></div>
+          <p>{{prop}}</p>
+        </div>
+      `,
+         host: {
+           '[style.color]': 'color',
+         },
+       })
+       class ParentCmp {
+         private _prop = '';
+
+         @ViewChild('template', {read: ViewContainerRef, static: false})
+         vcr: ViewContainerRef = null !;
+
+         private child: ComponentRef<ChildCmp> = null !;
+
+         @Input()
+         set prop(value: string) {
+           this._prop = value;
+           if (this.child && value === 'go') {
+             this.child.instance.readyHost = true;
+             this.child.instance.readyTpl = true;
+             this.child.changeDetectorRef.detectChanges();
+           }
+         }
+
+         get prop() { return this._prop; }
+
+         ngAfterViewInit() {
+           const factory = this.componentFactoryResolver.resolveComponentFactory(ChildCmp);
+           this.child = this.vcr.createComponent(factory);
+         }
+
+         constructor(private componentFactoryResolver: ComponentFactoryResolver) {}
+       }
+
+       @Component({
+         template: `<parent [prop]="prop"></parent>`,
+       })
+       class App {
+         prop = 'a';
+       }
+
+       @NgModule({
+         entryComponents: [ChildCmp],
+         declarations: [ChildCmp],
+       })
+       class ChildCmpModule {
+       }
+
+       TestBed.configureTestingModule({declarations: [App, ParentCmp], imports: [ChildCmpModule]});
+       const fixture = TestBed.createComponent(App);
+       fixture.detectChanges(false);
+
+       let readyHost = fixture.nativeElement.querySelector('.ready-host');
+       let readyChild = fixture.nativeElement.querySelector('.ready-child');
+
+       expect(readyHost).toBeFalsy();
+       expect(readyChild).toBeFalsy();
+
+       fixture.componentInstance.prop = 'go';
+       fixture.detectChanges(false);
+
+       readyHost = fixture.nativeElement.querySelector('.ready-host');
+       readyChild = fixture.nativeElement.querySelector('.ready-child');
+       expect(readyHost).toBeTruthy();
+       expect(readyChild).toBeTruthy();
+     });
+
+  it('should allow detectChanges to be run in a hook that causes additional styling to be rendered',
+     () => {
+       @Component({
+         selector: 'child',
+         template: `
+          <div [class.ready-child]="readyTpl"></div>
+        `,
+       })
+       class ChildCmp {
+         readyTpl = false;
+
+         @HostBinding('class.ready-host')
+         readyHost = false;
+       }
+
+       @Component({
+         selector: 'parent',
+         template: `
+          <div>
+            <div #template></div>
+            <p>{{prop}}</p>
+          </div>
+        `,
+       })
+       class ParentCmp {
+         updateChild = false;
+
+         @ViewChild('template', {read: ViewContainerRef, static: false})
+         vcr: ViewContainerRef = null !;
+
+         private child: ComponentRef<ChildCmp> = null !;
+
+         ngDoCheck() {
+           if (this.updateChild) {
+             this.child.instance.readyHost = true;
+             this.child.instance.readyTpl = true;
+             this.child.changeDetectorRef.detectChanges();
+           }
+         }
+
+         ngAfterViewInit() {
+           const factory = this.componentFactoryResolver.resolveComponentFactory(ChildCmp);
+           this.child = this.vcr.createComponent(factory);
+         }
+
+         constructor(private componentFactoryResolver: ComponentFactoryResolver) {}
+       }
+
+       @Component({
+         template: `<parent #parent></parent>`,
+       })
+       class App {
+         @ViewChild('parent', {static: true}) public parent: ParentCmp|null = null;
+       }
+
+       @NgModule({
+         entryComponents: [ChildCmp],
+         declarations: [ChildCmp],
+       })
+       class ChildCmpModule {
+       }
+
+       TestBed.configureTestingModule({declarations: [App, ParentCmp], imports: [ChildCmpModule]});
+       const fixture = TestBed.createComponent(App);
+       fixture.detectChanges(false);
+
+       let readyHost = fixture.nativeElement.querySelector('.ready-host');
+       let readyChild = fixture.nativeElement.querySelector('.ready-child');
+       expect(readyHost).toBeFalsy();
+       expect(readyChild).toBeFalsy();
+
+       const parent = fixture.componentInstance.parent !;
+       parent.updateChild = true;
+       fixture.detectChanges(false);
+
+       readyHost = fixture.nativeElement.querySelector('.ready-host');
+       readyChild = fixture.nativeElement.querySelector('.ready-child');
+       expect(readyHost).toBeTruthy();
+       expect(readyChild).toBeTruthy();
+     });
 });
 
 function assertStyleCounters(countForSet: number, countForRemove: number) {

packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json

@@ -167,9 +167,6 @@
   {
     "name": "_selectedIndex"
   },
-  {
-    "name": "_state"
-  },
   {
     "name": "addComponentLogic"
   },
@@ -590,9 +587,6 @@
   {
     "name": "resetPreOrderHookFlags"
   },
-  {
-    "name": "resetStylingState"
-  },
   {
     "name": "resolveDirectives"
   },

packages/core/test/bundling/hello_world/bundle.golden_symbols.json

@@ -146,9 +146,6 @@
   {
     "name": "_selectedIndex"
   },
-  {
-    "name": "_state"
-  },
   {
     "name": "addToViewTree"
   },
@@ -434,9 +431,6 @@
   {
     "name": "resetPreOrderHookFlags"
   },
-  {
-    "name": "resetStylingState"
-  },
   {
     "name": "selectIndexInternal"
   },

packages/core/test/bundling/todo/bundle.golden_symbols.json

@@ -467,9 +467,6 @@
   {
     "name": "applyProjectionRecursive"
   },
-  {
-    "name": "applyStyling"
-  },
   {
     "name": "applyStylingValue"
   },
@@ -914,9 +911,6 @@
   {
     "name": "hasConfig"
   },
-  {
-    "name": "hasDirectives"
-  },
   {
     "name": "hasParentInjector"
   },
@@ -945,7 +939,7 @@
     "name": "initNodeFlags"
   },
   {
-    "name": "initializeTNodeInputs"
+    "name": "initializeInputAndOutputAliases"
   },
   {
     "name": "injectElementRef"
@@ -1106,9 +1100,6 @@
   {
     "name": "markDirtyIfOnPush"
   },
-  {
-    "name": "markStylingStateAsDirty"
-  },
   {
     "name": "markViewDirty"
   },
@@ -1355,6 +1346,9 @@
   {
     "name": "stringifyForError"
   },
+  {
+    "name": "stylingApply"
+  },
   {
     "name": "stylingMapToString"
   },