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

fix: FluentRadioGroup crashes inside EditForm if using an explicit EventCallback #1103

Closed
iotalambda opened this issue Dec 12, 2023 · 4 comments · Fixed by #1116
Closed

fix: FluentRadioGroup crashes inside EditForm if using an explicit EventCallback #1103

iotalambda opened this issue Dec 12, 2023 · 4 comments · Fixed by #1116

Comments

@iotalambda
Copy link

🐛 Bug Report

FluentRadioGroup crashes when its value is changed. The bug occurs at least inside EditForms when using explicit EventCallbacks.

💻 Repro or Code Sample

  1. dotnet new install Microsoft.FluentUI.AspNetCore.Templates::4.1.1
  2. dotnet new fluentblazor
  3. Replace Counter.razor contents with this:
@page "/counter"
@rendermode InteractiveServer

<h1>Works OK: @model1.Item</h1>
<EditForm Model=@model1>
    <FluentRadioGroup @bind-Value=model1.Item>
        <FluentRadio Value=@ItemType.Item1></FluentRadio>
        <FluentRadio Value=@ItemType.Item2></FluentRadio>
    </FluentRadioGroup>
</EditForm>


<h1>Crashes when selection is changed: @model2.Item</h1>
<EditForm Model=@model2>
    <FluentRadioGroup TValue=@ItemType Value=@model2.Item ValueChanged=@(v =>
        {
            model2.Item = v;
        })>
        <FluentRadio Value=@ItemType.Item1></FluentRadio>
        <FluentRadio Value=@ItemType.Item2></FluentRadio>
    </FluentRadioGroup>
</EditForm>

@code {
    class Model
    {
        public ItemType Item { get; set; }
    }
    enum ItemType { Item1, Item2 }
    Model model1 = new();
    Model model2 = new();
}

🤔 Expected Behavior

Changing both FluentRadioGroup values should work.

😯 Current Behavior

The second FluentRadioGroup crashes when changing its value.

@pk9r
Copy link
Contributor

pk9r commented Dec 14, 2023

I would like to provide some more information about this issue.

I tried creating project like report and got the exception below.

warn: Microsoft.AspNetCore.Components.Server.Circuits.RemoteRenderer[100]
      Unhandled exception rendering component: Value cannot be null. (Parameter 'obj')
      System.ArgumentNullException: Value cannot be null. (Parameter 'obj')
         at System.OrdinalCaseSensitiveComparer.GetHashCode(String obj)
         at Microsoft.AspNetCore.Components.Forms.FieldIdentifier.GetHashCode()
         at System.Collections.Generic.Dictionary`2.FindValue(TKey key)
         at System.Collections.Generic.Dictionary`2.TryGetValue(TKey key, TValue& value)
         at Microsoft.AspNetCore.Components.Forms.EditContext.GetValidationMessages(FieldIdentifier fieldIdentifier)+MoveNext()
         at System.Linq.Enumerable.<Any>g__WithEnumerator|36_0[TSource](IEnumerable`1 source)
         at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source)
         at Microsoft.AspNetCore.Components.Forms.FieldCssClassProvider.GetFieldCssClass(EditContext editContext, FieldIdentifier& fieldIdentifier)
         at Microsoft.AspNetCore.Components.Forms.EditContextFieldClassExtensions.FieldCssClass(EditContext editContext, FieldIdentifier& fieldIdentifier)
         at Microsoft.FluentUI.AspNetCore.Components.FluentInputBase`1.get_ClassValue() in /_/src/Core/Components/Base/FluentInputBase.cs:line 258
         at Microsoft.FluentUI.AspNetCore.Components.FluentRadioGroup`1.<BuildRenderTree>b__17_0(RenderTreeBuilder __builder2)
         at Microsoft.AspNetCore.Components.CascadingValue`1.Render(RenderTreeBuilder builder)
         at Microsoft.AspNetCore.Components.Rendering.ComponentState.RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment, Exception& renderFragmentException)
