WPF Bug with a Workaround: Double Invocation of Button.Click

• Source code: steps to reproduce the problem, ToReproduce.zip
• Source code: the workaround, Workaround.zip

Table of Contents

1. Introduction
2. The Problem
3. Steps to Reproduce
4. The Workaround
5. Testing and Compatibility
6. Some Conclusions. Are we Fine?
7. Credits

1. Introduction

It is really wonderful that I spotted this bug this late. It is also really wonderful that I failed to find the description of this problem on the Web so far. It is quite likely that such description exists somewhere, because the problem is quite apparent, so I'll be much grateful if someone points one out.

It seems even more wonderful considering the practical importance of the situation when a bug is manifested. Indeed, it is really important to handle all the exceptions when there is a last chance to catch them: at the very top stack frame of the stack of each thread, and also at the outermost event cycle of the UI; in WPF, this is done by handling all the unhandled exceptions using the event Application.DispatcherUnhandledException. In this case, every unhandled exception will be caught inside the event cycle, handled, and then the cycle can be continued, if it makes sense under the given exceptional condition.

It is also important to use access characters on controls, which allows the user to activate or focus each control with one keyboard key combination (Alt+Key). Actually all well-designed applications are expected to allow really quick access to all controls using just the keyboard, so access characters are extremely helpful.

Unfortunately, the combination of these most basic techniques creates a problem with WPF, when the Button control is used and the exception is actually thrown and caught.

As a matter of fact, all programming communities have some members who tend to give "this is a feature, not a bug" arguments, insisting that "if you do everything correctly, the system will behave correctly". Specially for such people, I want to emphasize that the control's Click event is designed so they are supposed to show exact same behavior, regardless of the way they are activated, via the mouse or the keyboard, no matter how the application uses the event. This is not the case with WPF, under certain conditions I will describe in the next section.

Anyway, the topic of this article is discussible. The workaround I suggest helps to guarantee desired behavior in case of exception in every particular application, but it hardly can be done in a totally universal way, so the bug can sneak everywhere, due to its very nature. That's why I decided to write on this matter in the form of the article, which allows me to show the exact code to reproduce the problem. Also, I think it is really important to bring the problem to the attention of as many WPF developers as possible.

I will be very grateful if some readers point out better approach workarounds for this problem. All kinds of correct criticism will be very welcome.

2. The Problem

It's good to start with the notes showing the cases when the problem does not appear.

Notably, I found the problem only with WPF buttons, nothing else. I could not possibly try out all available controls, but my test of other most widely used controls did not show any abnormalities; I tried check boxes and radio buttons (which are the closest relatives of the class Button) and also menu items.

The problem only appears when the button is "clicked" using the access key present in its text. Such text is defined in XAML by the underscore ('_') character; if it prefixes some character of the button's text, this character serves as the access key which invokes the Click event when the user hits Ctrl+Key keyboard key combination. If the Click event is triggered with the mouse or keyboard in any other way, nothing wrong happens.

The problem appears only if some exception is thrown in the Click event handler, and only if it is handled using the event Application.DispatcherUnhandledException. In this case, the Click event is handled, exception is thrown and handled, and then the Click event is invoked again, and it leads to the second-time handling of the exception.

For further details, please see the exact steps used to reproduce the problem shown in the next section.

3. Steps to Reproduce

The easiest way to reproduce the problem would be loading the project referenced by the first code download link at the top of this article.

I'll also show the shortest way to reproduce it from scratch, starting from the Visual Studio template "WPF Application":

1. With Visual Studio, create such application from scratch.
2. Add some button to the main window XAML and give it a name. The button text should have an underscore character, to denote some access key, so the Click event would be invoked by Alt+Key click on the keyboard.
3. Add constructor to the Application class; in this constructor, add an event handler to the invocation list of the event System.Windows.Application.DispatcherUnhandledException (using the '+=' operator). This event handler should assign the value of the System.Windows.Threading.DispatcherUnhandledExceptionEventArgs.Cancel to true and show some exception information on screen.
4. Add some event handler to the even Click of the button. This handler should throw some exception.

Note that the "show some exception information" step mentioned above is not required and can be missing, it won't affect the manifestation of the bug. The double invocation of the Click event will happen anyway. And this is really the double invocation of the event, not double handling of the same exceptions or something like that. It can easily be checked up by adding some code line to the Click event handler, setting a breakpoint on this line and using the debugger. However, having some exception thrown at the end of the execution of the handler code is essential: without it, the effect of double event invocation won't be observed.

I use, for example, the following XAML for the demo window:

