Dialog: Fixed dialog parameters re-set on each StateHasChanged overwriting current values

[uhfath]

When using parameters for dialog like this:

public partial class PackLockDialog : ComponentBase
{
    // ...
    [Parameter]
    public DateTime? LockedDate { get; set; }

    // ...
}

// ...

var parameters = new DialogParameters
{
    { nameof(PackLockDialog.LockedDate), DateTime.Today },
};

var dialogRef = dialogService.Show<PackLockDialog>(string.Empty, parameters);

They get re-set every time a StateHasChanged is called for the components, which use these parameters.
This PR adds a AreParametersRendered property for a DialogReference to restrict parameters setting only for the first time when the component is created.

[uhfath]

Here is a small repro from stock sample:
https://try.mudblazor.com/snippet/GEGlEUESAcaefyDW

Just open any dialog, change the date to anything and click "Update".
This triggers StateHasChanged for the dialog itself causing it to re-render itself and re-set parameters once again overwriting their current values.

[henon]

Please create a test case that will fortify this behavior against accidental corruption by future PRs

[uhfath]

While creating the test I've faced some weird behavior.
Here is a stripped minimal repro:
https://try.mudblazor.com/snippet/camluAkKIjvJzHsQ

1. open dialog;
2. change value;
3. click "Test";
4. notice value is reverted back.

But.
If you comment out the marked line and run the sample again it will work as expected.
I've spent the whole day trying to figure out why is this happening but failed.

Also if you change the "Color" parameter's type to be, for example, integer, then the bug also doesn't appear.
Why it is dependent on the number and type of parameters is a mystery to me.

I even changed "AddComponentReferenceCapture" method to also use correct sequence but that doesn't help either.

[henon]

it even happens with this:

[Parameter] public object Color1 { get; set; }

and

parameters.Add("Color1", Color.Error);

but it doesn't happen with this:

parameters.Add("Color1", 1);

So it doesn't depend on the type of the param but on the value. Why Color.Error causes this is beyond me also.

[henon]

Check this out, even defining an own enum and using it causes it https://try.mudblazor.com/snippet/mEQvuAEAgaFzxDOS

[henon]

This is the method that is called when adding the attribute. I wonder what it does with Enums.

       /// <summary>
        /// Appends a frame representing a string-valued attribute.
        /// The attribute is associated with the most recently added element. If the value is <c>null</c>, or
        /// the <see cref="System.Boolean" /> value <c>false</c> and the current element is not a component, the
        /// frame will be omitted.
        /// </summary>
        /// <param name="sequence">An integer that represents the position of the instruction in the source code.</param>
        /// <param name="name">The name of the attribute.</param>
        /// <param name="value">The value of the attribute.</param>
        public void AddAttribute(int sequence, string name, object? value)
        {
            // This looks a bit daunting because we need to handle the boxed/object version of all of the
            // types that AddAttribute special cases.
            if (_lastNonAttributeFrameType == RenderTreeFrameType.Element)
            {
                if (value == null)
                {
                    // Treat 'null' attribute values for elements as a conditional attribute.
                    TrackAttributeName(name);
                }
                else if (value is bool boolValue)
                {
                    if (boolValue)
                    {
                        _entries.AppendAttribute(sequence, name, BoxedTrue);
                    }
                    else
                    {
                        // Don't add anything for false bool value.
                        TrackAttributeName(name);
                    }
                }
                else if (value is IEventCallback callbackValue)
                {
                    if (callbackValue.HasDelegate)
                    {
                        _entries.AppendAttribute(sequence, name, callbackValue.UnpackForRenderTree());
                    }
                    else
                    {
                        TrackAttributeName(name);
                    }
                }
                else if (value is MulticastDelegate)
                {
                    _entries.AppendAttribute(sequence, name, value);
                }
                else
                {
                    // The value is either a string, or should be treated as a string.
                    _entries.AppendAttribute(sequence, name, value.ToString());
                }
            }
            else if (_lastNonAttributeFrameType == RenderTreeFrameType.Component)
            {
                // If this is a component, we always want to preserve the original type.
                _entries.AppendAttribute(sequence, name, value);
            }
            else
            {
                // This is going to throw. Calling it just to get a consistent exception message.
                AssertCanAddAttribute();
            }
        }

[henon]

Check this out. Casting the enum value to int works! https://try.mudblazor.com/snippet/QEclOqaKUvbpaaaP

[uhfath]

