MenuItem: CommandParameters are ignored

[SetTrend]

• .NET Core Version: 5
• Windows version: 20H2

Problem description:

I created an MVVM sample repository. In this repository I'm loading MenuItems and Buttons with commands.

While the Button elements call ICommand::Execute(object? parameter) with the given CommandParameter, MenuItem elements don't.

Here's a code snipped showing both declarations:

<MenuItem Header="Film_liste" ItemsSource="{Binding Path=Commands}">
    <MenuItem.ItemTemplate>
      <DataTemplate>
        <MenuItem Command="{Binding}" CommandParameter="{Binding ElementName=Filmliste, Path=SelectedItem}" Header="{Binding Path=Text}" />
      </DataTemplate>
    </MenuItem.ItemTemplate>
  </MenuItem>

  ...

  <ItemsControl.ItemTemplate>
    <DataTemplate>
      <Button Command="{Binding}" CommandParameter="{Binding ElementName=Filmliste, Path=SelectedItem}" Content="{Binding Path=Text}" Margin="5" Padding="10 3"></Button>
    </DataTemplate>
  </ItemsControl.ItemTemplate>

Actual behavior:

Commands triggered by MenuItem elements are always passed null as command parameter.

Expected behavior:

Commands triggered by MenuItem elements should be passed the value provided as command parameter.

Minimal repro:

MVVM Test repository

Steps to reproduce:

MenuItem.doesn.t.convey.CommandParameter.mp4

[miloush]

Your MenuItem.ItemTemplate is not doing what you think it is doing - you are creating a MenuItem inside a MenuItem. The binding for the inner one works as expected, however, there is no binding on the outer one so it gets disabled which applies to its children too.

(Edit: You can see what I mean if you change your MenuItem.ItemTemplate to be a Button. Then you will have buttons in the menu, but they will still be inside menu items. And they will stay disabled unlike the other buttons.)

Try this instead:

<MenuItem Header="Film_liste" ItemsSource="{Binding Path=Commands}">
    <MenuItem.ItemContainerStyle>
        <Style TargetType="MenuItem">
            <Setter Property="Command" Value="{Binding}" />
            <Setter Property="CommandParameter" Value="{Binding ElementName=Filmliste, Path=SelectedItem}" />
            <Setter Property="Header" Value="{Binding Path=Text}" />
        </Style>
    </MenuItem.ItemContainerStyle>
</MenuItem>

[SetTrend]

This is strange ...

According to the book "Pro WPF 4.0 in C#" from Apress and Microsoft docs this should work:

<!-- In MainWindow.xaml -->
<Menu>
    <MenuItem Header="_File">
        <MenuItem Header="_Exit" 
                    Command="{Binding Path=CloseCommand}" />
    </MenuItem>
    <MenuItem Header="_Edit" />
    <MenuItem Header="_Options" />
    <MenuItem Header="_Help" />
</Menu>

In both examples, the outer MenuItem doesn't have a binding, too.

[SetTrend]

I think it's slowly getting in my grasp ...

So you suggest, the structure created from my original code is like this:

<Menu DockPanel.Dock="Top">
  <MenuItem Header="Film_liste" ItemsSource="{Binding Path=Commands}">
    <MenuItem>
      <MenuItem Command="{Binding}" CommandParameter="{Binding ElementName=Filmliste, Path=SelectedItem}" Header="{Binding Path=Text}" />

... because a menu item's item container is a menu item itself?

Shouldn't this layout practically be presented in the Visual Live Tree? It doesn't seem to be like this:

After changing the menu layout according to your (correct) suggestion, I just noticed another glitch:

When any of the films get selected for the first time after program start, while the "Remove Film" Button (the middle one) gets enabled, the corresponding MenuItem isn't:

[miloush]

No, I was suggesting your code was putting MenuItem sort of inside as it was the Header of the container-MenuItem.

Your book example, on the other hand, is creating submenus, i.e. populating the Items.

[SetTrend]

Did you read my second comment?

[SetTrend]

I believe I have evidence that there's a bug in the way commands in MenuItems are refreshed.

MenuItems are calling CanExecute when the main menu is opened. However, the CommandParameter provided to CanExecute isn't reliably acquiring the data bound to them.

In my example project, I can reliably reproduce the erroneous behaviour:

When you ...

1. select an item from the ListBox,
2. then open the main menu for the first time,

... the CommandParameter binding is not getting performed. ERROR

When, however, you ...

1. open the main menu first,
2. then select an item from the ListBox,
3. and then open the main menu again,

... binding is getting performed correctly.

Here's the flow chart, reporting the two approaches:

Here's a screencast, reporting a debug session performing the successful path:

And here's a screencast, reporting a debug session performing the error path:

In case anyone needs to manually pause/rewind, here's the original screencast recording:

MenuItem.doesn.t.convey.CommandParameter.-.2.mp4

[SetTrend]

Because this thread has grown old an possibly out of sight, I'd like to invite @ryalanms to this bug report.

[miloush]

Then this seems to be a duplicate of #3452.

(changing command parameter does not actually re-evaluate CanExecute)

[SetTrend]

Are you sure? After reading the other issue I don't think so, because:

• in my case, CanExecute is getting called
• it's just the parameter that's provided to CanExecute that isn't evaluated/provided correctly

So, in my case the CommandParameter binding isn't reliably evaluated before CanExecute is getting called.

Plus, in contrast to the other case, in my case the issue seems to only exist for MenuItem components. The same code in the same WPF form works flawlessly for Button components. (See my minimum sample repository referenced above and my screencast.)

[SetTrend]

I suppose, someone with sufficient Member's/Contributor's expertise, equipped with a debug build from WPF Core should smoothly be able to find and fix the command parameter binding issue I reported by debugging into the WPF framework function that's calling CanExecute for the menu item.

I provided all that's necessary to reproduce the issue: A minimum MVVM WPF project and detailed steps for how to reproduce.

[miloush]

I'm sorry you are stuck with me. I am pretty sure, you can see yourself by setting the command parameter first:

Change

        <Style TargetType="MenuItem">
            <Setter Property="Command" Value="{Binding}" />
            <Setter Property="CommandParameter" Value="{Binding ElementName=Filmliste, Path=SelectedItem}" />
            <Setter Property="Header" Value="{Binding Path=Text}" />
        </Style>

to

        <Style TargetType="MenuItem">
            <Setter Property="CommandParameter" Value="{Binding ElementName=Filmliste, Path=SelectedItem}" />
            <Setter Property="Command" Value="{Binding}" />
            <Setter Property="Header" Value="{Binding Path=Text}" />
        </Style>

[SetTrend]

Sincerely pardon me, @miloush, no insulting intended when I wrote my previous comment. I'm sorry if it inadvertently caused irritation on your behalf.

After reading your comment, I found the following blogs about this issue:

• WPF: XAML Attribute Order Matters
• The Order Issue of XAML Attributes

I don't think, this exotic behaviour is documented. I can't find any documentation on this. On the contrary - the following is the current state of WPF attribute sequence documentation I'm aware of:

Matthew MacDonald, "Pro WPF 4.5 in C#", 4^th edition, Apress

Page 102, "The Coercion Callback"

This is an important consideration for initialization, such as when a window is being created for a XAML document. All WPF controls guarantee that their properties can be set in any order, without causing any change in behavior.
[...]
This behavior is due to WPF’s property resolution system, which you learned about earlier. Although WPF stores the exact local value you’ve set internally, it evaluates what the property should be (using coercion and a few other considerations) when you read the property.

Microsoft Docs, "Coerce Value Callbacks and Property Changed Events"

Using these two callbacks in combination, you can create a series of properties on elements where changes in one property will force a coercion or reevaluation of another property.

Microsoft Docs, "DependsOnAttribute Class"

Indicates that the attributed property is dependent on the value of another property.
[...]
The Save method on XamlWriter will process the specified property before processing the property that this attribute is set on.

However:

Microsoft Docs, "MenuItem.CommandParameter Property"

[System.ComponentModel.Bindable(true)]
[System.Windows.Localizability(System.Windows.LocalizationCategory.NeverLocalize)]
public object CommandParameter { get; set; }

---

If this behaviour is not documented ...

• will it cause issues with the Visual Studio WPF designer?
• will it cause issues with the Save method on XamlWriter?

I can test and implement your suggested workaround. Then I will have a local solution and have learned from life. - However, shouldn't this be fixed in WPF, once and for all, so programmers around the globe won't repeatedly stumble over this irritating and undocumented behaviour?

On the other hand: if there's a requirement preventing to fix this, shouldn't Visual Studio render a warning message if these attributes weren't given in correct sequence?

Why does it work with the Button element?

[miloush]

So what happens is that when you open the menu for the first time, the MenuItems get created - the constructor is called, and all the properties are set in the order that it is specified in XAML. That way the MenuItem.Command is bound and set to a value, which, due to its property changed callback, reevaluates the command's CanExecute status:

See the OnCommandChanged > OnCommandChanged > HookCommand > UpdateCanExecute sequence here:
https://referencesource.microsoft.com/#PresentationFramework/src/Framework/System/Windows/Controls/MenuItem.cs,407

Note that at this point, the CommandParameter has not been set (bound) yet, and therefore CanExecute receives null parameter.

Now the loader sets (bounds) the CommandParameter, but there is no property changed handler for that, so it does not re-evaluate it.