<Window x:Class="HowToReproduce.ReproduceProblemWindow"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    WindowStartupLocation="CenterScreen"
    Title="To Reproduce the Problem..." SizeToContent="WidthAndHeight">
    <StackPanel Margin="8">
        <TextBlock>Activate the button using Alt+B or blank space or mouse
        and see the difference<LineBreak/></TextBlock>
        <Button x:Name="button" Width="200">
            Test _Button Click</Button>
    </StackPanel>
</Window> 

This is how I throw the exception:

using System;
using System.Windows;

//...

public partial class ReproduceProblemWindow : Window {

    public ReproduceProblemWindow() {
        InitializeComponent();
        this.button.Focus();
        this.button.Click += (sender, evenArgs) => {
            throw new ApplicationException("Button click");
        };
    } //ReproduceProblemWindow

} //class ReproduceProblemWindow

using System.Windows;

//...

public partial class App : Application {

    static class DefinitionSet {
        internal const string FormatException = "{0}:\n\n{1}";
        internal const string FormatTitle = "{0} Exception";
    } //class DefinitionSet

    internal App() {
        DispatcherUnhandledException += (sender, eventArgs) => {
            eventArgs.Handled = true;
            MessageBox.Show(
                string.Format(
                    DefinitionSet.FormatException,
                    eventArgs.Exception.GetType().FullName,
                    eventArgs.Exception.Message),
                string.Format(
                    DefinitionSet.FormatTitle,
                    App.Current.MainWindow.Title),
                MessageBoxButton.OK,
                MessageBoxImage.Error);
        }; //DispatcherUnhandledException
    } //App

} //class App

4. The Workaround

One workaround is quite apparent: we can serialize the event invocation through the Dispatcher.Invoke method. This puts the call, with the parameters and local variables needed for the call, to the event queue of the UI thread of the application; this way the delegate instance will be taken from the queue and invoked only once.

Some background on this and related techniques is given in my past answers in the CodeProject Questions & Answers forum:

• Control.Invoke vs Control.BeginInvoke,
• Problem with Treeview Scanner And MD5.

Also, my article explains the inter-thread delegation idea and how it works under the hood:

• Simple Blocking Queue for Thread Communication and Inter-thread Invocation.

Now, I wanted the workaround which would require no changes in already written code and would require only the change in XAML, just for the cases when all buttons controls are declared only in XAML. For this reason, the new control I called FixedButton declares the event with the same exact name:

namespace WpfFix {
    using System;
    using System.Windows.Controls;

    internal class FixedButton : Button {

        protected override void OnClick() {
            if (Click != null)
                Dispatcher.Invoke(new Action(() => {
                    Click.Invoke(this, new EventArgs());
                }));
        } //OnClick

        new internal event EventHandler Click;

    } //class FixedButton

} //namespace WpfFix

Note the keyword new; it allows to avoid the compilation warning about having the member Button.Click hidden in the derived class. Generally, such hiding is bad, but it helps me to minimize the changes in the code formerly using the class System.Windows.Controls.Button.

Now, the problem is to replace all of the buttons in all XAML files with the new class. For this purpose, an XML namespace corresponding to WpfFix should be added; Visual Studio Intellisense would help to find the appropriate namespace declaration:

xmlns:Fix="clr-namespace:WpfFix"

Accordingly, the tag Button should be replaced with the tag Fix:FixedButton, and the attribute Name — with the attribute x:Name. This is the complete XAML for my Workaround Demo window:

<Window x:Class="Workaround.WorkaroundDemoWindow"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:Fix="clr-namespace:WpfFix"
    WindowStartupLocation="CenterScreen"
    Title="Workaround Demo" SizeToContent="WidthAndHeight">
    <StackPanel Margin="8">
        <TextBlock>
            Activate the button using Alt+B or blank space or mouse and see the difference:
            <LineBreak/>
            the click should be invoked only once,
            <LineBreak/>
            and the exception should be thrown and handled only once.
            <LineBreak/>
        </TextBlock>
        <Fix:FixedButton x:Name="button" Width="200">
            Test _Button Click</Fix:FixedButton>
    </StackPanel>
</Window>

There is one remaining problem: the simple MessageBox text in the exception handling method shown above won't be very useful: it would only show the exception TargetInvocationException with the message "Exception has been thrown by the target of an invocation", thus hiding the root cause of the exception.

But this is quite a general problem which should be solved somehow anyway, because such exception is the usual problem of the beginners debugging the code using any kind of invocations. The key here is: the original exception object is still preserved in the property System.Exception.InnerException. One approach is to use this property and show all inner exceptions recursively. This can be implemented in different ways. Here is, for example, my improved exception handling code:

