Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[xaml] locate event handlers from base types #17075

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Aug 29, 2023

Fixes #11467
Context: https://github.com/maiia-kuzmishyna/MAUI-ProtectedEventHandlers

Reviewing the customer sample, it appears to be a valid case. Consider a ChildButton defined in XAML:

<local:ParentButton
    xmlns="http://xamarin.com/schemas/2014/forms"
    xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
    xmlns:local="clr-namespace:foo"
    x:Class="foo.ChildButton">
</local:ParentButton>

Where ParentButton is also defined in XAML:

<Button
    xmlns="http://xamarin.com/schemas/2014/forms"
    xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
    x:Class="foo.ParentButton"
    Clicked="ParentButton_OnClicked">
</Button>

Where ParentButton_OnClicked is of course private:

private void ParentButton_OnClicked(object sender, EventArgs e) { }

Throws an exception at runtime with:

No method ParentButton_OnClicked with correct signature found on type foo.ChildButton

What was also odd, it appears the problem just "goes away" in Release mode, meaning it works under XamlC. This kind of points to a bug with non-compiled XAML.

It appears the problem was the code:

foreach (var mi in rootElement.GetType().GetRuntimeMethods())
{
    //...
    if (mi.Name == (string)value)
    {
        addMethod.Invoke(element, new[] { mi.CreateDelegate(eventInfo.EventHandlerType, mi.IsStatic ? null : rootElement) });
        return true;
    }
    //...
}

In this example, rootElement is of type ChildButton while the method is on type ParentButton. As mentioned on:

https://stackoverflow.com/a/2267299

You need to access Type.BaseType to find private methods on base types.

I changed this to instead:

  • Iterate over rootElement.GetType() and its base types.

  • Call Type.GetMethod() using the method name, passing the appropriate BindingFlags.

This appears to make the new test pass, solving the issue.

Fixes: dotnet#11467
Context: https://github.com/maiia-kuzmishyna/MAUI-ProtectedEventHandlers

Reviewing the customer sample, it appears to be a valid case. Consider a
`ChildButton` defined in XAML:

    <local:ParentButton
        xmlns="http://xamarin.com/schemas/2014/forms"
        xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
        xmlns:local="clr-namespace:foo"
        x:Class="foo.ChildButton">
    </local:ParentButton>

Where `ParentButton` is also defined in XAML:

    <Button
        xmlns="http://xamarin.com/schemas/2014/forms"
        xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
        x:Class="foo.ParentButton"
        Clicked="ParentButton_OnClicked">
    </Button>

Where `ParentButton_OnClicked` is of course private:

    private void ParentButton_OnClicked(object sender, EventArgs e) { }

Throws an exception at runtime with:

    No method ParentButton_OnClicked with correct signature found on type foo.ChildButton

What was also odd, it appears the problem just "goes away" in Release
mode, meaning it works under XamlC. This kind of points to a bug with
non-compiled XAML.

It appears the problem was the code:

    foreach (var mi in rootElement.GetType().GetRuntimeMethods())
    {
        //...
        if (mi.Name == (string)value)
        {
            addMethod.Invoke(element, new[] { mi.CreateDelegate(eventInfo.EventHandlerType, mi.IsStatic ? null : rootElement) });
            return true;
        }
        //...
    }

In this example, `rootElement` is of type `ChildButton` while the method
is on type `ParentButton`. As mentioned on:

https://stackoverflow.com/a/2267299

You need to access `Type.BaseType` to find private methods on base types.

I changed this to instead:

* Iterate over `rootElement.GetType()` and its base types.

* Call `Type.GetMethod()` using the method name, passing the appropriate
  `BindingFlags`.

This appears to make the new test pass, solving the issue.
@jonathanpeppers jonathanpeppers added the area-xaml XAML, CSS, Triggers, Behaviors label Aug 29, 2023
Comment on lines +14 to +22
EventHandler _myEvent;

public event EventHandler MyEvent
{
add => _myEvent += value;
remove => _myEvent -= value;
}

public int MyEventSubscriberCount => _myEvent.GetInvocationList().Length;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added my own event here, so I could assert MyEventSubscriberCount is 1.

@samhouts samhouts added this to the .NET 8 GA milestone Aug 29, 2023
@jonathanpeppers jonathanpeppers marked this pull request as ready for review August 29, 2023 20:46
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner August 29, 2023 20:47
@rmarinho rmarinho merged commit c2626ba into dotnet:main Sep 4, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2023
@samhouts samhouts added the fixed-in-8.0.0-rc.2.9373 Look for this fix in 8.0.0-rc.2.9373! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-8.0.0-rc.2.9373 Look for this fix in 8.0.0-rc.2.9373!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Private EventHandlers throw an exception on XAML-defined custom control, inherited by other control
4 participants