microsoft / microsoft/microsoft-ui-reactor
Padding/Margin reconciler + pool gaps found while fixing #950 (dead Margin reset, stale inline edges, Control pool leak)
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
Spun out of #950. Everything below is **pre-existing** — measured on `main` before and after that PR — and deliberately left out of its scope. Three related defects, all in the same neighbourhood.
Measurements were taken with throwaway selftest fixtures that encode the observed value into the TAP check *name*, run against the current tree and again after `git checkout HEAD~1 -- src/Reactor/Core/Reconciler.cs src/Reactor/Core/ElementPool.cs`.
---
## 1. The `Margin` reset arm in `ApplyModifiers` is unreachable dead code
`src/Reactor/Core/Reconciler.cs` (~`:3670` / `:3680`) has the same shape #950 repaired for `Padding`:
```csharp
var resolvedMargin = m.Margin ?? oldM?.Margin;
if (resolvedMargin.HasValue && resolvedMargin != oldM?.Margin) { /* set */ }
else if (!resolvedMargin.HasValue && oldM?.Margin.HasValue == true) { /* unreachable */ }
```
`!resolvedMargin.HasValue` requires `oldM?.Margin` to be null; the second conjunct requires it non-null. The two can never hold together, so **the reset branch never runs**: dropping `.Margin(...)` from a re-render leaves the old margin on the control forever.
`Padding` was fixed in #950 by splitting the guard into `wantsPadding` / `hadPadding` locals (the `?? oldM?...` fallback has to stay, because it seeds `basePad` for the BiDi inline overlay). `Margin` needs the same treatment — it has no inline-overlay consumer, so it may be even simpler.
`Margin` also differs from `Padding` in that it *is* reset by `ElementPool.CleanElement` (`poolReset: true` in `ModifierTable`), so the pool masks the leak on recycled controls but not on a live update of the same instance.
## 2. Partially removing an inline padding leaves a stale edge
Two cases, both byte-identical before and after #950 (i.e. not caused by it):
| Transition (on a `Button`) | Observed final `Padding` | Expected |
|---|---|---|
| `.Padding(10).PaddingInlineStart(20)` → `.Padding(10)` | `20,10,10,10` | `10,10,10,10` |
| `.PaddingInlineStart(8)` → `.PaddingInlineEnd(4)` | `8,0,4,0` | `0,0,4,0` |
Two causes compound:
- the set arm compares the newly resolved padding against the raw `oldM.Padding` slot rather than against the padding it previously *resolved*, so a change that only touches `PaddingInlineStart`/`End` can compare equal and skip the write;
- `basePad` reads the control's **live** padding, so a stale edge from the previous render is re-adopted as the base for the next overlay instead of being recomputed from the element.
A real fix needs the previously-resolved value tracked per element (or `basePad` derived from `oldM` instead of the control), which is a design change rather than a guard tweak — hence a separate issue.
## 3. `Control` / `Border` / `StackPanel` padding leaks across pool reuse
`ElementPool.CleanElement` resets `Margin`, `Width`, `Height`, `MinWidth/Height`, `MaxWidth/Height`, alignment, opacity and visibility, but **never** `Padding` for these three types. #950 added a `TextBlock.PaddingProperty` clear inside the type-specific `switch (fe)`, which leaves the other three still leaking.
Measured on a `Button`:
```
Zz2_mount_30_30_30_30 # .Padding(30) applied
Zz2_sameInstance_True # the pooled control is genuinely reused
Zz2_afterRent_30_30_30_30 # rented by an element with NO padding modifier
Zz2_afterRent_localSet_True # ...and it is still a local value, not the theme default
```
So a padded `Button` returned to the pool hands its padding to the next unpadded `Button`.
**This is not a one-line fix**, which is why #950 left it alone. `PoolResetSetConsistencyTests.ReadCleanElementCommonBlock` scans `CleanElement` from its opening brace up to the first `switch (fe)` and requires exact agreement with the `PoolReset` flags in `ModifierTable`. Putting the clear in that common block therefore forces `Padding` to `poolReset: true`, which flips every `.Set(c => c.Padding = ...)` from `REACTOR_MOD_002` (Info) to **`REACTOR_POOL_001` (Warning)** across samples and user code — a re-sweep of `samples/.editorconfig` and a compat call, not a drive-by.
Options worth weighing:
- put the clears in the common block and accept the `poolReset: true` flip plus the samples sweep;
- put them in the type-specific `switch` alongside the `TextBlock` clear, keeping them invisible to the consistency scan — cheaper, but it makes the table describe the pool less accurately;
- teach the consistency scan about type-narrowed resets in the switch so the table can say `poolReset: true` truthfully without the analyzer severity change.
## 4. `.Set` writes lose to a modifier reset (documented, not a bug — noting it for discoverability)
Because `DescriptorHandler` runs `ApplySetters` **before** `ApplyModifiers`, a render that drops `.Padding(...)` and writes the property through `.Set(...)` instead ends with the property cleared: the setter lands first, then the reset arm wipes it.
Confirmed to be pre-existing, family-wide behaviour rather than anything #950 introduced:
```
Zz3_mount_120 # .Width(120)
Zz3_sameInstance_True
Zz3_afterSetOnly_NaN # -> .Set(b => b.Width = 55) is cleared, not honoured
```
`Width`, `Height`, `MinWidth` and `RequestedTheme` have always answered this way; #950 only brought `Padding` into line with them. `REACTOR_MOD_002` already steers callers toward the first-class modifier, and #950 pins the semantics in `Issue950_ModifierResetOutranksASetterWrite` (including the `Width` arm, so any future change has to decide for the whole family at once). Filed here only so the ordering rule is findable — closing this sub-item as "working as intended" is a fine outcome.
Contributor guide
Research direction
Start with src/Reactor/Core/Reconciler.cs and src/Reactor/Core/ElementPool.cs, then run the selftest fixtures described for Margin, inline padding, and pooled controls. Read PoolResetSetConsistencyTests.ReadCleanElementCommonBlock and Issue950_ModifierResetOutranksASetterWrite before changing reset semantics. Done means the reported transitions no longer leak state, pool behavior agrees with the modifier table, and any samples/.editorconfig compatibility impact is addressed; the setter-ordering item remains documented if unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- desktop
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100