microsoft / microsoft/microsoft-ui-reactor
[Bug] Transform modifiers (Scale/Rotation/Translation/CenterPoint) have no unset arm — and ClearValue alone can't undo an animated write
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
Split out of #986. That issue added the missing `ClearValue` unset arms for the five plain-DP modifiers (`IsTabStop`, `TabIndex`, `ElementSoundMode`, `XYFocusKeyboardNavigation`, `HeadingLevel`) and audited `ApplyAccessibilityModifiers`. The transform family was deliberately left out because it is not a plain DP write.
## Symptom
```csharp
phase == 0
? Button("Go", Go).Scale(1.5f)
: Button("Go", Go) // still scaled at 1.5
```
Same shape for `.Rotation(...)`, `.Translation(...)`, `.CenterPoint(...)`.
## Why `ClearValue` alone is not the fix
`UIElement.Scale` / `Rotation` / `Translation` / `CenterPoint` are XAML *facade* properties: they have real DP identifiers, but the live value lives on the element's composition visual. `Reconciler.ApplyModifiers` writes them through `AnimationHelper.SetOrAnimate` / `SetOrAnimateVector3`, which branch on the ambient `AnimationScope` / the element's `.Animate()` curve:
* **no curve** — `SetScalarDirect` / `SetVector3Direct` assign the XAML property, so a local DP value exists and `ClearValue` would release it normally.
* **curve present** — `ElementCompositionPreview.GetElementVisual(element).StartAnimation(target, anim)`. A running composition animation outranks the DP, and the XAML property was never assigned, so `ClearValue` has no local value to release and plausibly raises no change notification at all. The visual stays where the animation left it.
So the reset has to undo whichever path ran, and the two paths are not distinguishable from the modifier bag alone.
## Proposed shape
Add a helper next to the existing set path, reusing `AnimationHelper.CompositorPropertyName` so the target mapping stays in one place:
```csharp
internal static void StopCompositionAnimation(UIElement element, string property)
{
try
{
var visual = ElementCompositionPreview.GetElementVisual(element);
visual.StopAnimation(CompositorPropertyName(property));
}
catch (COMException ex) when (Diagnostics.HResults.IsTeardownReentry(ex.HResult)) { }
}
```
then the arm:
```csharp
else if (!m.Scale.HasValue && oldM?.Scale.HasValue == true)
{
AnimationHelper.StopCompositionAnimation(fe, "Scale");
fe.ClearValue(UIElement.ScaleProperty);
}
```
Keeping the literal `fe.ClearValue(...)` in the arm body matters: `ModifierUnsetClearValueTests` scans the syntax tree for it, and an arm that only calls the helper would read as inert.
**Fallback if that is not enough.** `StopAnimation` leaves the visual at the last animated value, and `ClearValue` may not push the default back when no local value was ever set. If the selftest shows that, the sequence becomes "write the facade default, then `ClearValue`" — the assignment forces the visual to re-sync, the clear then releases the local value so styles can still win. That is the one place an assignment is defensible under the #952 rule, and it needs a comment saying so or the structural test will read it as a regression.
## Verification
Headless xUnit cannot construct a WinUI object, so this needs a selftest, not a unit test. `ModifierEventFixtures.ModifierClearResets` is the template.
The fixture has to cover **both** paths — the un-animated case and one with an ambient curve — because a `ClearValue`-only fix passes the first and fails the second, which is exactly the trap this issue exists to avoid. Assert `ReadLocalValue(dp) == DependencyProperty.UnsetValue` *and* that the live value is back at the facade default (`Scale == Vector3.One`, `Rotation == 0f`, `Translation == Vector3.Zero`, `CenterPoint == Vector3.Zero`) — the local-value read alone cannot see a compositor animation still driving the visual, which is the whole failure mode here.
Mutation-check it: comment out the `StopAnimation` call and confirm the animated-case check flips to `not ok`. If it doesn't, the fixture isn't actually reaching the animation path.
## Also in scope
`Opacity`'s unset arm has the identical gap, acknowledged in a comment already in the source:
```csharp
// Note: if the set arm started a compositor animation on Visual.Opacity,
// clearing the XAML DP does not cancel it — neither did the previous
// `fe.Opacity = 1.0`, so this is not a regression.
fe.ClearValue(UIElement.OpacityProperty);
```
Once the helper exists, `Opacity` should use it too and that comment should go.
## First commit
Delete the four `Scale` / `Rotation` / `Translation` / `CenterPoint` entries from `ModifierUnsetClearValueTests.MissingUnsetArmExceptions` — the test then fails until the arms exist, which is the point.
## Not in scope
`ElementPool.CleanElement` does not clear these DPs either, so a *recycled* control can still carry a transform into an unrelated renter. That is the same class of leak as #965 and belongs there: adding a pool reset forces a `poolReset: true` row in `ModifierTable.Properties`, which escalates `.Set(c => c.Scale = ...)` to `REACTOR_POOL_001`.
Contributor guide
Research direction
Start in Reconciler.ApplyModifiers and AnimationHelper, especially the existing SetOrAnimate paths and CompositorPropertyName mapping. Use ModifierUnsetClearValueTests and ModifierEventFixtures.ModifierClearResets to cover animated and unanimated Scale, Rotation, Translation, CenterPoint, and Opacity resets. Done means unset arms exist, composition animations are stopped, local values are unset, and live values return to their facade defaults.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- desktop, frontend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 58/100