KeyTrigger fires multiple times in a TabControl

[gix]

Describe the bug KeyTrigger fires multiple times in a TabControl.

To Reproduce Steps to reproduce the behavior:

1. Create a TabControl with multiple tabs.
2. Add an element (e.g. TextBox) to the second TabItem.
3. Add a KeyTrigger to the TextBox.
4. After switching to the second tab, invoking the key trigger fires its actions multiple times.

Expected behavior KeyTrigger fires only once.

Desktop (please complete the following information):

• Version with bug: 1.1.39

Reproduction Link WpfKeyTrigger.zip

[zmrbak]

KeyTrigger fires as many times as tabs count.

[brianlagunas]

The reason this is happening is because the KeyTrigger hooks into the control's Loaded event. In the loaded event it will then add an event handler to either the KeyUp or KeyDown event. The problem with this is that when the control is in the context of a TabControl, the loaded event is fired each time the TabItem containing the control is selected. So the command will invoke the total number of times the TabItem has been selected.

Not only that, but because of the multiple Loaded events, the Key events actually get added to the MainWindow because the logic will get the root object. So not only does the event get fired multiple times, but it also gets added to the MainWindow object so anytime you press the key no matter which tab you are on, it will invoke the command.

Not sure how to fix this just yet, but at least we know the issue.

[brianlagunas]

Essentially, the Loaded event is never unregistered from the control. I am wondering if it would be safe to simply unhook the loaded event after the KeyUp or KeyDown events have been hooked.

        protected override void OnEvent(EventArgs eventArgs)
        {
            // Listen to keyboard events.
            if (this.ActiveOnFocus)
            {
                this.targetElement = this.Source;
            }
            else
            {
                this.targetElement = KeyTrigger.GetRoot(this.Source);
            }

            if (this.FiredOn == KeyTriggerFiredOn.KeyDown)
            {
                this.targetElement.KeyDown += this.OnKeyPress;
            }
            else
            {
                this.targetElement.KeyUp += this.OnKeyPress;
            }

            // Unregister the Loaded event to prevent the KeyUp or KeyDown events from being registered multiple times.
            UnregisterEvent(targetElement, "Loaded");
        }

I'm trying to think about what the side effects of this would be. Could this break other scenarios I'm not thinking about?

[mgoertz-msft]

The problem with this is that when the control is in the context of a TabControl, the loaded event is fired each time the TabItem containing the control is selected.

Are you saying that you get more than one Loaded event for the same instance of a control? I would say that is not expected. I can't think of a reason the "Loaded" event needs to stay registered after the key events have been hooked up either. One slight change I would suggest is:

...
UnregisterEvent(this.targetElement, this.GetEventName());

[brianlagunas]

@mgoertz-msft Correct. Actually I found another method we have internally specifically for the unloaded event. I am creating a PR now that uses this line

UnregisterLoaded(targetElement as FrameworkElement);

What's taking a long time is that I just realized that the KeyTrigger doesn't have any unit tests so I am adding a couple.

I should have a PR ready in a couple of hours

[mgoertz-msft]

@brianlagunas Looks like there's some prior art in DataTrigger too.