dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.5k stars 10.04k forks source link

Cannot pass through directive attributes, so components can't support @on{EVENT}:stopPropagation etc. #20560

Open rpedretti opened 4 years ago

rpedretti commented 4 years ago

Is your feature request related to a problem? Please describe.

This feature relates to https://github.com/dotnet/aspnetcore/issues/18460 where I'm building a custom styled component and want to keep all features for a simple tag, eg:

<div class="@boxClass" @attributes="AdditionalAttributes">
    @ChildContent
</div>

@code  {
    [Parameter(CaptureUnmatchedValues = true)]
    public IDictionary<string, object>? AdditionalAttributes { get; set; }
}

where, @boxClass is generated somewhere else. All attributes works except the @on{EVENT} in combination with @on{EVENT}:stopPropagation and/or @on{EVENT}:preventDefault.

Describe the solution you'd like

I'd expect that if I'm able to pass down the event handler it should be possible to pass the event custom attributes also since it's not practical (could be for a workaround) to have two flags for each possible event and declare them into the component.

Additional context

None

javiercn commented 4 years ago

@on{EVENT}:stopPropagation are not attributes, they are directives, so it is impossible for us to make that work (afaik).

@SteveSandersonMS can correct me if I'm wrong or suggest alternative approaches.

SteveSandersonMS commented 4 years ago

@javiercn is correct - there isn't currently a way to pass through arbitrary directive attributes.

That's because although it would seem to make sense in this case, there are plenty of other cases where it certainly wouldn't. For example, if someone was to use @key or @ref on your component, should that apply to the component itself, or to some other component or element that gets rendered by your component? The next opportunity we have to consider adding new programming model features to deal with this is in .NET Core 5.0, so I think we should keep this on the backlog until then.

@rpedretti In the meantime, I recommend you avoid trying to make the component handle literally all theoretically possible event types, and instead just add some specific properties for the event types you actually need to use. I know this isn't as convenient as being able to pass through arbitrary directive attributes.

rpedretti commented 4 years ago

Ok, I found a way using RenderFragment

// Box.tsx
@RenderParent()

@code
{
    [Parameter(CaptureUnmatchedValues = true)]
    public IDictionary<string, object>? AdditionalAttributes { get; set; }

    private RenderFragment RenderParent()
    {
        RenderFragment parent = builder =>
        {
            builder.OpenElement(1, "div");
            foreach (var (k, v) in AdditionalAttributes ?? new Dictionary<string, object>())
            {
                builder.AddAttribute(1, k, v);
            }

            foreach (var config in EventsConfig)
            {
                if (config.PreventDefault)
                {
                    builder.AddEventPreventDefaultAttribute(1, config.EventName, true);
                }
                if (config.StopPropagation)
                {
                    builder.AddEventStopPropagationAttribute(1, config.EventName, true);
                }
            }
            builder.AddContent(1, ChildContent);
            builder.CloseElement();
        };

        return parent;
    }
}

so the user can use like this

<Box @onclick="ClickHandler" EventsConfig="EventsConfig">
   Any content
</Box>

@code
{
    private EventConfig[] EventsConfig = new EventConfig[] {
        new EventConfig { EventName = "onclick", StopPropagation = true }
    };
}

And I agree about @key and @ref but they are standalone attributes while the @on{EVENT}:stopPropagation is not. Maybe there are some middle ground where we can capture these events directives the same way we capture unmatched parameters. But my way works fine for now.

mkArtakMSFT commented 4 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to happen for the coming release. We will reassess the backlog following the current release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.

stefanloerwald commented 3 years ago

Hi @SteveSandersonMS,

I came across this issue (again) and decided to look at code this time. For library developers who want to support arbitrary event listeners on their components, there's just no way around attribute splatting, but that mechanism doesn't support @onclick="Handler" @onclick:stopPropagation="Stop", because RZ10010 ("The component parameter 'onclick' is used two or more times for this component. Parameters must be unique (case-insensitive). The component parameter 'onclick' is generated by the '@onclick:stopPropagation' directive attribute.") will prevent compiling that code.