fail: Microsoft.AspNetCore.Components.Server.Circuits.CircuitHost[111]
      Unhandled exception in circuit 'tCbgIzDVJUN2RlnlZD6xJgvx2LdfX6LN9vzFyZIcBM4'.
      System.ArgumentNullException: Value cannot be null. (Parameter 'obj')
         at System.OrdinalCaseSensitiveComparer.GetHashCode(String obj)
         at Microsoft.AspNetCore.Components.Forms.FieldIdentifier.GetHashCode()
         at System.Collections.Generic.Dictionary`2.FindValue(TKey key)
         at System.Collections.Generic.Dictionary`2.TryGetValue(TKey key, TValue& value)
         at Microsoft.AspNetCore.Components.Forms.EditContext.GetValidationMessages(FieldIdentifier fieldIdentifier)+MoveNext()
         at System.Linq.Enumerable.<Any>g__WithEnumerator|36_0[TSource](IEnumerable`1 source)
         at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source)
         at Microsoft.AspNetCore.Components.Forms.FieldCssClassProvider.GetFieldCssClass(EditContext editContext, FieldIdentifier& fieldIdentifier)
         at Microsoft.AspNetCore.Components.Forms.EditContextFieldClassExtensions.FieldCssClass(EditContext editContext, FieldIdentifier& fieldIdentifier)
         at Microsoft.FluentUI.AspNetCore.Components.FluentInputBase`1.get_ClassValue() in /_/src/Core/Components/Base/FluentInputBase.cs:line 258
         at Microsoft.FluentUI.AspNetCore.Components.FluentRadioGroup`1.<BuildRenderTree>b__17_0(RenderTreeBuilder __builder2)
         at Microsoft.AspNetCore.Components.CascadingValue`1.Render(RenderTreeBuilder builder)
         at Microsoft.AspNetCore.Components.Rendering.ComponentState.RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment, Exception& renderFragmentException)

I started searching for the cause at /src/Core/Components/Base/FluentInputBase.cs
I see some code that causes this exception.

// file: /src/Core/Components/Base/FluentInputBase.cs
...
internal bool FieldBound => ValueExpression != null || ValueChanged.HasDelegate;
...
protected virtual string? ClassValue
{
    get
    {
        string? fieldClass = FieldBound ? EditContext?.FieldCssClass(FieldIdentifier) : null; // line 258: location of exception
        ...
    }
}
...
public override Task SetParametersAsync(ParameterView parameters)
{
        parameters.SetParameterProperties(this);

        if (!_hasInitializedParameters)
        {
            // This is the first run
            // Could put this logic in OnInit, but its nice to avoid forcing people who override OnInit to call base.OnInit()

            if (ValueExpression is not null)
            {
                FieldIdentifier = FieldIdentifier.Create(ValueExpression);
            }

            if (CascadedEditContext != null)
            ...

            _hasInitializedParameters = true;
        }
        ...
}
...

Above is the code I want the maintainer to pay attention to. According to the logic of the code if ValueExpression (Normally this is provided automatically when using 'bind-Value') is null and ValueChanged.HasDelegate is true, this will result in FieldBound being true to then execute EditContext?.FieldCssClass(FieldIdentifier) while FieldIdentifier is not created.

I propose to fix this issue by creating a FieldIdentifier when ValueChanged.HasDelegate is true, this solves this issue however I'm not sure it doesn't create additional problems.
It's very simple to do, just edit the code to

            if (ValueExpression is not null)
            {
                FieldIdentifier = FieldIdentifier.Create(ValueExpression);
            }
            else if (ValueChanged.HasDelegate)
            {
                FieldIdentifier = FieldIdentifier.Create(() => Value);
            }

I hope the above information will be useful.

@pk9r
Copy link
Contributor

pk9r commented Dec 14, 2023

@iotalambda On the user side, you can temporarily provide manual ValueExpression to avoid crashes

<EditForm Model=@model2>
    <FluentRadioGroup TValue=@ItemType Value=@model2.Item
                      ValueExpression="@(() => model2.Item)"
                      ValueChanged=@(v => model2.Item = v)>
        <FluentRadio Value=@ItemType.Item1></FluentRadio>
        <FluentRadio Value=@ItemType.Item2></FluentRadio>
    </FluentRadioGroup>
</EditForm>

@vnbaaij
Copy link
Collaborator

vnbaaij commented Dec 14, 2023

Hi La Minh Phuc,
Yes that code is very useful! Where did you put it? In SetParametersAsync? (to replace what was there at lines 286-289)

@pk9r
Copy link
Contributor

pk9r commented Dec 14, 2023

Yes, put it in SetParametersAsync and replace the old if statement.

@vnbaaij vnbaaij linked a pull request Dec 14, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants