CanExecuteChanged causes exceptions because ContextMenu passes wrong parameters

[Bernhard König]

I have a ContextMenu on a JumpList with multiple context menu entries.

A RelayCommand (from the MVVM Light Toolkit) is bound to their Command property and this command also has CanExecute logic attached.

Within the CanExecute handling of the MVVM Light toolkit I started to get InvalidCastExceptions which I could not explain. So I downloaded the MVVM Light source code and looked what happens.

It looks like sometimes the context menu passes in some random other controls like Border or TextBox as the CanExecute parameter instead of the RadDataBoundListBoxItem.

I only could reproduce this behavior when CanExecute returns false, meaning the ContextMenu item is disabled at the moment. But also in this case, it only appears if I open the context menu for a 2nd time while items are disabled. On the 1st time, it works and all items are shown disabled as expected. If CanExecute returns true I can open the ContextMenu as often as I want and this exception does not occur.

I attached a screenshot showing you how the CanExecute method gets called when this exception appears. Notice that the CanExecute type is based on the RadDataBoundListBoxItem (as this is what I have defined in the RelayCommand and what I would expect to always get as a parameter from the ContextMenu binding) whereas the passed parameter is of type System.Windows.Controls.Border ...

Please have a look at this. I can again send you my project in a support ticket if this helps you finding the issue, please let me know.

StackTrace:

at GalaSoft.MvvmLight.Command.RelayCommand`1.CanExecute(Object parameter)
at Telerik.Windows.Controls.RadContextMenuItem.GetActualCommandParameter()
at Telerik.Windows.Controls.RadContextMenuItem.OnLoaded(Object sender, RoutedEventArgs e)
at MS.Internal.CoreInvokeHandler.InvokeEventHandler(Int32 typeIndex, Delegate handlerDelegate, Object sender, Object args)
at MS.Internal.JoltHelper.FireEvent(IntPtr unmanagedObj, IntPtr unmanagedObjArgs, Int32 argsTypeIndex, Int32 actualArgsTypeIndex, String eventName)

Edit:
I can workaround this issue if I use just object instead of RadDataBoundListBoxItem for the generic RelayCommand declaration like this:

// Instead of this
public RelayCommand<RadDataBoundListBoxItem> MoveFeedContextCommand { get; private set; }

// Use that
public RelayCommand<object> MoveFeedContextCommand { get; private set; }

Then I can work around the exception within the RelayCommand but I have to do some casting and type checking myself which I want to avoid, so it would be great if this could be fixed.

[Victor]

Hello Bernhard,

Thanks for writing.
I managed to reproduce the exception. Casting from object should be the correct approach. The context menu item tests its command several times by passing potential parameters to CanExecute so that it can later invoke the command with the correct parameter. Please have a look at this help article, more specifically the CommandParameter section.

In order to avoid duplicated code you can inherit from RelayCommand and see if you can override a method in which you can centralize the logic that does the type cast.

Thank you for understanding.

All the best,
Victor
the Telerik team

[Bernhard König]

Hi Victor!

Thanks your your reply and your explainations!

While it's perfectly ok for me to handle this scenario by casting to object, I don't fully understand this behavior.

The documentation you referred to stated "If CommandParameter is not set the focused element of the owner menu will be passed to CanExecute of the current command."

So I do not set CommandParameter manually, thus "the focused element of the owner menu" should be passed. As I use my ContextMenu on top of a JumpList, this focused element should always be, IMHO, a DataBoundListBoxItem ...

This also always works fine and as expected when the actual command gets executed (= when the user taps on an ContextMenuItem). It normally also works fine with the CanExecute call, only not if items were disabled at some point (at lest this is my observation).

So this passing in of a Border or a TextBlock seems to conflict with the above statement in the documentation, it seems that for some reason the ContextMenu finds the wrong control in the XAML hierarchy, some child of the DataBoundListBoxItem apparently.
Thank for any clarification :)

Bernhard

[Victor]

Hello Bernhard,

Thanks for writing again.
The key sentence in the documentation is the one after you mentioned. It says this:
"If the focused element is also null the original source of the open manipulation of RadContextMenu is passed to the CanExecute method of the command."

This explanation is actually slightly misleadingр we'll change that. The difference is that even when the focused element is not null, but CanExecute returns false for the focused element for some reason, CanExecute is called again with the original manipulation source (border in your case). Maybe when the focused element is not null, the original source should not be considered at all. If the context menu is changed in this way you will not have the current issue. However this may result in a breaking change for some user out there... though it seems unlikely.
Here's the piece of code that we're considering right now:

object parameter = this.ReadLocalValue(RadContextMenuItem.CommandParameterProperty);
if (parameter != DependencyProperty.UnsetValue)
{
    // After the check for UnsetValue we check if our CLR property is not null because
    // parameter may be a BindingExpression and if it is, we want to use GetValue instead of
    // ReadLocalValue.
    return this.CommandParameter;
}

parameter = this.Owner.GetFocusedElement();
if (command.CanExecute(parameter))
{
    return parameter;
}

parameter = this.Owner.GetCurrentManipulationSource();
if (command.CanExecute(parameter))
{
    return parameter;
}

The change that we are considering is this:

parameter = this.Owner.GetFocusedElement();
if (command.CanExecute(parameter) || parameter != null)
{
    return parameter;
}

parameter = this.Owner.GetCurrentManipulationSource();
if (command.CanExecute(parameter))
{
    return parameter;
}

If the focused element is not null, we don consider the manipulation source at all and still use the focused element.

What do you think?

The reason for passing the manipulation source is that it is not a good idea to call Execute or CanExecute with a null parameter from inside the context menu. It should pass something so that the command can have some intelligent logic if there is no focused element or command parameter. It has more information to work with when the element under the menu is passed than when null is passed.

I am looking forward to your reply.
Greetings,
Victor
the Telerik team

[Bernhard König]

Hi Victor!

Thanks, now it makes perfect sense. I ignored the sentence after the one I mentioned because the focused element could not be NULL in my scenario. If also a returned value of false from CanExecute triggers this, I of course have to take that into account (and this also explains why this problem only appeared when elements were disabled).

Your suggested change seems perfectly right to me ... if you pass some state to the CanExecute method and it returns false, this can be considered as a valid decision by that method and there should not be a need to call it again with some other state object.

If you can include this change in a future release, I'd appreciate that very much. It's not urgent as I can work around it for now, but I can make my code cleaner again in the future. I'm a big fan of strongly typed code and I hate to have those objects and castings in it :) - and it would be generally more clear what's going on.

Thanks again,
Bernhard

[Victor]

Hi Bernhard,

Thanks for writing again.
I agree that if the control can retain its correct behavior casting should be eliminated when possible.
We'll include the change in the next internal build.

Thanks for the great feedback. :)
Your Telerik points have been updated.

Regards,
Victor
the Telerik team