microsoft / microsoft/microsoft-ui-reactor

[Bug] ElementPool.CleanElement never releases Control.HorizontalContentAlignment / VerticalContentAlignment

Open
#1,013 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
C#
Stars
646
Forks
54
Avg merge
1d 3h
Merged PRs (30d)
84

Description

Split out of #985, which fixed the same class of bug for `Padding`, `CornerRadius`, `BorderThickness`, `BorderBrush`, `Background` and `IsEnabled` but deliberately scoped itself to the six properties the issue enumerated.

## The leak

`Reconciler.ApplyModifiers` writes both content-alignment properties onto any `Control` receiver:

```csharp
// src/Reactor/Core/Reconciler.cs:3775-3786
if (fe is WinUI.Control contentAlignmentControl)
{
// ... HorizontalContentAlignment / VerticalContentAlignment
}
```

`src/Reactor/Core/ElementPool.cs` has **zero** occurrences of `ContentAlignment`. So the same mechanism #985 describes applies unchanged:

- On mount `oldM is null`, so `ApplyModifiers` runs no unset arm.
- A pooled `Button` / `ScrollViewer` / any poolable `Control` therefore starts life carrying the **local** value its previous renter set.
- A local value outranks every `Style` setter in WinUI's dependency-property precedence order, so the new renter silently renders with the old control's content alignment and cannot be corrected by a style.

## Why it wasn't fixed in #985

1. #985 enumerates its scope explicitly and says such adjacent finds should be *"a separate, deliberate change that shouldn't ride along"*.
2. `HorizontalContentAlignment` is currently the last **ungated** `REACTOR_MOD_002` anchor in the analyzer test suite. Flipping it to `poolReset: true` escalates it to `REACTOR_POOL_001` and destroys the premise of two tests in `tests/Reactor.Tests/AnalyzerTests/ModifierAvailableAnalyzerTests.cs`:
- `CodeFix_Chains_Across_Both_Rule_Ids_In_One_Body` (~line 501) — pairs a POOL_001 subject with a MOD_002 subject precisely to prove the two-id code-fix path. Both sides become POOL_001.
- the MOD_002 expectation at ~line 294.

A comment at `ModifierAvailableAnalyzerTests.cs:486-489` documents this dependency. Whoever takes this needs to re-anchor those two tests on a property that stays MOD_002 (the font family/size/weight rows, `TextWrapping` and `TextTrimming` are all still MOD_002, so the id is not orphaned) **before** flipping the table rows.

## Suggested fix

Extend the `if (fe is Control resetControl)` arm that #985 added to `CleanElement`'s `FrameworkElement`-common region (it is deliberately placed **before** the `switch` so the pool ⇄ analyzer consistency invariants can see it):

```csharp
resetControl.ClearValue(Control.HorizontalContentAlignmentProperty);
resetControl.ClearValue(Control.VerticalContentAlignmentProperty);
```

Then, in the same change:

- flip both rows to `poolReset: true` in `src/Reactor.Analyzers/ModifierTable.cs`, leaving the `controlGate` untouched;
- add the two pins to `CleanElementRequiredClears` in `tests/Reactor.Tests/AnalyzerTests/ModifierUnsetClearValueTests.cs` so an outright deletion fails positively rather than merely un-tripping an invariant;
- re-anchor the two `ModifierAvailableAnalyzerTests` cases above;
- add a selftest fixture alongside `ModifierPoolClearValueControlPanel` in `tests/Reactor.AppTests.Host/SelfTest/Fixtures/ModifierEventFixtures.cs`, registered in **both** `AllFixtures` and the `Create()` switch.

## Non-vacuity notes for the fixture

Copying the constraints that made the #985 fixtures meaningful:

- Keep a `ReferenceEquals(first, second)` instance-reuse guard — without it every "cleared" assertion passes trivially on a freshly constructed control.
- Assert `ReadLocalValue(dp) == DependencyProperty.UnsetValue`, never "equals the default value". Assigning the default instead of clearing is exactly the #952 bug and would sail past a default-value oracle.
- Keep a phase-1 "returned to the pool" check. Without it the reconciler may update in place, and the cleared assertions would be exercising `ApplyModifiers`' unset arm rather than `CleanElement`.
- Carry the modifiers on a control whose descriptor does **not** write the property on every mount, or the oracle is vacuous. (`ScrollViewer` was used in #985 for exactly this reason — `ButtonElement`'s descriptor writes `IsEnabled` unconditionally.)

## Severity fallout

Same shape as #985: `.Set(c => c.HorizontalContentAlignment = …)` moves from `REACTOR_MOD_002` (Info) to `REACTOR_POOL_001` (Warning). The suggested fix and the shipped code fix are unchanged; consumers building with `TreatWarningsAsErrors` may need to convert those call sites. Worth a CHANGELOG note.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with CleanElement in src/Reactor/Core/ElementPool.cs and the content-alignment handling in src/Reactor/Core/Reconciler.cs. Update the referenced ModifierTable.cs rows and inspect the required-clear and analyzer tests before adjusting their anchors. Add coverage in ModifierEventFixtures.cs, register it in AllFixtures and Create(), and run the specified analyzer and self-tests to verify pooled controls clear both local values.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
desktop, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.