microsoft / microsoft/microsoft-ui-reactor

Padding/Margin reconciler + pool gaps found while fixing #950 (dead Margin reset, stale inline edges, Control pool leak)

Open
#965 6 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.