microsoft / microsoft/microsoft-ui-reactor
[Bug] ElementPool.CleanElement never releases Control.HorizontalContentAlignment / VerticalContentAlignment
Nobody has claimed this yet.
- 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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