fix(core): Reattached views that are dirty from a signal update should refresh (#53001)

atscott · AndrewKushnir · commit b35c6731e51d · 2023-11-20T12:06:32.000-08:00
Related to #52928 but `updateAncestorTraversalFlagsOnAttach` is called on view insertion and _should_ have made that work for views dirty from signals but it wasn't updated to read the `dirty` flag when we changed it from sharing the `RefreshView` flag. For #52928, we've traditionally worked under the assumption that this is working as expected. The created view is `CheckAlways`. There is a question of whether we should automatically mark things for check when the attached view has the `Dirty` flag and/or has the `FirstLViewPass` flag set (or other flags that indicate it definitely needs to be prefreshed). PR Close #53001
diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts
@@ -18,7 +18,7 @@ import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView,
 import {getOrBorrowReactiveLViewConsumer, maybeReturnReactiveLViewConsumer, ReactiveLViewConsumer} from '../reactive_lview_consumer';
 import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state';
 import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils';
-import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
+import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, requiresRefreshOrTraversal, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
 
 import {executeTemplate, executeViewQueryFn, handleError, processHostBindingOpCodes, refreshContentQueries} from './shared';
 
@@ -72,8 +72,7 @@ function detectChangesInViewWhileDirty(lView: LView) {
   // descendants views that need to be refreshed due to re-dirtying during the change detection
   // run, detect changes on the view again. We run change detection in `Targeted` mode to only
   // refresh views with the `RefreshView` flag.
-  while (lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh) ||
-         lView[REACTIVE_TEMPLATE_CONSUMER]?.dirty) {
+  while (requiresRefreshOrTraversal(lView)) {
     if (retries === MAXIMUM_REFRESH_RERUNS) {
       throw new RuntimeError(
           RuntimeErrorCode.INFINITE_CHANGE_DETECTION,
diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts
@@ -13,7 +13,7 @@ import {LContainer, LContainerFlags, TYPE} from '../interfaces/container';
 import {TConstants, TNode} from '../interfaces/node';
 import {RNode} from '../interfaces/renderer_dom';
 import {isLContainer, isLView} from '../interfaces/type_checks';
-import {DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view';
+import {DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, REACTIVE_TEMPLATE_CONSUMER, TData, TView} from '../interfaces/view';
 
 
 
@@ -195,21 +195,22 @@ export function walkUpViews(nestingLevel: number, currentView: LView): LView {
   return currentView;
 }
 
+export function requiresRefreshOrTraversal(lView: LView) {
+  return lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh) ||
+      lView[REACTIVE_TEMPLATE_CONSUMER]?.dirty;
+}
+
 
 /**
- * Updates the `DESCENDANT_VIEWS_TO_REFRESH` counter on the parents of the `LView` as well as the
- * parents above that whose
- *  1. counter goes from 0 to 1, indicating that there is a new child that has a view to refresh
- *  or
- *  2. counter goes from 1 to 0, indicating there are no more descendant views to refresh
- * When attaching/re-attaching an `LView` to the change detection tree, we need to ensure that the
- * views above it are traversed during change detection if this one is marked for refresh or has
- * some child or descendant that needs to be refreshed.
+ * Updates the `HasChildViewsToRefresh` flag on the parents of the `LView` as well as the
+ * parents above.
  */
 export function updateAncestorTraversalFlagsOnAttach(lView: LView) {
-  if (lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh)) {
-    markAncestorsForTraversal(lView);
+  if (!requiresRefreshOrTraversal(lView)) {
+    return;
   }
+
+  markAncestorsForTraversal(lView);
 }
 
 /**
diff --git a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts
@@ -793,7 +793,9 @@ describe('OnPush components with signals', () => {
 
     @Component({
       selector: 'on-push-parent',
-      template: `<signal-component></signal-component>{{incrementChecks()}}`,
+      template: `
+      <signal-component></signal-component>
+      {{incrementChecks()}}`,
       changeDetection: ChangeDetectionStrategy.OnPush,
       standalone: true,
       imports: [SignalComponent],
@@ -827,6 +829,18 @@ describe('OnPush components with signals', () => {
       expect(trim(fixture.nativeElement.textContent)).toEqual('initial');
     });
 
+    it('refreshes when reattached if already dirty', () => {
+      const fixture = TestBed.createComponent(OnPushParent);
+      fixture.detectChanges();
+      fixture.componentInstance.signalChild.value.set('new');
+      fixture.componentInstance.signalChild.cdr.detach();
+      fixture.detectChanges();
+      expect(trim(fixture.nativeElement.textContent)).toEqual('initial');
+      fixture.componentInstance.signalChild.cdr.reattach();
+      fixture.detectChanges();
+      expect(trim(fixture.nativeElement.textContent)).toEqual('new');
+    });
+
     // Note: Design decision for signals because that's how the hooks work today
     // We have considered actually running a component's `afterViewChecked` hook if it's refreshed
     // in targeted mode (meaning the parent did not refresh) and could change this decision.
diff --git a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json
@@ -1328,6 +1328,9 @@
   {
     "name": "replacePostStylesAsPre"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json
@@ -1400,6 +1400,9 @@
   {
     "name": "replacePostStylesAsPre"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json
@@ -1112,6 +1112,9 @@
   {
     "name": "renderView"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json
@@ -2249,6 +2249,9 @@
   {
     "name": "renderView"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
@@ -1553,6 +1553,9 @@
   {
     "name": "renderView"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json
@@ -1523,6 +1523,9 @@
   {
     "name": "requiredValidator"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json
@@ -881,6 +881,9 @@
   {
     "name": "renderView"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json
@@ -1220,6 +1220,9 @@
   {
     "name": "renderView"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json
@@ -1880,6 +1880,9 @@
   {
     "name": "replaceSegment"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json
@@ -977,6 +977,9 @@
   {
     "name": "renderView"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },
diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -1334,6 +1334,9 @@
   {
     "name": "renderView"
   },
+  {
+    "name": "requiresRefreshOrTraversal"
+  },
   {
     "name": "resetPreOrderHookFlags"
   },

Original file line number	Diff line number	Diff line change
`@@ -1328,6 +1328,9 @@`
`1328`	`1328`	`{`
`1329`	`1329`	`"name": "replacePostStylesAsPre"`
`1330`	`1330`	`},`
	`1331`	`+ {`
	`1332`	`+ "name": "requiresRefreshOrTraversal"`
	`1333`	`+ },`
`1331`	`1334`	`{`
`1332`	`1335`	`"name": "resetPreOrderHookFlags"`
`1333`	`1336`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1400,6 +1400,9 @@`
`1400`	`1400`	`{`
`1401`	`1401`	`"name": "replacePostStylesAsPre"`
`1402`	`1402`	`},`
	`1403`	`+ {`
	`1404`	`+ "name": "requiresRefreshOrTraversal"`
	`1405`	`+ },`
`1403`	`1406`	`{`
`1404`	`1407`	`"name": "resetPreOrderHookFlags"`
`1405`	`1408`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1112,6 +1112,9 @@`
`1112`	`1112`	`{`
`1113`	`1113`	`"name": "renderView"`
`1114`	`1114`	`},`
	`1115`	`+ {`
	`1116`	`+ "name": "requiresRefreshOrTraversal"`
	`1117`	`+ },`
`1115`	`1118`	`{`
`1116`	`1119`	`"name": "resetPreOrderHookFlags"`
`1117`	`1120`	`},`
Original file line number	Diff line number	Diff line change
`@@ -2249,6 +2249,9 @@`
`2249`	`2249`	`{`
`2250`	`2250`	`"name": "renderView"`
`2251`	`2251`	`},`
	`2252`	`+ {`
	`2253`	`+ "name": "requiresRefreshOrTraversal"`
	`2254`	`+ },`
`2252`	`2255`	`{`
`2253`	`2256`	`"name": "resetPreOrderHookFlags"`
`2254`	`2257`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1553,6 +1553,9 @@`
`1553`	`1553`	`{`
`1554`	`1554`	`"name": "renderView"`
`1555`	`1555`	`},`
	`1556`	`+ {`
	`1557`	`+ "name": "requiresRefreshOrTraversal"`
	`1558`	`+ },`
`1556`	`1559`	`{`
`1557`	`1560`	`"name": "resetPreOrderHookFlags"`
`1558`	`1561`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1523,6 +1523,9 @@`
`1523`	`1523`	`{`
`1524`	`1524`	`"name": "requiredValidator"`
`1525`	`1525`	`},`
	`1526`	`+ {`
	`1527`	`+ "name": "requiresRefreshOrTraversal"`
	`1528`	`+ },`
`1526`	`1529`	`{`
`1527`	`1530`	`"name": "resetPreOrderHookFlags"`
`1528`	`1531`	`},`
Original file line number	Diff line number	Diff line change
`@@ -881,6 +881,9 @@`
`881`	`881`	`{`
`882`	`882`	`"name": "renderView"`
`883`	`883`	`},`
	`884`	`+ {`
	`885`	`+ "name": "requiresRefreshOrTraversal"`
	`886`	`+ },`
`884`	`887`	`{`
`885`	`888`	`"name": "resetPreOrderHookFlags"`
`886`	`889`	`},`