Now when you are done loading, both Command and CommandParameter are bound and will stay current thanks to the databinding (EDIT: unless you change the Command value, then above happens again). Once the item is loaded, any operations, including Save will therefore be fine.

All WPF controls guarantee that their properties can be set in any order, without causing any change in behavior.

I don't see such guarantee in the documentation, and there are other types where the order matters (e.g. whether you set first WindowStyle or WindowState determines whether taskbar will be covered by a maximized borderless window).

I do agree, however, that pretending that CommandParameter does not affect whether Command can be executed is a design overview. I believe that is covered by #3452. If changing the CommandParameter would invalidate the CanExecute, then it will be called twice during loading, once with null parameter and one with the set one. That I believe is the correct behaviour

[SetTrend]

Shouldn't this line in MenuItem.OpenMenu:

if ((owner != null) && ((owner is MenuItem) || (owner is MenuBase)))
  {
    // Parent must be MenuItem or MenuBase in order for menus to open.
    // Otherwise, odd behavior will occur.
    SetCurrentValueInternal(IsSubmenuOpenProperty, BooleanBoxes.TrueBox);
    return true; // The value was actually changed
  }

... be rather amended by subsequently iterating through the MenuItem's children to call their UpdateCanExecute method, similar to this:

if ((owner != null) && ((owner is MenuItem) || (owner is MenuBase)))
  {
    // Parent must be MenuItem or MenuBase in order for menus to open.
    // Otherwise, odd behavior will occur.
    SetCurrentValueInternal(IsSubmenuOpenProperty, BooleanBoxes.TrueBox);

    foreach (MenuItem mi in Children) mi.UpdateCanExecute();

    return true; // The value was actually changed
  }

Wouldn't that make sense?

[miloush]

#316 - this is the issue I was looking for

[SetTrend]

We've been writing in parallel. Did you read my last comment? I'll be reading your references now.

[miloush]

I don't think you should invalidate all CanExecute for each item when nothing changed, that might have non-trivial performance costs.

[SetTrend]

I believe you need to. Because only at the moment when a menu opens you get the current program state, particularly when the CommandParameter is a binding expression. That's something you cannot cache.

And, yes, this issue very much resembles #316. But that seems to have been fixed. I'm using the Command="..." CommandParameter="..." syntax in my application without flaw.

[miloush]

What scenario do you have in mind where calling CanExecute after CommandParameter is changed would not fix the issue but re-evaluating on menu opening would? I don't see why opening a menu should be a particularly important event to achieve correctness, since menus can be "always open" (for example the top level menu). We should aim for the current state being always reflected, not only at specific points in time.

[SetTrend]

Ah, OK, I see. Right ...

The issue comes in when CommandParameter being a Binding expression. In that case, CommandParameter never changes (the Binding object stays the same), but the actual value bound to CommandParameter changes.

So, is it possible to bind an event handler to call UpdateCanExecute when the actual value bound to CommandParameter changes (in a binding expression that'd be the source value that's bound)? Wouldn't this be beneficial?

[SetTrend]

Something similar to (I'm writing a coarse idea here):

[CommonDependencyProperty]
public static readonly DependencyProperty CommandParameterProperty = DependencyProperty.Register( "CommandParameter", typeof(object), typeof(...), new FrameworkPropertyMetadata((object) null),
  param => switch (param)
  {
    case Binding b:
      b.TargetUpdated +=new EventHandler<DataTransferEventArgs>(delegate (depObj obj, DataTransferEventArgs args) => UpdateCanExecute();)
  }
);

[miloush]

Yes, that's exactly what #3452 is asking for. Sorry I didn't make it clear, the property changed callback is called anytime the value is changed, which can come from binding source value change, style trigger change, direct value assignment etc., (i.e. it does not only mean "assigning a binding to it")

This is how it's done with the Command property:

https://referencesource.microsoft.com/#PresentationFramework/src/Framework/System/Windows/Controls/MenuItem.cs,389

You want the same with CommandParameter where the OnCommandParameterChanged would call UpdateCanExecute().

[SetTrend]

Ahh ... now I get it!

Perfect. So, I voted for that issue now.

Shall we close this issue here then? Or keep it open for documentation reasons?

You've been great, very helpful, thorough, thoughtful. Thanks so much for your patience and spending your time in enlightening me and following my concerns!

[miloush]

You're welcome, sorry for the delays in responding.

I think you can close this issue (as duplicate of #316), it will stay available for documentation purposes.

[SetTrend]

@miloush:

Just one more question:

I can see that issues #3452 and #316 don't reference each other. Wouldn't it be beneficial to mention one from the other so members of each thread get aware of the other thread?

[miloush]

Sure, go ahead!