Skip to content

Commit 4846a8a

Browse files
fix(reconcile-highlight): replace gradient brush + animations with timer-driven solid sprites (#167) (#171)
* fix(reconcile-highlight): replace gradient brush + animations with timer-driven solid sprites (#167) The overlay's diagonal-stripe LinearGradientBrush (transparent gradient stops, ExtendMode=Wrap, rotated TransformMatrix) at sub-1.0 sprite opacity triggers a Composition render-cache bug: nearby content (chart slice paths in #167's repro) ends up at ~50% blend with white until the next layout pass. Empirical bisect (see C:\temp\visualizer-bug\log.md) confirmed the gradient brush itself is the trigger; switching to a plain CompositionColorBrush clears the manual-typing repro. Overlay rewrite: - Replace ScalarKeyFrameAnimation + CompositionScopedBatch lifecycle with direct Opacity assignment + per-target DispatcherQueueTimer. - Per-target dedup via Dictionary<UIElement, ActiveHighlight>: repeat hits within the hold window refresh the existing sprite (size, offset, brush, timer reset) instead of stacking duplicates. - Solid CompositionColorBrush at 0.33 opacity; color (red mounted / yellow modified) carries the differentiation that the stripe pattern previously did. - Class doc cross-references issue #167 to deter reintroducing the gradient brush. Tests (tests/Reactor.AppTests.Host): - 12 new lifecycle fixtures pin dedup, refresh, expiry, geometry update, brush swap, dispose cleanup, budget cap, and a regression test asserting no Composition animations are attached to sprites. - 2 new integration fixtures cover rapid-click bursts staying bounded and clearing within the hold window. - Existing 8 ReconcileHighlight + 12 TemplatedListHL fixtures still green. Also adds internal test accessors (LiveSpriteCount, ActiveTargetCount, TestActiveSprites, TestForceExpire, TestHoldDurationOverrideMs) and exposes OverlayWiring/HighlightOverlay/DebugForceHighlightFlush so fixtures can drive the overlay deterministically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review: address Copilot CR — drop unused usings, replace local path with issue link - Remove unused `Microsoft.UI.Reactor.Core` and `Microsoft.UI.Xaml.Hosting` usings from ReconcileHighlightOverlay.cs (no references after the refactor). - Replace `C:\temp\visualizer-bug\log.md` references in the overlay class doc and the lifecycle fixtures doc with a link to issue #167, since the local path isn't available to other contributors or CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 9d411b4 commit 4846a8a

6 files changed

Lines changed: 659 additions & 138 deletions

File tree

src/Reactor/Hosting/OverlayHostWiring.cs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,22 @@ public OverlayHostWiring(DispatcherQueue dispatcherQueue)
8181
public Grid? WrapperRoot => _wrapperRoot;
8282
public Canvas? OverlayCanvas => _overlayCanvas;
8383

84+
/// <summary>Test-only: live reconcile-highlight overlay (null if not yet created).</summary>
85+
internal ReconcileHighlightOverlay? HighlightOverlay => _highlightOverlay;
86+
87+
/// <summary>
88+
/// Test-only: bypass the highlight cooldown and dispatch a flush
89+
/// synchronously. Returns false if no overlay context exists yet.
90+
/// </summary>
91+
internal bool DebugForceHighlightFlush()
92+
{
93+
if (_overlayCanvas is null) return false;
94+
_highlightCooldown.Reset();
95+
_highlightFlushPending = false;
96+
FlushHighlight();
97+
return true;
98+
}
99+
84100
/// <summary>
85101
/// Install <paramref name="newControl"/> into the shared wrapper Grid.
86102
/// The wrapper + Canvas are created on first call and reused forever

src/Reactor/Hosting/ReactorHost.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,9 @@ public sealed class ReactorHost : IDisposable
7171
// flips on.
7272
private OverlayHostWiring? _overlayWiring;
7373

74+
/// <summary>Test-only: access the live overlay-host wiring (null if not constructed).</summary>
75+
internal OverlayHostWiring? OverlayWiring => _overlayWiring;
76+
7477
// ── Layout cost overlay data pipeline (gated by ReactorFeatureFlags.ShowLayoutCost) ──
7578
// Owned by the host so the ETW session lifetime matches the host's lifetime.
7679
// Constructed lazily on first observed flag-on; never torn down except on Dispose
Lines changed: 150 additions & 138 deletions
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,73 @@
11
using System.Numerics;
22
using Microsoft.UI.Composition;
3-
using Microsoft.UI.Reactor.Core;
3+
using Microsoft.UI.Dispatching;
44
using Microsoft.UI.Xaml;
55
using Microsoft.UI.Xaml.Controls;
6-
using Microsoft.UI.Xaml.Hosting;
76

87
namespace Microsoft.UI.Reactor.Hosting;
98

109
/// <summary>
11-
/// Draws diagonal-striped overlay rectangles over UIElements that were mounted (red, 45°)
12-
/// or modified (yellow, 135°) during a reconcile pass. Uses the Composition visual layer
13-
/// to avoid creating XAML elements (which would themselves show up as reconcile churn).
14-
/// Each overlay fades out over <see cref="FadeDurationMs"/> milliseconds.
15-
/// Designed for best-effort display under high update cadence — caps sprites and
16-
/// uses a single scoped batch per flush to avoid swamping the compositor.
10+
/// Draws solid-color overlay rectangles over UIElements that were mounted (red)
11+
/// or modified (yellow) during a reconcile pass. Uses the Composition visual
12+
/// layer to avoid creating XAML elements (which would themselves show up as
13+
/// reconcile churn). One sprite per live target — repeat hits within
14+
/// <see cref="DefaultHoldDurationMs"/> refresh the existing sprite (geometry +
15+
/// brush) and restart its expiry timer instead of stacking duplicates. Sprites
16+
/// snap on/off via direct Opacity assignment + a one-shot
17+
/// <see cref="DispatcherQueueTimer"/>; no Composition animation is involved.
1718
/// </summary>
19+
/// <remarks>
20+
/// Issue #167 — sub-1.0 opacity sprites with a tiled <c>LinearGradientBrush</c>
21+
/// (transparent gradient stops, <c>ExtendMode=Wrap</c>, rotated
22+
/// <c>TransformMatrix</c>) cause a persistent ~50%-blend-with-white wash on
23+
/// nearby content (e.g. chart slice paths) until the next layout pass. The
24+
/// previous design used such a brush to paint the diagonal stripe pattern.
25+
/// Empirical bisect confirmed the gradient brush itself is the trigger:
26+
/// solid <c>CompositionColorBrush</c> at any opacity (1.0 or 0.33) clears
27+
/// the bug; the gradient brush at 0.33 reproduces it. This file uses solid
28+
/// color brushes only. Distinguishability stays via color (red vs yellow).
29+
/// See https://github.com/microsoft/microsoft-ui-reactor/issues/167 for
30+
/// the full repro investigation and bisect data.
31+
/// </remarks>
1832
internal sealed class ReconcileHighlightOverlay : IDisposable
1933
{
20-
private const float MountedOpacity = 0.22f;
21-
private const float ModifiedOpacity = 0.22f;
22-
private const int FadeDurationMs = 600;
23-
private const float StripeWidth = 5f;
34+
private const float MountedOpacity = 0.33f;
35+
private const float ModifiedOpacity = 0.33f;
36+
private const int DefaultHoldDurationMs = 600;
2437

25-
/// <summary>Max sprites to add per flush call (excess elements are dropped).</summary>
38+
/// <summary>
39+
/// Test-only override for the hold duration (ms). Set before constructing
40+
/// an overlay to make lifecycle tests run against a tighter window. Null
41+
/// (default) means use <see cref="DefaultHoldDurationMs"/>.
42+
/// </summary>
43+
internal static int? TestHoldDurationOverrideMs { get; set; }
44+
45+
/// <summary>Max NEW sprites to add per flush call (refreshes don't count).</summary>
2646
private const int MaxSpritesPerFlush = 200;
2747

28-
/// <summary>Max live sprites in the container — skip adding more if exceeded.</summary>
48+
/// <summary>Max live sprites in the container — skip adding new ones if exceeded.</summary>
2949
private const int MaxLiveSprites = 500;
3050

3151
private static readonly global::Windows.UI.Color MountedColor =
32-
global::Windows.UI.Color.FromArgb(255, 220, 40, 40); // red at 45°
52+
global::Windows.UI.Color.FromArgb(255, 220, 40, 40); // red — mounted
3353
private static readonly global::Windows.UI.Color ModifiedColor =
34-
global::Windows.UI.Color.FromArgb(255, 240, 200, 20); // yellow at 135°
54+
global::Windows.UI.Color.FromArgb(255, 240, 200, 20); // yellow — modified
3555

3656
private readonly Canvas _overlayCanvas;
3757
private readonly ContainerVisual _parentContainer;
3858
private readonly Compositor _compositor;
59+
private readonly DispatcherQueue _dispatcherQueue;
3960
private readonly ContainerVisual _container;
61+
private readonly Dictionary<UIElement, ActiveHighlight> _active = new();
62+
private readonly int _holdDurationMs;
4063
private CompositionBrush? _mountedBrush;
4164
private CompositionBrush? _modifiedBrush;
42-
private ScalarKeyFrameAnimation? _fadeMountedAnim;
43-
private ScalarKeyFrameAnimation? _fadeModifiedAnim;
65+
66+
private sealed class ActiveHighlight
67+
{
68+
public SpriteVisual Sprite = default!;
69+
public DispatcherQueueTimer Timer = default!;
70+
}
4471

4572
/// <summary>
4673
/// Ctor takes both the Canvas (for hit-testing / size queries) and the
@@ -55,8 +82,12 @@ public ReconcileHighlightOverlay(Canvas overlayCanvas, ContainerVisual parentCon
5582
_overlayCanvas = overlayCanvas;
5683
_parentContainer = parentContainer;
5784
_compositor = parentContainer.Compositor;
85+
_dispatcherQueue = DispatcherQueue.GetForCurrentThread()
86+
?? throw new InvalidOperationException(
87+
"ReconcileHighlightOverlay must be constructed on a UI thread with a DispatcherQueue.");
5888
_container = _compositor.CreateContainerVisual();
5989
_parentContainer.Children.InsertAtTop(_container);
90+
_holdDurationMs = TestHoldDurationOverrideMs ?? DefaultHoldDurationMs;
6091
}
6192

6293
/// <summary>
@@ -69,158 +100,139 @@ public void Show(
69100
IReadOnlyList<UIElement> mounted,
70101
IReadOnlyList<UIElement> modified)
71102
{
72-
// Back-pressure: if too many sprites are already animating, skip this flush entirely
73-
if (_container.Children.Count >= MaxLiveSprites) return;
103+
_mountedBrush ??= _compositor.CreateColorBrush(MountedColor);
104+
_modifiedBrush ??= _compositor.CreateColorBrush(ModifiedColor);
74105

75-
_mountedBrush ??= CreateStripeBrush(MountedColor, 45f);
76-
_modifiedBrush ??= CreateStripeBrush(ModifiedColor, 135f);
77-
_fadeMountedAnim ??= CreateFadeAnimation(MountedOpacity);
78-
_fadeModifiedAnim ??= CreateFadeAnimation(ModifiedOpacity);
106+
int newBudget = MaxSpritesPerFlush;
79107

80-
int budget = MaxSpritesPerFlush;
108+
for (int i = 0; i < mounted.Count; i++)
109+
RefreshOrAdd(host, mounted[i], _mountedBrush, MountedOpacity, ref newBudget);
81110

82-
// Single scoped batch for ALL sprites in this flush — avoids per-sprite batch overhead.
83-
// CompositionScopedBatch is IDisposable; we dispose it from the Completed handler
84-
// (or from the catch path below) so long debugging sessions don't leak COM/resource
85-
// pressure one batch at a time.
86-
var batch = _compositor.CreateScopedBatch(CompositionBatchTypes.Animation);
87-
bool disposeInCompleted = false;
88-
var thisFlushSprites = new List<SpriteVisual>(Math.Min(mounted.Count + modified.Count, MaxSpritesPerFlush));
89-
90-
try
91-
{
92-
for (int i = 0; i < mounted.Count && budget > 0; i++)
93-
{
94-
if (TryAddHighlight(host, mounted[i], _mountedBrush, MountedOpacity, _fadeMountedAnim, thisFlushSprites))
95-
budget--;
96-
}
97-
98-
for (int i = 0; i < modified.Count && budget > 0; i++)
99-
{
100-
if (TryAddHighlight(host, modified[i], _modifiedBrush, ModifiedOpacity, _fadeModifiedAnim, thisFlushSprites))
101-
budget--;
102-
}
103-
104-
// When the batch's animations complete, remove exactly the sprites
105-
// we added in this flush. We can't use sprite.Opacity to decide —
106-
// CompositionAnimation drives the rendered value but does not write
107-
// back to the static property, so the getter still reads the last
108-
// value we assigned (0.17 / 0.22) even after the fade completes.
109-
// Tracking the per-flush list directly avoids that pitfall and
110-
// prevents sprites from leaking permanently into the container.
111-
var container = _container;
112-
batch.Completed += (_, _) =>
113-
{
114-
try
115-
{
116-
foreach (var sprite in thisFlushSprites)
117-
{
118-
try { container.Children.Remove(sprite); } catch { }
119-
try { sprite.Dispose(); } catch { }
120-
}
121-
}
122-
finally
123-
{
124-
batch.Dispose();
125-
}
126-
};
127-
128-
disposeInCompleted = true;
129-
batch.End();
130-
}
131-
catch
132-
{
133-
if (!disposeInCompleted) batch.Dispose();
134-
throw;
135-
}
111+
for (int i = 0; i < modified.Count; i++)
112+
RefreshOrAdd(host, modified[i], _modifiedBrush, ModifiedOpacity, ref newBudget);
136113
}
137114

138-
private bool TryAddHighlight(UIElement host, UIElement target, CompositionBrush brush,
139-
float opacity, ScalarKeyFrameAnimation fadeAnim, List<SpriteVisual> flushSprites)
115+
private void RefreshOrAdd(UIElement host, UIElement target, CompositionBrush brush,
116+
float opacity, ref int newBudget)
140117
{
141-
if (target is not FrameworkElement fe) return false;
142-
if (fe.ActualWidth <= 0 || fe.ActualHeight <= 0) return false;
118+
if (target is not FrameworkElement fe) return;
119+
if (fe.ActualWidth <= 0 || fe.ActualHeight <= 0) return;
143120

121+
Vector2 size;
122+
Vector3 offset;
144123
try
145124
{
146125
var transform = target.TransformToVisual(host);
147-
var position = transform.TransformPoint(default);
148-
149-
var sprite = _compositor.CreateSpriteVisual();
150-
sprite.Size = new Vector2((float)fe.ActualWidth, (float)fe.ActualHeight);
151-
sprite.Offset = new Vector3((float)position.X, (float)position.Y, 0);
152-
sprite.Opacity = opacity;
153-
sprite.Brush = brush;
154-
155-
_container.Children.InsertAtTop(sprite);
156-
flushSprites.Add(sprite);
157-
sprite.StartAnimation("Opacity", fadeAnim);
158-
return true;
126+
var pos = transform.TransformPoint(default);
127+
size = new Vector2((float)fe.ActualWidth, (float)fe.ActualHeight);
128+
offset = new Vector3((float)pos.X, (float)pos.Y, 0);
159129
}
160130
catch (ArgumentException)
161131
{
162132
// TransformToVisual throws if target is in a different visual tree (popup/flyout)
163-
return false;
133+
return;
134+
}
135+
136+
// Refresh path: same target already has a live sprite — update geometry/brush
137+
// and restart the expiry timer. No new SpriteVisual, no stacking.
138+
if (_active.TryGetValue(target, out var existing))
139+
{
140+
existing.Sprite.Brush = brush;
141+
existing.Sprite.Opacity = opacity;
142+
existing.Sprite.Size = size;
143+
existing.Sprite.Offset = offset;
144+
try { existing.Timer.Stop(); } catch { }
145+
try { existing.Timer.Start(); } catch { }
146+
return;
164147
}
148+
149+
// New sprite path — gated by per-flush and global caps.
150+
if (newBudget <= 0) return;
151+
if (_container.Children.Count >= MaxLiveSprites) return;
152+
153+
var sprite = _compositor.CreateSpriteVisual();
154+
sprite.Size = size;
155+
sprite.Offset = offset;
156+
sprite.Opacity = opacity;
157+
sprite.Brush = brush;
158+
_container.Children.InsertAtTop(sprite);
159+
160+
var timer = _dispatcherQueue.CreateTimer();
161+
timer.Interval = TimeSpan.FromMilliseconds(_holdDurationMs);
162+
timer.IsRepeating = false;
163+
164+
var capturedTarget = target;
165+
var container = _container;
166+
timer.Tick += (s, _) =>
167+
{
168+
try
169+
{
170+
if (_active.TryGetValue(capturedTarget, out var ah))
171+
{
172+
try { container.Children.Remove(ah.Sprite); } catch { }
173+
try { ah.Sprite.Dispose(); } catch { }
174+
_active.Remove(capturedTarget);
175+
}
176+
}
177+
finally
178+
{
179+
try { ((DispatcherQueueTimer)s).Stop(); } catch { }
180+
}
181+
};
182+
183+
_active[target] = new ActiveHighlight { Sprite = sprite, Timer = timer };
184+
timer.Start();
185+
newBudget--;
165186
}
166187

167-
private ScalarKeyFrameAnimation CreateFadeAnimation(float fromOpacity)
188+
// ─────────────────────────────────────────────────────────────────────
189+
// Test-only accessors (gated by InternalsVisibleTo on Reactor.AppTests.Host)
190+
// ─────────────────────────────────────────────────────────────────────
191+
192+
/// <summary>Test-only: live sprite count in the overlay container.</summary>
193+
internal int LiveSpriteCount => _container.Children.Count;
194+
195+
/// <summary>Test-only: number of distinct targets currently tracked.</summary>
196+
internal int ActiveTargetCount => _active.Count;
197+
198+
/// <summary>Test-only: snapshot of the current sprites for inspection.</summary>
199+
internal IReadOnlyList<SpriteVisual> TestActiveSprites()
168200
{
169-
var anim = _compositor.CreateScalarKeyFrameAnimation();
170-
anim.InsertKeyFrame(0f, fromOpacity);
171-
anim.InsertKeyFrame(1f, 0f);
172-
anim.Duration = TimeSpan.FromMilliseconds(FadeDurationMs);
173-
return anim;
201+
var list = new List<SpriteVisual>(_active.Count);
202+
foreach (var ah in _active.Values) list.Add(ah.Sprite);
203+
return list;
174204
}
175205

176206
/// <summary>
177-
/// Creates a repeating diagonal-stripe brush. The gradient tiles with
178-
/// <see cref="CompositionGradientExtendMode.Wrap"/> and is rotated to
179-
/// the requested angle (e.g. 45° or 135°).
207+
/// Test-only: synchronously fire all pending expiry timers, removing
208+
/// every active sprite. Use to make lifecycle tests deterministic without
209+
/// waiting on real wall-clock time.
180210
/// </summary>
181-
private CompositionBrush CreateStripeBrush(global::Windows.UI.Color color, float angleDegrees)
211+
internal void TestForceExpire()
182212
{
183-
var brush = _compositor.CreateLinearGradientBrush();
184-
brush.MappingMode = CompositionMappingMode.Absolute;
185-
brush.ExtendMode = CompositionGradientExtendMode.Wrap;
186-
187-
// Vertical gradient over one period (stripe + gap), then rotate
188-
float period = StripeWidth * 2f;
189-
brush.StartPoint = new Vector2(0, 0);
190-
brush.EndPoint = new Vector2(0, period);
191-
192-
var transparent = global::Windows.UI.Color.FromArgb(0, color.R, color.G, color.B);
193-
brush.ColorStops.Add(_compositor.CreateColorGradientStop(0f, color));
194-
brush.ColorStops.Add(_compositor.CreateColorGradientStop(0.5f, color));
195-
brush.ColorStops.Add(_compositor.CreateColorGradientStop(0.5f, transparent));
196-
brush.ColorStops.Add(_compositor.CreateColorGradientStop(1f, transparent));
197-
198-
float radians = angleDegrees * MathF.PI / 180f;
199-
brush.TransformMatrix = Matrix3x2.CreateRotation(radians);
200-
201-
return brush;
213+
var snapshot = new List<KeyValuePair<UIElement, ActiveHighlight>>(_active);
214+
foreach (var kv in snapshot)
215+
{
216+
try { kv.Value.Timer.Stop(); } catch { }
217+
try { _container.Children.Remove(kv.Value.Sprite); } catch { }
218+
try { kv.Value.Sprite.Dispose(); } catch { }
219+
_active.Remove(kv.Key);
220+
}
202221
}
203222

204223
public void Dispose()
205224
{
206-
// Dispose any in-flight sprite children before tearing down the
207-
// container itself. They hold animations + brushes that the
208-
// compositor would otherwise leak across long debug sessions.
209-
try
225+
foreach (var ah in _active.Values)
210226
{
211-
for (int i = _container.Children.Count - 1; i >= 0; i--)
212-
{
213-
var child = _container.Children.ElementAt(i);
214-
try { _container.Children.Remove(child); } catch { }
215-
try { child.Dispose(); } catch { }
216-
}
227+
try { ah.Timer.Stop(); } catch { }
228+
try { _container.Children.Remove(ah.Sprite); } catch { }
229+
try { ah.Sprite.Dispose(); } catch { }
217230
}
218-
catch { }
231+
_active.Clear();
232+
219233
try { _parentContainer.Children.Remove(_container); } catch { }
220234
try { _container.Dispose(); } catch { }
221235
try { _mountedBrush?.Dispose(); } catch { }
222236
try { _modifiedBrush?.Dispose(); } catch { }
223-
try { _fadeMountedAnim?.Dispose(); } catch { }
224-
try { _fadeModifiedAnim?.Dispose(); } catch { }
225237
}
226238
}

0 commit comments

Comments
 (0)