The thing is: I don't at all see why that's the case! This is what I observed with the razor compiler:

  1. @{eventName}:stopPropagation="value" (and similarly preventDefault), will generate the code __builder.AddEventStopPropagationAttribute(seq, eventName, value);
  2. AddEventStopPropagationAttribute is implemented in WebRenderTreeBuilderExtensions with a one-liner: https://github.com/dotnet/aspnetcore/blob/a4154ac58b07bce7ddb086427c1bc8a13073babd/src/Components/Web/src/Web/WebRenderTreeBuilderExtensions.cs#L50
  3. Since AddEventStopPropagationAttribute is implemented as builder.AddAttribute(sequence, $"__internal_stopPropagation_{eventName}", value);, @{eventName}:stopPropagation="value" could be replaced by __internal_stopPropagation_{eventName}="@value" with the exact same effect as AddEventStopPropagationAttribute.
  4. Testing this with .NET 5 suggests that replacing the directive by the internal attribute works perfectly fine: the stopPropagation behavior is correctly applied.

This makes me wonder why RZ10010 is there in the first place. The implementation does not use the same attribute name twice at all (because one attribute is named onclick and the other __internal_stopPropagation_onclick).

Could it perhaps be that this is an error that was necessary in some old version of blazor, but now is just obsolete because implementation changed?

javiercn commented 3 years ago

@stefanloerwald we are re-thinking this in more depth based on other work that we've been doing. You are correct in your assessment (I made this work myself in the way you suggest). That said we don't know if this is something we will get around in 6.0

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

simonziegler commented 2 years ago

Hi - did this get reviewed for ASP.NET 7? @stefanloerwald was working with us on Material.Blazor at the time of his comments 18 months back, and it would be good to have a resolution.

biohazard999 commented 1 year ago

This is also a blocker for us (or any body that is using a third party component library)

carlblanchard commented 1 year ago

Do you happen to have any news on the progress of this?

LunicLynx commented 1 year ago

Would love this as well. If the solution @stefanloerwald mentioned just works why not implement it? I can't see how this would be problematic in the future? @SteveSandersonMS @javiercn

LunicLynx commented 1 year ago

If it helps I could create the necessary PR for this.

javiercn commented 1 year ago

I think we want a more holistic approach to tackle this that requires a bit of design. Right now we have our hands full with other Blazor work, so I don't think we'll get to this in the near future.

I'd rather have a type that can capture the details about the binding at the call site, flow those through the system, and let the receiver make a call on what it wants to allow.

It's not just about configuring stopPropagation or prevent default, I would also be able to have something that allows the caller to choose the actual element event (like onchange or oninput)

LunicLynx commented 1 year ago

I completely get that, but this would be an easy fast fix that would not break anyone right now. So a Version x.x.1 increase. And your proposal sounds absolutely fantastic, since this always fell a bit short, but it is a version 1.x.x increase.

javiercn commented 1 year ago

@LunicLynx This can probably be done with a custom type and an extension method on the rendertreebuilder APIs if you want to do it yourself in the app.

I'd rather we avoid monkey patching this area in the core framework itself and just provide a clear and supported way of doing this instead.

LunicLynx commented 1 year ago

@javiercn Ok, I tried this now and I don't see a way without writing the internal name or switch to a custom renderer. I looked into TagHelpers, but as far as i can see they are not supported this way? Saw that internally the compiler uses TagHelperDescriptor but no way to make use of it.

You seem to have a better idea how this could be done, can you point me to an article or a source file I could lern from? Would be highly appreciated.

Thanks!

javiercn commented 1 year ago

Parent.razor

<Child @bind-Value="@value" BindingProperties="new BindValueProperties("oninput",preventDefault:true, ...)" />

Child.razor

[Parameter] BindValueProperties BindingProperties { get; set; }
<input unused="@BindingProperties" />

Some extension method

public static void AddAttribute(this RenderTreeBuilder builder, int sequence, string name, BindValueProperties properties)
{
   // map the event manually
   // builder.AddAttribute(....)
}
stefanloerwald commented 1 year ago

Some extension method

public static void AddAttribute(this RenderTreeBuilder builder, int sequence, string name, BindValueProperties properties)
{
   // map the event manually
   // builder.AddAttribute(....)
}

Wouldn't multiple operations in the extension method break the sequence numbers?

javiercn commented 1 year ago

You don't have to increment the sequence numbers.

I apologize in advance, that I have not figured out the details, and unfortunately, I don't have time to spend on this as I am heads down on Blazor united work. Take my suggestion as a rough sketch and play with it if you want, it might not pan out, but I think it can potentially be done.

MrX101 commented 1 year ago

Can you please explain to me why this isn't priotized, how is blazor useable when you can't control events triggering upwards into the other components? It just results in a mess... parent components doing things when a child component get clicked...This ticket has been here since 2020, this should have been fixed a long time ago. I don't see how blazor is useable without this.

bytefish commented 3 months ago

@mkArtakMSFT Is there any news on this issue? I just stumbled upon this issue as well, and don't see a simple workaround. Is there any progress on this issue?