-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
Clicking a submit <button> or <submit> input element doesn't trigger parent <form>'s onsubmit event handler #563
Comments
Hey @egil I had a short look at the discussion and the bug itself. An idea (out of my brain, this is most likely not compilable) to extend if (eventName == "click" && element is IHtmlInputElement { Type: "submit" } && candidate is IHtmlFormElement form)
{
try
{
var info = new EventFieldInfo() { FieldValue = "onsubmit" };
form.TryGetEventId(Htmlizer.ToBlazorAttribute("onsubmit"), out var formSubmitEventId)
eventTasks.Add(renderer.DispatchEventAsync(formSubmitEventId, info, eventArgs));
}
catch (UnknownEventHandlerIdException) when (eventTasks.Count > 0)
{
// Capture and ignore NoEventHandlerException for bubbling events
// if at least one event handler has been triggered without throwing.
}
} Of course without doubling all the logic et cetera... Any input on that? |
Hi, thanks for taking the time. You are on the right track. The challenge here is to make sure that we do exactly as the browser does, and bubble the right types of events to the correct In general, this is a specialization of the bubbling implementation in TriggerBubblingEvent you found, we just need to figure out all corner cases/scenarios. This issue is definitely up for grabs if you want to take a crack at it. |
Hey @egil thanks for the quick response. I can try to have a look at the end of the week / weekend, but can't promise anything. note to myself: We can leverage overriding the TriggerBubble event for IHtmlInputElement |
I try to keep this comment up to date with all the findings / requirements I found.
|
Sounds good. If you find any other issues while investing this, we can expand this issue. I'm very hung up with a work thing the next two days, but hopefully get back to this on Friday. |
<button form="my-form">Click to submit</button>
<form id="my-form" onsubmit="(() => console.log('submitted'))()">
</form> Would print "Submitted" on the console and submit afterwards the form. And here a very super special edge-case: <form onsubmit="(() => console.log('submitted'))()">
<input type="text" name="name" />
<input type="submit" id="submit-form" class="hidden" />
</form>
<label for="submit-form" tabindex="0">Submit</label> clicking on the label would raise the submit |
I made a small proposal PR. It does not fix the last issue (button / label outside the form which can raise the onsubmit event). Right now 1 test is failing which invokes I purposely throw an exception even though the browser would just silently swallow any |
Great. I will take a look tonight and give you my feedback. |
Check.
I have no objection to switching to a mode where we invoke and then await each event handler in the proper order. Then, if users need, they can use the async versions of the trigger methods and await them.
So what you are saying is that if we have an Another point of view would be that this would not work in the browser, assuming Blazor does as the spec specifies, and then its very unlikely that users would get into a situation where they write a test in bUnit for something that will never work i Blazor.
Gah, well, we can work around that by using the DOM querying capabilities to find the related |
Sorry I wrote that a bit confusing. The browser as well as blazor behaves here the same: Only the <EditForm OnValidSubmit="OnSubmit" Model="model">
<button @onsubmit="OnButtonSubmit"> Submit</button>
</EditForm>
@code {
private MyModel model = new();
private void OnSubmit()
{
Console.WriteLine("Valid Form Submit");
}
private void OnButtonSubmit()
{
Console.WriteLine("I will be not triggered");
}
} Clicking on the submit button will only Display:
|
Yep, let's just leave that as it is. It took almost 2 years before somebody tripped over this one 😂 |
I tried to create a valid test case for the <div id="outer-div" @onclick="OuterDivClick">
<div id="inner-div" @onclick="InnerDivClick">
<button id="button" @onclick="ButtonClick"></button>
</div>
</div>
@code {
public Stack<string> ComponentCallStack = new();
private void OuterDivClick()
{
ComponentCallStack.Push("outer div");
}
private void InnerDivClick()
{
var b = CountPartitions(20, 9);
Console.WriteLine(b);
ComponentCallStack.Push("inner div");
}
private void ButtonClick()
{
ComponentCallStack.Push("button");
}
private static int CountPartitions(int elements, int subsets)
{
if (elements == 0 || subsets == 0 || subsets > elements)
return 0;
if (subsets == 1 || subsets == elements)
return 1;
return subsets * CountPartitions(elements - 1, subsets)
+ CountPartitions(elements - 1, subsets - 1);
}
} Practically speaking |
So having a look at this: await Task.WhenAll(DoSomething(1000, "1"), DoSomething(10, "2"));
static async Task DoSomething(int timeout, string output)
{
await Task.Delay(timeout);
Console.WriteLine(output);
} This will output:
We clearly see that it's not in order. But I don't get it reproduced with the events :D EDIT: await Task.WhenAll(DoSomething(1000, "1"), DoSomething(10, "2"));
static Task DoSomething(int timeout, string output)
{
var t = Task.Delay(timeout);
Console.WriteLine(output);
return t;
} Output:
Without directly awaiting we get in order again... and that makes sense. Because we don't await on our own we leave it to |
So blazor will run as much it can synchronously, an entire event handler if possible ø So, actually, now that I think of it, |
Describe the bug
<input type="submit">
or<button type="submit">
onclick
events do not triggeronsubmit
events a parent<form>
element.Example:
Testing this component:
With this test:
Results in this output:
Test fails because there is no
onclick
handler on the button/input element. If there is, then the test fails because the form'sonsubmit
is not bubbled to.Expected behavior:
Version info:
Additional context:
The text was updated successfully, but these errors were encountered: