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

Support multiple event handlers for the same event on the same element #14365

Closed
SteveSandersonMS opened this issue Sep 24, 2019 · 9 comments
Closed
Assignees
Labels
affected-medium This issue impacts approximately half of our customers area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues 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) Needs: Design This issue requires design work before implementating. Priority:1 Work that is critical for the release, but we could probably ship without severity-major This label is used by an internal tool
Milestone

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Sep 24, 2019

Currently, Blazor only allows a single onsomething for a given element. A consequence of this is that:

  • if you're using @bind you can't also use @onchange (because the framework is using it)
  • if you're using @bind:event="oninput" you can't also use @oninput (because the framework is using it)

This is a limitation we should be able to relax, and would help fix issues like #14242 (comment)

Ways we could fix it:

  • At compile-time, by making the Razor compiler able to detect duplicate @onsomeevent (including ones it creates itself via @bind) and collapse them into a single @onsomeevent that calls a code-genned method that runs all the handlers.
    • Some design would be needed to decide on execution order. Do we run them in parallel, or sequentially? Do we inject StateHasChanged calls that run after each of the returned tasks complete, so as to preserve the usual updating behavior?
  • At runtime, by changing the diff algorithm to be able to handle multiple values for a given-named attribute. It might be tricky to do this while preserving current perf characteristics, but I haven't attempted yet.
    • We'd also need the ts-side code to understand that, when a certain DOM event occurs, it might need to trigger multiple event handlers. If it currently stores the eventname->handler_id mappings as a dictionary keyed by eventname, it would have to be generalised.
@SteveSandersonMS SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Sep 24, 2019
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Sep 24, 2019
@mkArtakMSFT mkArtakMSFT added this to the 5.0.0-preview1 milestone Sep 24, 2019
@rynowak
Copy link
Member

rynowak commented Sep 25, 2019

a single @onsomeevent that calls a code-genned method that runs all the handlers.

Let's be careful when we get here - this kind of solution will be really tricky because we don't want to influence evaluation order.

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Sep 26, 2019

Agreed it's tricky.

One especially difficult bit, that might even be fatally difficult to most designs, is making this work alongside our existing AddMultipleAttributes-overriding logic. Currently we reason that, since you can only finish with one attribute with a given name, we can have a precedence rule whereby later instances overwrite earlier ones. It's unclear how this makes sense in a world where you're allowed to finish with multiple instances of a given attribute - how do we know when to overwrite, and when to duplicate?

Maybe the distinction is just "event handlers combine, other things overwrite" but I'm not confident that's desirable without a lot more careful consideration.

@orthoxerox
Copy link

I've ran into this with my toy application: three sliders that are bound to three properties and must recalculate a few output properties on change. The most concise way would be to @bind the sliders to the properties and handle @onchange with Recalculate().

This means that @bind must come first in the combined event handler, but I wonder if anyone has the opposite requirement.

@mrlife
Copy link
Contributor

mrlife commented May 24, 2020

It's unclear how this makes sense in a world where you're allowed to finish with multiple instances of a given attribute - how do we know when to overwrite, and when to duplicate?

Maybe the distinction is just "event handlers combine, other things overwrite" but I'm not confident that's desirable without a lot more careful consideration.

@SteveSandersonMS For event handlers, you could add -callback or similar to the event handler name, e.g. onchange-callback to indicate "run this after the built-in onchange event handler".

@mkArtakMSFT mkArtakMSFT added the Needs: Design This issue requires design work before implementating. label Jul 23, 2020
@ghost
Copy link

ghost commented Jul 28, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@SteveSandersonMS SteveSandersonMS added affected-medium This issue impacts approximately half of our customers severity-major This label is used by an internal tool labels Oct 9, 2020 — with ASP.NET Core Issue Ranking
@ghost
Copy link

ghost commented Oct 9, 2020

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.

@szalapski
Copy link

szalapski commented Dec 11, 2020

I would very much like to see this worked on. I have in many components implemented an EventCallback parameter such as ValueChanged, upon which I always implement another parameter EventCallback ValueChanged2. The first is to support @bind-Value, the second to support actual custom events. In every case, I have some kind of private Task ValueChangedInternal() method that always does the internal change work, then calls await ValueChanged.InvokeAsync(Value); then await ValueChanged.InvokeAsync(Value);. I figured I cannot run ValueChanged2 before I await ValueChanged, as the result of ValueChanged (for @bind-value) might be required in whatever event is wired up to ValueChanged2.

If you could support @bind-Value with also having a custom ValueChanged unrelated to the binding, that would clean this up a lot. I would not expect it to be super-deluxe foolproof, just to finish the @Bind function (which should always be quite fast, right?) and then start the custom ValueChanged function.

@javiercn javiercn added the feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) label Apr 19, 2021
@mkArtakMSFT mkArtakMSFT added Priority:1 Work that is critical for the release, but we could probably ship without triaged labels Nov 5, 2021
@SteveSandersonMS SteveSandersonMS self-assigned this Jan 12, 2022
@SteveSandersonMS
Copy link
Member Author

Note that #39837 supersedes this requirement. That might seem different, but it's a proposal to enhance @bind in some ways that, if done, would entirely cover the realistic use cases expressed here.

@mkArtakMSFT
Copy link
Member

Closing as this is being superseded by #39837 which we've decided to move forward with.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-medium This issue impacts approximately half of our customers area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues 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) Needs: Design This issue requires design work before implementating. Priority:1 Work that is critical for the release, but we could probably ship without severity-major This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

7 participants