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

OnClick method on a Button within a foreach fires one time only #80

Closed
dylanberry opened this issue Feb 5, 2020 · 6 comments
Closed
Assignees
Labels
area-mobileblazorbindings Experimental Mobile Bindings for Blazor bug Something isn't working
Milestone

Comments

@dylanberry
Copy link

The OnSelectType method is only called the first time a button is clicked. Afterwards, the button shows the tap animation, but the method is not called.

@foreach (var option in Options)
{
    <Button Text="Select" OnClick="(() => OnSelectType(option))" />
}

Sample here: https://github.com/dylanberry/XamarinDynamicUI/tree/master/MobileBlazorBindingsFlags

@dylanberry
Copy link
Author

Also finding that IsVisible isn't being set properly:

@foreach (var option in Options)
{
       <Label Text="✓" IsVisible="option.IsSelected" />
}

@Eilon
Copy link
Member

Eilon commented Feb 5, 2020

Wondering - are you using the preview bits on nuget.org, or are you building from the source in this repo? I wouldn't be surprised at some of these issues being in the bits on nuget.org, but I think some are fixed in the latest source. I'll try both ways and see what I find. Thank you for reporting this!

@Eilon
Copy link
Member

Eilon commented Feb 5, 2020

OK I can reproduce this in the latest sources in the repo. I will investigate.

@Eilon Eilon self-assigned this Feb 5, 2020
@Eilon Eilon added area-mobileblazorbindings Experimental Mobile Bindings for Blazor bug Something isn't working labels Feb 5, 2020
@Eilon
Copy link
Member

Eilon commented Feb 5, 2020

Interestingly this bug doesn't repro in the todo app, but does repro in the code shown above.

What's happening is this:

  1. On first render:
    • The first Button.OnClick is mapped to let's say event handler id = 1
  2. On second render:
    • That same Button.OnClick is overwritten to let's say event handler id = 2 (this is fine)
    • And then it is un-mapped from event handler id = 1 (this is also fine)
    • But that Button is now event handler id 2, not 1, and it erroneously un-maps the new event handler id association, so it's not mapped to any event handler

And this seems to happen to all these buttons.

I think the fix will be to ensure that when events are un-mapped during a re-render to ensure they only un-map from the specific event being un-mapped and not to un-map from whatever the current event handler id happens to be. Hopefully an easy fix.

@Eilon Eilon closed this as completed in 54c20fe Feb 5, 2020
@Eilon
Copy link
Member

Eilon commented Feb 5, 2020

The fix was as straight-forward as I thought and it's checked in now. This was a very interesting issue to investigate so hopefully this fix works for you!

Here's the code I was using to test it:

<Frame CornerRadius="10" BackgroundColor="Color.LightBlue">

    <StackLayout Orientation="StackOrientation.Vertical">

        <Button Text="Increment" OnClick="IncrementCount" />

        <Label Text="@("The button was clicked " + count + " times")"
               FontAttributes="FontAttributes.Bold"
               VerticalTextAlignment="TextAlignment.Center" />

        @foreach (var option in Options)
        {
            <Button Text="@($"Select: {option.Name}")" OnClick="(() => OnSelectType(option))" />
        }


        @foreach (var option in Options)
        {
            <Label Text="@($"✓: {option.Name}")" IsVisible="option.IsSelected" />
        }

    </StackLayout>

</Frame>

@code
{
    class Option
    {
        public string Name { get; set; }
        public bool IsSelected { get; set; }
    }

    List<Option> Options = new List<Option>()
    {
        new Option { Name = "first", },
        new Option { Name = "second", },
        new Option { Name = "third", },
        new Option { Name = "fourth", },
    };

    async Task OnSelectType(Option o)
    {
        o.IsSelected = !o.IsSelected;
        //await Application.Current.MainPage.DisplayAlert("Selected", $"You selected: {o.Name}", "OK");
    }


    int count;

    void IncrementCount()
    {
        count++;
    }
}

@Eilon
Copy link
Member

Eilon commented Feb 5, 2020

Oh, and IsVisible was working fine for me. It was presumably fixed as part of #57, which also was about IsVisible not working.

@Eilon Eilon added this to the 0.2-preview milestone Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mobileblazorbindings Experimental Mobile Bindings for Blazor bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants