microsoft / microsoft/microsoft-ui-reactor

Flyout / MenuFlyout / CommandBarFlyout silently disable their target's own callbacks

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

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.