From 00c1167a8a03966a5682e8a6b5e472b986da792c Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Fri, 17 Nov 2023 13:34:45 -0800
Subject: [PATCH] fix(core): Dirty views that are reattached should get
 refreshed during CD

Another fix related to #52928. When a view has the `Dirty` flag and is
reattached, we should ensure that it is reached and refreshed during
the next change detection run from above.

This commit does not actually fix the reproduction reported in #52928. However,
that's arguably not something we should try to address because there's
no clear indication that a `CheckAlways` view needs to be refreshed
during change detection (aside from it being in creation mode/first
update pass like the reported issue).
---
 packages/core/src/render3/util/view_utils.ts  |  6 ++++
 .../test/acceptance/change_detection_spec.ts  | 36 +++++++++++--------
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts
index 59f2656e7bae51..a7cb65b4279b89 100644
--- a/packages/core/src/render3/util/view_utils.ts
+++ b/packages/core/src/render3/util/view_utils.ts
@@ -206,6 +206,12 @@ export function requiresRefreshOrTraversal(lView: LView) {
  * parents above.
  */
 export function updateAncestorTraversalFlagsOnAttach(lView: LView) {
+  // When we attach a view that's marked `Dirty`, we should ensure that it is reached during the
+  // next CD traversal so we add the `RefreshView` flag and mark ancestors accordingly.
+  if (lView[FLAGS] & LViewFlags.Dirty) {
+    lView[FLAGS] |= LViewFlags.RefreshView;
+  }
+
   if (!requiresRefreshOrTraversal(lView)) {
     return;
   }
diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts
index 77efadee3007c4..5262b9fb5321b5 100644
--- a/packages/core/test/acceptance/change_detection_spec.ts
+++ b/packages/core/test/acceptance/change_detection_spec.ts
@@ -187,14 +187,13 @@ describe('change detection', () => {
       @Component({
         selector: `test-cmpt`,
         template: `{{counter}}|<ng-template #vc></ng-template>`,
-        changeDetection: ChangeDetectionStrategy.OnPush
+        changeDetection: ChangeDetectionStrategy.OnPush,
+        standalone: true,
       })
       class TestCmpt {
         counter = 0;
         @ViewChild('vc', {read: ViewContainerRef}) vcRef!: ViewContainerRef;
 
-        constructor() {}
-
         createComponentView<T>(cmptType: Type<T>): ComponentRef<T> {
           return this.vcRef.createComponent(cmptType);
         }
@@ -202,18 +201,14 @@ describe('change detection', () => {
 
       @Component({
         selector: 'dynamic-cmpt',
-        template: `dynamic`,
+        template: `dynamic|{{binding}}`,
+        standalone: true,
         changeDetection: ChangeDetectionStrategy.OnPush
       })
       class DynamicCmpt {
+        @Input() binding = 'binding';
       }
 
-      @NgModule({declarations: [DynamicCmpt]})
-      class DynamicModule {
-      }
-
-      TestBed.configureTestingModule({imports: [DynamicModule], declarations: [TestCmpt]});
-
       const fixture = TestBed.createComponent(TestCmpt);
 
       // initial CD to have query results
@@ -221,20 +216,33 @@ describe('change detection', () => {
       fixture.detectChanges(false);
       expect(fixture.nativeElement).toHaveText('0|');
 
-      // insert a dynamic component
+      // insert a dynamic component, but do not specifically mark parent dirty
+      // (dynamic components with OnPush flag are created with the `Dirty` flag)
       const dynamicCmptRef = fixture.componentInstance.createComponentView(DynamicCmpt);
       fixture.detectChanges(false);
-      expect(fixture.nativeElement).toHaveText('0|dynamic');
+      expect(fixture.nativeElement).toHaveText('0|dynamic|binding');
 
       // update model in the OnPush component - should not update UI
       fixture.componentInstance.counter = 1;
       fixture.detectChanges(false);
-      expect(fixture.nativeElement).toHaveText('0|dynamic');
+      expect(fixture.nativeElement).toHaveText('0|dynamic|binding');
 
       // now mark the dynamically inserted component as dirty
       dynamicCmptRef.changeDetectorRef.markForCheck();
       fixture.detectChanges(false);
-      expect(fixture.nativeElement).toHaveText('1|dynamic');
+      expect(fixture.nativeElement).toHaveText('1|dynamic|binding');
+
+      // Update, mark for check, and detach before change detection, should not update
+      dynamicCmptRef.setInput('binding', 'updatedBinding');
+      dynamicCmptRef.changeDetectorRef.markForCheck();
+      dynamicCmptRef.changeDetectorRef.detach();
+      fixture.detectChanges(false);
+      expect(fixture.nativeElement).toHaveText('1|dynamic|binding');
+
+      // reattaching and run CD from the top should update
+      dynamicCmptRef.changeDetectorRef.reattach();
+      fixture.detectChanges(false);
+      expect(fixture.nativeElement).toHaveText('1|dynamic|updatedBinding');
     });
 
     it('should support re-enterant change detection', () => {