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

[Components] EventCallback<T> changes the RenderTreeBuilder.AddAttribute(object) behavior #8336

Closed
chucker opened this issue Mar 8, 2019 · 4 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed

Comments

@chucker
Copy link

chucker commented Mar 8, 2019

Describe the bug

The introduction of EventCallback<T> in #6351 changes the behavior of RenderTreeBuilder.AddAttribute(object) such that calls that were previously treated as delegates — an onclick attribute, for example — now fall back to a ToString() call.

This breaks code between Blazor 0.8 and 0.9, and doesn't appear to be the intended behavior of the AddAttribute(object) overload. There still exists a case for delegates, but it doesn't seem to be used any more:

// BUG: Action<UIMouseEventArgs> in Blazor 0.8 _was_ a MulticastDelegate
// but EventCallback<UIMouseEventArgs> is not!
else if (value is MulticastDelegate)
{
    Append(RenderTreeFrame.Attribute(sequence, name, value));
}

To Reproduce

Steps to reproduce the behavior:

  1. Using this version of ASP.NET Core: 3.0.0-preview3-19153-02
  2. Run this code:
        protected override void BuildRenderTree(RenderTreeBuilder builder)
        {
            base.BuildRenderTree(builder);
            builder.OpenElement(0, "MyCustomTag");

            var param = new KeyValuePair<string, object>("onclick", 
                EventCallback.Factory.Create<UIMouseEventArgs>(this, s => { }));

            builder.AddAttribute(2, param.Key, param.Value);
            builder.CloseElement();
        }

Expected behavior

param.Value should be recognized as a delegate.

Actual behavior

RenderTreeBuilder falls back to treating param.Value as an unrecognized object, and calls ToString() on it.

Therefore, in the generated DOM in the browser looks something like:

<button
onclick="Microsoft.AspNetCore.Components.EventCallback`1[[Microsoft.AspNetCore.Components.UIMouseEventArgs,
Microsoft.AspNetCore.Components,
Version=3.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]]">

Additional context

This came up in the BlazorStrap project, and is tracked there as chanan/BlazorStrap#42.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 8, 2019
@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Mar 8, 2019
@mkArtakMSFT
Copy link
Member

Thanks for contacting us, @chucker.
@SteveSandersonMS was this intentional? Do we plan to do anything here?

@SteveSandersonMS
Copy link
Member

I don’t think we expected this to affect anything for other delegate types, but @rynowak has the most context on this change.

@danroth27 danroth27 added this to the 3.0.0-preview6 milestone Mar 11, 2019
@rynowak
Copy link
Member

rynowak commented Mar 12, 2019

We will need to fix this, it's basic quality of life for using C# to author components. This will come up again when we do splatting as well.

@mkArtakMSFT mkArtakMSFT added PRI: 1 - Required bug This issue describes a behavior which is not expected - a bug. labels Apr 19, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
rynowak pushed a commit that referenced this issue May 19, 2019
- Adds AddMultipleAttributes
- Fix RTB to de-dupe attributes
- Fix RTB behaviour with boxed EventCallback (#8336)
- Add lots of tests for new RTB behaviour and EventCallback
rynowak pushed a commit that referenced this issue May 21, 2019
- Adds AddMultipleAttributes
- Fix RTB to de-dupe attributes
- Fix RTB behaviour with boxed EventCallback (#8336)
- Add lots of tests for new RTB behaviour and EventCallback
rynowak pushed a commit that referenced this issue May 24, 2019
- Adds AddMultipleAttributes
- Fix RTB to de-dupe attributes
- Fix RTB behaviour with boxed EventCallback (#8336)
- Add lots of tests for new RTB behaviour and EventCallback
rynowak pushed a commit that referenced this issue May 26, 2019
- Adds AddMultipleAttributes
- Fix RTB to de-dupe attributes
- Fix RTB behaviour with boxed EventCallback (#8336)
- Add lots of tests for new RTB behaviour and EventCallback
rynowak pushed a commit that referenced this issue May 28, 2019
- Adds AddMultipleAttributes
- Fix RTB to de-dupe attributes
- Fix RTB behaviour with boxed EventCallback (#8336)
- Add lots of tests for new RTB behaviour and EventCallback
rynowak added a commit that referenced this issue May 29, 2019
* Add basic tests for duplicate attributes

* Add AddMultipleAttributes improve RTB

- Adds AddMultipleAttributes
- Fix RTB to de-dupe attributes
- Fix RTB behaviour with boxed EventCallback (#8336)
- Add lots of tests for new RTB behaviour and EventCallback

* Harden EventCallback test

This was being flaky while I was running E2E tests locally, and it
wasn't using a resiliant equality comparison.

* Add new setting on ParameterAttribute

Adds the option to mark a parameter as an *extra* parameter. Why is this
on ParameterAttribute and not a new type? It makes sense to make it a
modifier on Parameter so you can use it both ways (explicitly set it, or
allow it to collect *extras*).

Added unit tests and validations for interacting with the new setting.

* Add renderer tests for 'extra' parameters

* Refactor Diagnostics for more analyzers

* Simplify analyzer and fix CascadingParameter

This is the *easy way* to write an analyzer that looks at declarations.
The information that's avaialable from symbols is much more high level
than syntax. Much of what's in this code today is needed to reverse
engineer what the compiler does already. If you use symbols you get to
benefit from all of that.

Also added validation for cascading parameters to the analyzer that I
think was just missing due to oversight.

The overall design pattern here is what I've been converging on for the
ASP.NET Core analyzers as a whole, and it seems to scale really well.

* Add analyzer for types

* Add analyzer for uniqueness

This involved a refactor to run the analyzer per-type instead of
per-property.

* Fix project file

* Adjust name

* PR feedback on PCE and more renames

* Remove unused parameter

* Fix #10398

* Add E2E test

* Pranavs cool feedback

* Optimize silent frame removal

* code check

* pr feedback
@rynowak
Copy link
Member

rynowak commented May 29, 2019

This was fixed in #10357

@rynowak rynowak closed this as completed May 29, 2019
@rynowak rynowak added Done This issue has been fixed and removed Working labels May 29, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

6 participants