Well in our case everything from "AddAttribute" actually goes directly here:
https://github.com/dotnet/aspnetcore/blob/dc5e11abdb05b322f4b74b3afbcfb352fe984b2e/src/Components/Components/src/RenderTree/RenderTreeFrameArrayBuilder.cs#L61

public void AppendAttribute(int sequence, string attributeName, object? attributeValue)
{
    if (_itemsInUse == _items.Length)
    {
        GrowBuffer(_items.Length * 2);
    }

    _items[_itemsInUse++] = new RenderTreeFrame
    {
        SequenceField = sequence,
        FrameTypeField = RenderTreeFrameType.Attribute,
        AttributeNameField = attributeName,
        AttributeValueField = attributeValue,
    };
}

So the value is stored directly without any conversions.

I wonder if the problem lies somewhere inside rendering parts of Blazor framework itself.
Perhaps I'll try to come up with a clean repro without MudBlazor used - raw Blazor code.

[uhfath]

https://github.com/uhfath/TestBlazorParameters

A small repro with just the Blazor code.
Notice the value is reverted back as expected when "Test" button is clicked.

However, adding any number of extra parameters of different types doesn't change anything.

[henon]

Sure, it is logical that this will happen in your code. Even commenting out the Enum parameter doesn't change anything, so the Enum parameter is not the cause in your example.

In your example calling StateHasChanged re-renders the renderfragment which applies the same parameters again with which it was initialized. I am not sure this is is a real repro because as I have demonstrated it doesn't happen in the tryMB when you pass the enum value as an int: https://try.mudblazor.com/snippet/QEclOqaKUvbpaaaP

[henon]

Change this:

public partial class Index : ComponentBase
    {
        private string title = "test";

		private RenderFragment renderFragment => new RenderFragment(builder =>
		{
			var i = 0;
			builder.OpenComponent(i++, typeof(SurveyPrompt));
			builder.AddAttribute(i++, nameof(SurveyPrompt.Title), title);
			//builder.AddAttribute(i++, nameof(SurveyPrompt.TestEnum), TestEnum.Error);
			builder.AddAttribute(i++, nameof(SurveyPrompt.OnTest), (Action<string>)OnTest);
			builder.CloseComponent();
		});

		private void OnTest(string title)
        {
            this.title = title;
			StateHasChanged();
		}

	}

and

	[Parameter]
	public Action<string> OnTest { get; set; }

	private async void OnTestClicked()
	{
		Console.WriteLine("Child pre: {0}", Title);
		OnTest(Title);
		Console.WriteLine("Child post: {0}", Title);
	}

[uhfath]

I've done a bit more digging and this is what came up (taken from MudBlazor test).

This is a diff of a case when test is passed but shouldn't (one "TestValue" parameter):
https://www.textcompare.org/?id=60bf240e9f60320015151dbe

And this is when we use two parameters and the test fails:
https://www.textcompare.org/?id=60bf24a69f60320015151dbf

Notice the latter is completely identical as it should (the parameter is restored after StateHasChanged).
But the first one is a bit different.

The left part is before StateHasChanged.
Notice the missing values for "blazor:elementReference" on the right side.
Also they are completely absent in the latter case.

I'm not entirely sure what is this attribute for and why it's values are missing or showing up.
But I believe this could be the root cause of the problem.

If this attribute is used as a reference to elements then perhaps in the first case the framework couldn't find the exact elements to diff with, hence no parameter was changed.

[henon]

I don't know where we stand now. Do you think this is a bug in MudDialog or in the Blazor framework or is it a bug on your side application side?

[uhfath]

I actually believe this is a bug in MudBlazor, since parameters are overwritten every time.
But we went aside while fixing this because sometimes the bug wasn't reproducing.
On the other side it still needs to be fixed and so I will PR a new commit with some more code and tests.
It's just that my day job needed some more attention lately.

[uhfath]

Added a test and made some refactoring.
But the reasons for the described behavior are still a puzzle to be solved although the fix should be valid even for those cases.

[rapuyrav]

I have encountered a similar issue which seems very related to this.
When doing rich dialog interaction (with complex parameters), I'm using DisableBackdropClick = true to prevent data loss by unexpected cancellation. But a simple click on the background overlay, while not exiting the dialog, induce a overwriting back to the original parameters as described above.
I don't know if StateHasChanged is called during this process since the HandleBackgroundClick method is supposed to do nothing. Also I'm hosting server side if it's make a difference.
Easy workaround is to set the FullScreen to true, but it doesn't suits my need from a style perspective.
Hope this PR will fix it.

[henon]

Looks good. Thank you very much.