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

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

Open
rpedretti opened this issue Apr 5, 2020 · 21 comments
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) feature-rendering Features dealing with how blazor renders components severity-major This label is used by an internal tool
Milestone

Comments

@rpedretti
Copy link

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

This feature relates to #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 javiercn added the area-blazor Includes: Blazor, Razor Components label Apr 5, 2020
@javiercn
Copy link
Member

javiercn commented Apr 6, 2020

@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.

@javiercn javiercn added ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. question labels Apr 6, 2020
@ghost ghost added the Status: Resolved label Apr 6, 2020
@SteveSandersonMS
Copy link
Member

@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.

@SteveSandersonMS SteveSandersonMS changed the title Cannot use @on{EVENT}:stopPropagation with @on{Event} on custom component Cannot pass through directive attributes, so components can't support @on{EVENT}:stopPropagation etc. Apr 7, 2020
@SteveSandersonMS SteveSandersonMS added enhancement This issue represents an ask for new feature or an enhancement to an existing one and removed ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. Status: Resolved question labels Apr 7, 2020
@javiercn javiercn added this to the Next sprint planning milestone Apr 7, 2020
@rpedretti
Copy link
Author

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
Copy link
Member

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.

@SteveSandersonMS SteveSandersonMS added affected-most This issue impacts most of the customers severity-minor This label is used by an internal tool labels Oct 6, 2020 — with ASP.NET Core Issue Ranking
@SteveSandersonMS SteveSandersonMS added severity-major This label is used by an internal tool and removed severity-minor This label is used by an internal tool labels Oct 7, 2020
@javiercn javiercn added feature-rendering Features dealing with how blazor renders components feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) labels Apr 19, 2021
@stefanloerwald
Copy link

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:
    builder.AddAttribute(sequence, $"__internal_stopPropagation_{eventName}", value);
  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
Copy link
Member

@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

@TanayParikh TanayParikh modified the milestones: .NET 7 Planning, Backlog Oct 22, 2021
@ghost
Copy link

ghost commented Oct 22, 2021

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
Copy link

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
Copy link

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

@carlblanchard
Copy link

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

@LunicLynx
Copy link
Contributor

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
Copy link
Contributor

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

@javiercn
Copy link
Member

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
Copy link
Contributor

LunicLynx commented Mar 23, 2023

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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Member

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
Copy link

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
Copy link
Member

javiercn commented Mar 24, 2023

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
Copy link

MrX101 commented Aug 3, 2023

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
Copy link

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) feature-rendering Features dealing with how blazor renders components severity-major This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests