microsoft / microsoft/microsoft-ui-reactor
Flyout / MenuFlyout / CommandBarFlyout silently disable their target's own callbacks
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
## Symptom
Wrapping a control in `Flyout(...)`, `MenuFlyout(...)` or `CommandBarFlyout(...)` silently kills the **target's own** callbacks.
```csharp
MenuFlyout(Button("Go", () => count++), MenuItem("Item")) // OnClick never fires
CommandBarFlyout(CheckBox(false, _ => n++, label: "x"), primaryCommands: [...]) // OnIsCheckedChanged never fires
Button("Go", () => count++) // works
```
No exception, no diagnostic — the handler just never runs.
## Root cause
All three target-wrapping decorators retag the *target control* with the *decorator's* element:
- `OverlayLifecycle.MountFlyout` → `Reconciler.SetElementTag(targetFe, flyEl)`
- `OverlayLifecycle.MountMenuFlyout` → `Reconciler.SetElementTag(targetFe, mfEl)`
- `OverlayLifecycle.MountCommandBarFlyout` → `Reconciler.SetElementTag(targetFe, cbf)`
(plus the matching `SetElementTag(targetFe, n)` in each `UpdateXxx`.)
That tag is `ReactorState.Element` — the same pointer the target's own event trampolines resolve
through to find their current element. After the decorator overwrites it, the target's trampoline
looks up e.g. `GetElementTag(sender) as ButtonElement`, gets a `MenuFlyoutElement` instead, and
drops the event.
The retagging is deliberate — it's how `Update()` refreshes the flyout's `Opened`/`Closed`
delegates without freezing them to the mount-time element (see the comment above
`MountFlyout` in `OverlayLifecycle.cs`). The two uses of the slot just collide.
## Evidence
Selftest fixtures, a decorated target and a plain control side by side:
```
not ok Diag_MenuFlyoutTargetOnClick - assertion failed
ok Diag_PlainButtonOnClick
```
and for `CommandBarFlyout`:
```
not ok CbfCallbacks_ButtonOnClickFired - assertion failed
not ok CbfCallbacks_CheckBoxOnChangedFired - assertion failed
```
`MenuFlyout` reproduces it on code untouched by any in-flight fix, and the `CheckBox` case shows
it isn't specific to `Button.Flyout` — it's the shared tag.
## Suggested fix
Stop routing the decorator's callbacks through the target's `ReactorState.Element`. Options:
1. Keep the decorator element somewhere of its own (an attached DP on the flyout, or a
per-instance map keyed by the flyout) and leave the target's tag to the target.
2. Give `ReactorState` a separate `DecoratorElement` slot so the two don't alias.
Either way the `Opened`/`Closed` handlers should still resolve the *current* decorator element at
invocation time so `Update()` keeps working.
## Notes
Found while fixing "`CommandBarFlyout` never opens from its target". That fix is deliberately
scoped to the flyout wiring and does **not** address this — it is pre-existing and affects all
three decorators equally.
Contributor guide
Research direction
Start in OverlayLifecycle.cs at MountFlyout, MountMenuFlyout, MountCommandBarFlyout and their matching Update methods, then inspect how ReactorState.Element is used by target event trampolines. Run the Diag_MenuFlyoutTargetOnClick and CbfCallbacks_ButtonOnClickFired/CbfCallbacks_CheckBoxOnChangedFired selftests. Done means decorator Opened/Closed updates still work while the wrapped target's callbacks fire.
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
- Mostly clear
- Newbie friendliness
- 48/100