using System;
using System.Windows;
using StringBuilder = System.Text.StringBuilder;
using StringList = System.Collections.Generic.List<string>;
using IStringList = System.Collections.Generic.IList<string>;

//...

public partial class App : Application {

    static class DefinitionSet {
        internal const string FormatException = "{0}:\n{1}";
        internal const string FormatTitle = "{0} Exception";
        internal const string ExceptionDelimiter = "\n\n";
    } //class DefinitionSet

    internal App() {
        DispatcherUnhandledException += (sender, eventArgs) => {
            Func<Exception, string> exceptionTextFinder = (ex) => {
                Action<Exception, IStringList> exceptionTextCollector
                    = null; // for recursiveness
                exceptionTextCollector = (exc, aList) => {
                    aList.Add(string.Format(
                        DefinitionSet.FormatException,
                        exc.GetType().Name,
                        exc.Message));
                    if (exc.InnerException != null)
                        exceptionTextCollector(exc.InnerException, aList);
                }; //exceptionTextCollector
                IStringList list = new StringList();
                exceptionTextCollector(ex, list);
                StringBuilder sb = new StringBuilder();
                bool first = true;
                foreach (string item in list)
                    if (first) {
                        sb.Append(item);
                        first = false;
                    } else
                        sb.Append(DefinitionSet.ExceptionDelimiter + item);
                return sb.ToString();
            };
            MessageBox.Show(
                exceptionTextFinder(eventArgs.Exception),
                string.Format(
                    DefinitionSet.FormatTitle,
                    App.Current.MainWindow.Title),
                MessageBoxButton.OK,
                MessageBoxImage.Error);
            eventArgs.Handled = true;
        }; //DispatcherUnhandledException
    } //App

} //class App 

That's all. The problem is very unpleasant, but the workaround is not too hard to apply.

5. Testing and Compatibility

The problem was tested only on .NET Framework v. 3.5 and v. 4.0, on Windows 7. I will be much grateful for testing the problem demo for other versions, platforms and the combinations

6. Some Conclusions. Are We Fine?

Of course not. This is just the workaround, it does not eliminate and cannot eliminate the problem. The developers should be aware of the problem and be careful. There are a number of ways in which the problem can still sneak in the code. One apparent way is the access to the Button.Click event — it is hidden by FixedButton.Click but cannot be made inaccessible. Here is the way to access it:

FixedButton button = new FixedButton();
//...
Button btn = button;
btn.Click += (sender, eventArgs) => {
    // will handle System.Windows.Controls.Button.Click
    // reproducing the same problem
}; 

Besides, the developer can accidentally always create and use the instance of System.Windows.Controls.Button directly.

So, the problem still remains and needs developer's attention. The problem can only be totally eliminated by fixing WPF itself.

I will be much grateful if someone could share some better idea, as well as for some criticism.

7. Credits

When this article was first published, CodeProject member Nicolas Séveno provided a link to Microsoft Connect Feedback: The Click event of a button is raised twice.

Thank you very much, Nicolas!

This issue is quite different, but probably somehow related. On 12/1/2011, the Microsoft representative acknowledged this bug but closed it with "Won't Fix" status and motivated this decision. I don't think this motivation provides enough excuse for having this bug, because there is no a perfect workaround, but this message provides a clue on what's going on:

Microsoft wrote:

The reason is that there are 2 ways to invoke the button: AccessKeyManager.OnKeyDown and AccessKeyManager.OnText. One is raised in response to a KeyDown(Key.Return) event, the other is raised on a TextInput("\r") event. The KeyDown event comes from WM_KEYDOWN and is normally marked as handled. But when throwing an exception from Click, the event is not marked as handled. Since you catch the exception we return out to the message pump which thinks WM_KEYDOWN was not handled and so calls TranslateMessage which generates a WM_CHAR. This is turned into the TextInput event which is routed back to the button, and the AccessKeyManager invokes the click again.

The only "fix" is to consider events as handled if their handlers raise exceptions. Unfortunately this is a policy change that would be hard to do at this point. So resolving this bug as Won't Fix.

Unfortunately, this explanation from Microsoft does not provide a way for a workaround. The attempt to mark the event as handled using, in case of my "steps to reproduce" code, eventArgs.Handled = true; (please see my second code fragment here), won't help. I did not yet check up the bug reported by Nicolas, but it looks like it also needs some different workaround, please see his comments to this article.