-
Notifications
You must be signed in to change notification settings - Fork 37
fix(reconciler): re-render context consumers behind reference-stable child skips (#811) #812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9714135
b2ddde9
c5558a7
f7f2485
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,5 +48,7 @@ internal T Read<T>(Context<T> context) | |
| return context.DefaultValueBoxed; | ||
| } | ||
|
|
||
| internal bool HasActiveValues => _stack.Count > 0; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There's already a |
||
|
|
||
| internal long Version => _version; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1314,6 +1314,8 @@ internal IDisposable PushContextDisposable<T>(Context<T> context, T value) | |
| // without exposing the traversal stack itself. | ||
| internal T ReadContext<T>(Context<T> context) => _contextScope.Read(context); | ||
|
|
||
| internal bool HasActiveContextValues => _contextScope.HasActiveValues; | ||
|
|
||
| /// <summary> | ||
| /// Spec 047 §14 Phase 1 (1.6) — push a stagger scope for child enter | ||
| /// transitions. Returns an <see cref="IDisposable"/> that pops the | ||
|
|
@@ -1908,6 +1910,27 @@ private bool HasConsumedContextChanged(ComponentNode node) | |
| return false; | ||
| } | ||
|
|
||
| internal bool HasConsumedContextChangedInSubtree(UIElement control) | ||
| { | ||
| if (_componentNodes.TryGetValue(control, out var node) | ||
| && HasConsumedContextChanged(node)) | ||
| return true; | ||
|
|
||
| bool found = false; | ||
| ForEachReactorChildControl(control, child => | ||
|
Comment on lines
+1913
to
+1920
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This walk is only as complete as Better to drive traversal off the descriptor child metadata / |
||
| { | ||
| if (HasConsumedContextChangedInSubtree(child)) | ||
| { | ||
| found = true; | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| }); | ||
|
|
||
| return found; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Calls ShouldUpdate(oldProps, newProps) on a Component<TProps> via interface dispatch. | ||
| /// </summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| using Microsoft.UI.Reactor; | ||
| using Microsoft.UI.Reactor.Core; | ||
| using Microsoft.UI.Reactor.AppTests.Host.SelfTest; | ||
| using static Microsoft.UI.Reactor.Factories; | ||
|
|
||
| namespace Microsoft.UI.Reactor.AppTests.Host.SelfTest.Fixtures; | ||
|
|
||
| /// <summary> | ||
| /// Issue #811 — a reference-stable child subtree that consumes context must still | ||
| /// re-render when the provided context value changes. | ||
| /// | ||
| /// The reproducer is intentionally minimal: a parent owns internal interactive | ||
| /// state, provides it via <c>.Provide(...)</c>, and re-emits the SAME overlay | ||
| /// element instance on every render. The overlay consumes that context to derive | ||
| /// both visible text and a click action. If child-skip short-circuits before the | ||
| /// reconciler descends into the consumer, the label stays stale and the click path | ||
| /// keeps dispatching the old action shape. | ||
| /// </summary> | ||
| internal static class Issue811ContextConsumerSkipFixtures | ||
| { | ||
| private static readonly Context<bool> InteractiveCtx = new(true); | ||
|
|
||
| private sealed class Probe | ||
| { | ||
| public int RenderCount; | ||
| public string? LastAction; | ||
| } | ||
|
|
||
| private sealed record OverlayProps(Probe Probe); | ||
|
|
||
| private sealed class OverlayConsumer : Component<OverlayProps> | ||
| { | ||
| public override Element Render() | ||
| { | ||
| Props.Probe.RenderCount++; | ||
| bool interactive = UseContext(InteractiveCtx); | ||
|
|
||
| return VStack( | ||
| TextBlock(interactive ? "Lock interactivity" : "Unlock interactivity"), | ||
| Button("Invoke overlay action", () => | ||
| Props.Probe.LastAction = interactive ? "lock" : "unlock")); | ||
| } | ||
| } | ||
|
|
||
| internal sealed class ReferenceStableChildSkip_ContextConsumerRerenders(Harness h) | ||
| : SelfTestFixtureBase(h) | ||
| { | ||
| public override async Task RunAsync() | ||
| { | ||
| var probe = new Probe(); | ||
| var stableOverlay = Component<OverlayConsumer, OverlayProps>(new OverlayProps(probe)); | ||
|
|
||
| var host = H.CreateHost(); | ||
| host.Mount(ctx => | ||
| { | ||
| var (interactive, setInteractive) = ctx.UseState(true); | ||
|
|
||
| return VStack( | ||
| TextBlock(interactive ? "surface:on" : "surface:off"), | ||
| Button("Toggle interactive", () => setInteractive(!interactive)), | ||
| stableOverlay) | ||
| .Provide(InteractiveCtx, interactive); | ||
|
Comment on lines
+61
to
+62
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Solid repro for the positional/unkeyed case. To cover the rest of the fix, worth adding three variants:
|
||
| }); | ||
|
|
||
| await Harness.Render(); | ||
|
|
||
| H.Check("Issue811_Mount_SurfaceOn", H.FindText("surface:on") is not null); | ||
| H.Check("Issue811_Mount_LabelLock", H.FindText("Lock interactivity") is not null); | ||
| H.Check("Issue811_Mount_OverlayRenderedOnce", probe.RenderCount == 1); | ||
|
|
||
| H.ClickButton("Toggle interactive"); | ||
| await Harness.Render(); | ||
|
|
||
| H.Check("Issue811_Toggle_SurfaceOff", H.FindText("surface:off") is not null); | ||
| H.Check("Issue811_Toggle_LabelUpdated", H.FindText("Unlock interactivity") is not null); | ||
| H.Check("Issue811_Toggle_OverlayRerendered", probe.RenderCount >= 2); | ||
|
|
||
| H.ClickButton("Invoke overlay action"); | ||
| await Harness.Render(); | ||
|
|
||
| H.Check("Issue811_ActionUsesCurrentContext", probe.LastAction == "unlock"); | ||
| H.Check("Issue811_ActionDidNotUseStaleContext", probe.LastAction != "lock"); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This context-aware skip decline only lives here in the positional
UpdateCommonChildarm, so the fix is incomplete for keyed lists. The other skip sites in this file don't have it:continueonCanSkipUpdatewith noHasConsumedContextChangedInSubtreecheck, so a keyed stable child (grid rows, keyed list items) that consumes context still goes stale on a provider change — the exact Bug: pure context changes can be lost behind reference-stable child skip paths #811 bug via the keyed path;ChildDiffHintsfast-path (~L111-143) skips untouched indices wholesale and only callsUpdateCommonChildfor changed indices, so it bypasses the check too.The rule now exists in three shapes (this
goto perform_update, the keyedcontinue, and the&&chain in Reconciler.Update.cs). I'd pull it into a singleShouldDeclineSkip(oldEl, newEl, control)helper used by all four sites — that closes the keyed/hint gaps in one place and lets this arm drop the one-offgoto. Please add a keyed selftest variant to lock it in.