Skip to content

Forms behavior clarifications #49340

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

Merged
merged 25 commits into from
Jul 17, 2023

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Jul 11, 2023

This is just about all the remaining usability work for SSR form handling.

In this PR:

  • Make <form> more usable
    • Make it automatically emit the hidden input with the handler name (which in turn respects any surrounding mapping scope name)
    • Make InputBase generate field names in a plain <form>
    • Requirements for submitting and receiving data from a <form> are reduced to having method=post and some @formname (also, <AntiForgeryToken> if that's enabled, which it is by default). Having an @onsubmit itself is an independent choice, and you don't have to supply a handler field manually.
  • Simplify EditForm
    • Stop emitting name/action attributes - they are no longer needed, so now the developer can do what they want with those (or nothing)
    • Stop creating a nested FormMappingScope - it's not used for anything, and would make it harder to work with [SupplyValueFromForm] on the same component that includes the EditForm as a child
  • Simplify use of [SupplyParameterFromForm]
    • Specifying the form handler name is now optional. If not set, it accepts any handler in the same form mapping scope.
  • Simplify semantics of FormMappingScope
    • Mapping scope name is now a distinct concept from the form handler name, so we can enforce different semantics for it (matching on it is not optional: forms can only trigger their submit event if they are in the right scope, and [SupplyParameterFromForm] will only receive values if it's in the right scope)
    • IDs are no longer multi-levelled. If you have <FormMappingScope>, its name simply overrides any parent name, like other cascaded values. It's simpler and still covers all the scenarios (and in most cases people won't use this anyway).
  • Fix a bug I discovered in the DOM syncing logic's handling of editable elements (e.g., when the .value prop is different from the value attribute)
  • Return a 400 (not 500) if there's a post with missing handler or handler not found (fixes When form is submitted with no handler, or the handler can't be found, change status to 400/404 #49359)
  • Require form handler names to be nonempty in order to accept the POST.
    • ...otherwise the default experience for adding a 2nd form to your site is that it breaks the 1st one (e.g., you add an EditForm to your layout, but don't realise this has now broken random unrelated pages that contain EditForm), plus the error message about "more than one named event uses the name ''" has poor usability.
  • Don't reject ambiguous forms at the point of rendering; only do so when dispatching the event. Fixes Don't reject ambiguous forms until we try to dispatch the submit event #49362
  • Rename @onsubmit:form to @formname

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jul 11, 2023
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/forms-behavior-clarifications branch 3 times, most recently from 1974dd1 to 1fbcb22 Compare July 12, 2023 11:35
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/named-events-tracking branch from d89d848 to d3122c7 Compare July 12, 2023 14:58
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/forms-behavior-clarifications branch from d11e239 to a0224b1 Compare July 12, 2023 14:59
Base automatically changed from stevesa/named-events-tracking to main July 12, 2023 16:51
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/forms-behavior-clarifications branch from d087641 to 24af917 Compare July 12, 2023 18:21
@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review July 13, 2023 14:39
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner July 13, 2023 14:39
@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Jul 14, 2023

One further usability issue I've realised. We're not providing any mechanism to handle the case where a submitted form has an unmatched handler name due to changes in underlying data. We just return a 400.

While designing this feature, we've been saying that people rendering lists would be advised to create a unique form or mapping scope name for each item in the list, derived from an ID for the item. But then this happens:

  • Users A and B both go to the list editor page at the same time
  • User A deletes item X
  • User B, still looking at the original version of the page before this change, submits a form related to item X

This will lead to a 400 error, but the app developer is not at fault here. They really need some way to provide a nice UX around "sorry, this item was already deleted" or similar. We don't provide a good way to do that if the error comes from "no matching handler".

The one way I can think of doing this well is having a single large form surrounding the entire list, and then for each item, have a different submit button, e.g.:

<form method="post" @formname="list-editor" @onsubmit="HandleAction">
    <AntiforgeryToken />
    <button name="deleteitem" value="1" type="submit">Delete item 1</button>
    <button name="deleteitem" value="2" type="submit">Delete item 2</button>
    <button name="deleteitem" value="3" type="submit">Delete item 3</button>
</form>

@code {
    [SupplyParameterFromForm] public int? DeleteItem { get; set; }

    void HandleAction()
    {
        if (DeleteItem.HasValue)
        {
            MyDb.DeleteById(DeleteItem.Value); // Or if it was already deleted, show some UI for that, etc.
        }
    }
}

AFAICT this works fine and is arguably much more useful since you can also round-trip other state in the single big form, and you only have to render AntiforgeryToken once.

My point with raising this is that we probably should not be advising people to generate form names programmatically based on IDs from data, unless they can truly guarantee the underlying data never changes in some way that would disrupt it.

Comment on lines +245 to +250
_shouldGenerateFieldNames = EditContext.ShouldUseFieldIdentifiers;
}
else
{
// Ideally we'd know if we were in an SSR context but we don't
_shouldGenerateFieldNames = !OperatingSystem.IsBrowser();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do this always or have a way to configure it in webassembly (as it was possible with EditContext). Otherwise people will start on server, write CSS rules using name, etc. and then suddenly those things will start to fail in webassembly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do this always or have a way to configure it in webassembly (as it was possible with EditContext)

It is still configurable in WebAssembly, as the EditContext approach still works. People who need the field names in WebAssembly for some reason can still use this to configure it on a per-area basis.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so this is then just moving it from EditContext to here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !OperatingSystem.IsBrowser() default is still in EditContext as a default, but there also needs to be a default in InputBase for the case where there is no EditContext.

throw new InvalidOperationException($"The mapping scope name '{Name}' starts with a disallowed character.");
}

_cascadingValueSupplier = new SupplyParameterFromFormValueProvider(FormValueModelBinder, Name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming: ModelBinder (replace for something else)

/// <param name="valueType">The <see cref="Type"/> of the value to map.</param>
/// <param name="parameterName">The name of the parameter to map data to.</param>
public FormValueMappingContext(string formName, Type valueType, string parameterName)
public FormValueMappingContext(string mappingScopeName, string? restrictToFormName, Type valueType, string parameterName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second parameter "sounds like a boolean" but it's not. why not just "formName"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While working on this code I found many parts of it hard to make sense of because they just had parameters called things like "name" or "handler" without indicating what it's a name of, whether it's an incoming value we're checking against vs an outgoing value we're emitting, etc. I had to keep tracing backwards through multiple layers of reference to work out what a value really represented.

In this case I was trying to clarify that this is not a name of the incoming form, but rather is a filter criteria that we're applying. It makes it clearer that when you read this value, you're not getting the name of the posted form (as it would sound if it was FormName), but rather are getting the name of a restriction that's in effect within this FormValueMappingContext.

Reviewing it now I see that's inconsistent with the name MappingScopeName so perhaps I should rename these two to AllowMappingScopeName and AllowFormName.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing it now I see that's inconsistent with the name MappingScopeName so perhaps I should rename these two to AllowMappingScopeName and AllowFormName.

Making this change in my follow-up PR.

@javiercn
Copy link
Member

  • Form tag requirements
    • What does it mean to have a form without onsubmit and why do we accept POST requests in that case.
    • How does this pattern scale up when the component goes interactive.
    • Hod do you ensure rules like validation/security have been applied prior to executing some business logic.

Overall looks similar to the existing implementation. There are a few things that are hard to understand:

  • Based on the new "form scope + form name" it's not clear what the combinations are. It seems that the scope limits the applicability of binding to a given context of the page, and that the form name can further restrict the binding.
  • It seems from the code, that providing a form name is always required. Why is the scope not enough?
  • Can the scope be empty? Does this mean that we bind from the root component all the way down?
  • Can we clarify what the different possible combinations of scope+form name are?
    • For example, is it correct to say that [SupplyFromForm] will bind on a matching scope independent of the form name?

Form as hidden input:

  • I think baking this into the framework at a low level is a mistake. I agree it is convenient to emit the field automatically, but you aren't buying much in terms of usability.
    • You still need to provide a formname.
    • You still need to provide antiforgery.
  • At that point, emiting the form name automagically as a hidden field doesn't add a ton of value as opposed to having a component that emits it for you.
  • More than that, it feels wrong force a concrete endpoint shape for handling actions instead of letting developers decide for themselves.
    • That means, we might want to instead offer a service that can be used to configure the dispatching aspect of the form (where do the scope+form name come from).
    • We might want to offer a component that does the three things above for you instead (and defaults to emiting a hidden field).
  • One of the challenging parts of putting the handler in a hidden field is that it is used to make "routing decissions" (in this case, what event to dispatch).
    • The default buffering limit for a form is 128MB in ASP.NET Core. It's trivial for someone with ill intentions to craft a form that puts the handler right at the end of a 128MB block, and as a result there are two things that are unclear:
    • Can this be used to DoS the app. (At the very least sounds like a pit of failure if you need to read 128Mb for determining the handler as opposed to reading it from the path, query, or a header, which is more constrained).
    • What happens when someone sends a form that exceeds the buffering limit (let's say 1GB data) without the form handler being present in that first GB.

Concrete suggestions:

  • Require an event handler to dispatch a form post.
  • Avoid having a hardcoded mechanism for dispatching the event. Let the developer decide where the scope+name come from.
    • Asses the security risks of dispatching the submit handler inside a hidden input field.
  • Provide an out of the box component that handles all aspects transparently:
    • Form name
    • Antiforgery
    • Emitting scope+handler in the way we choose to do so.

@SteveSandersonMS
Copy link
Member Author

What does it mean to have a form without onsubmit and why do we accept POST requests in that case.

It lets you send a POST request to the server which can receive data in the form body. Whether or not the developer wants to trigger an @onsubmit event or simply access data from the request is up to them.

It seems from the code, that providing a form name is always required. Why is the scope not enough?

It doesn't benefit anyone to find based on the scope alone, since the form name is always provided in the request by default.

Can the scope be empty? Does this mean that we bind from the root component all the way down?

The scope name is an empty string by default. And that means we will bind to any [SupplyParameterFromForm] that doesn't specify a form name or scope (as per the plan #48760). Supporting [SupplyParameterFromForm] without needing to specify a form name or scope makes it much more usable.

I think baking this into the framework at a low level is a mistake. I agree it is convenient to emit the field automatically, but you aren't buying much in terms of usability.

It's not really viable to expect people to write out the handler name by hand, as it involves combining with a potential scope name too. I find it makes a very significant difference in usability. It would be pretty much impractical to use this if people had to remember the markup for a hidden field with a magic name every time they wanted a form.

Having a component that emits this would still require baking special integration into the framework, as it would have no way of discovering the form name without magically looking at the rendertree of its parent.

More than that, it feels wrong force a concrete endpoint shape for handling actions instead of letting developers decide for themselves.

These endpoints are specifically Razor Component endpoints, not arbitrary server-side endpoints, so I don't find it problematic that we require a single specific system-defined field to indicate which form is being submitted. There are dozens of other places we control aspects of the data sent to the server when they tie in with framework features. For example, EditForm also does this and is even more opinionated (submitting antiforgery token, for example). This feature doesn't interfere with other forms that aren't for named events.

One of the challenging parts of putting the handler in a hidden field is that it is used to make "routing decissions" (in this case, what event to dispatch).

The routing decision is that it goes to a Razor Component endpoint, isn't it? The behavior after that is internal to Razor Component endpoints.

@javiercn
Copy link
Member

It lets you send a POST request to the server which can receive data in the form body. Whether or not the developer wants to trigger an @onsubmit event or simply access data from the request is up to them.

This does not answer the questions I raised, or why do we actually need to support this as opposed to binding the handler.

  • How does this pattern scale up when the component goes interactive.
  • How do you ensure rules like validation/security have been applied prior to executing some business logic.

It's not really viable to expect people to write out the handler name by hand, as it involves combining with a potential scope name too. I find it makes a very significant difference in usability. It would be pretty much impractical to use this if people had to remember the markup for a hidden field with a magic name every time they wanted a form.

Why then not a component that handles it for you, similar to how Antiforgery works?

Having a component that emits this would still require baking special integration into the framework, as it would have no way of discovering the form name without magically looking at the rendertree of its parent.

Why couldn't that be a cascading value that the renderer provides and the component can inject to generate the final string? We already do this for the scope.

These endpoints are specifically Razor Component endpoints, not arbitrary server-side endpoints, so I don't find it problematic that we require a single specific system-defined field to indicate which form is being submitted. There are dozens of other places we control aspects of the data sent to the server when they tie in with framework features. For example, EditForm also does this and is even more opinionated (submitting antiforgery token, for example). This feature doesn't interfere with other forms that aren't for named events.

EditForm is an optional component, you can choose to use it or use something different. This is a system primitive that users can't change.

The routing decision is that it goes to a Razor Component endpoint, isn't it? The behavior after that is internal to Razor Component endpoints.

Using the term routing might have caused confusion. What I mean is that we are forcing a concrete way of selecting what properties get to bind + what form gets to dispatch an action without any way for the developer to override that choice.

It's still not clear to me, what the implications of putting the value inside a hidden field are security wise, specially since the limits for the form are much bigger than for the request headers/URL. Specifically, some of the scenarios that I raised above.

@SteveSandersonMS
Copy link
Member Author

This does not answer the questions I raised, or why do we actually need to support this as opposed to binding the handler.

Perhaps the issue is that I'm not seeing what problem you're citing.

How does this pattern scale up when the component goes interactive.
How do you ensure rules like validation/security have been applied prior to executing some business logic.

If people want to put in logic that applies on an event handler, then they use an event handler. If they want to put logic in OnInitializedAsync they can do so. The proposal to force them to have a no-op event handler wouldn't stop them from putting logic in OnInitializedAsync. People will put logic where they want to, and it's their job to make it line up with their scenarios. For example if someone needs validation rules to be applied first, they will likely put their logic in OnValidSubmit. Making them have a no-op event handler won't change that.

Why then not a component that handles it for you, similar to how Antiforgery works?

As I mentioned, it doesn't save us from having to put special handling in the framework to make that work. So at that point, we might as well just make it even easier for people by covering the requirement automatically.

EditForm is an optional component, you can choose to use it or use something different. This is a system primitive that users can't change.

This only affects forms that submit to a Razor Component endpoint, not arbitrary forms or arbitrary endpoints. We've decided Razor Component endpoint form posts only work when a handler is specified, so it's helpful to do that.

What I mean is that we are forcing a concrete way of selecting what properties get to bind + what form gets to dispatch an action without any way for the developer to override that choice.

There are many kinds of choice the developer can make but I'm not sure which specific kinds of choices you think are required and missing here, or why it would be substantially different if we made them type out the handler field manually.

It's still not clear to me, what the implications of putting the value inside a hidden field are security wise

We put antiforgery data in there too, and it's less sensitive than that.

@javiercn
Copy link
Member

As I mentioned, it doesn't save us from having to put special handling in the framework to make that work. So at that point, we might as well just make it even easier for people by covering the requirement automatically.

This only affects forms that submit to a Razor Component endpoint, not arbitrary forms or arbitrary endpoints. We've decided Razor Component endpoint form posts only work when a handler is specified, so it's helpful to do that.

We put an opinion in, but any developer that build on top can override it and apply their own idiom. They aren't boxed into the handler being something that gets delivered within the form. A service can retrieve the value from the request and produce the required string. Anyone can choose how that value gets generated and in what form it gets delivered from the browser.

There are many kinds of choice the developer can make but I'm not sure which specific kinds of choices you think are required and missing here, or why it would be substantially different if we made them type out the handler field manually.

The choices are where those parameters come from (query, header, url or form body) as opposed to having a fixed way in the framework that can be changed.

We put antiforgery data in there too, and it's less sensitive than that.

It's different because by the time you check antiforgery you are already sure you want to do something with the body. In this case, you are trying to decide whether you want/need to do something with it, and that has a fix cost that can be controlled from a third-party.

With this in mind, I think at this point we have different views on the subject, so let's get this in.

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments in case you want to consider them

@SteveSandersonMS
Copy link
Member Author

Thanks for the further feedback. We can certainly add more options in the future if demand arises. Until then, hopefully the approach here will be meet needs in the vast majority of cases, while being pretty easy to discover.

@jonhilt
Copy link

jonhilt commented Oct 5, 2023

One further usability issue I've realised. We're not providing any mechanism to handle the case where a submitted form has an unmatched handler name due to changes in underlying data. We just return a 400.

While designing this feature, we've been saying that people rendering lists would be advised to create a unique form or mapping scope name for each item in the list, derived from an ID for the item. But then this happens:

  • Users A and B both go to the list editor page at the same time
  • User A deletes item X
  • User B, still looking at the original version of the page before this change, submits a form related to item X

This will lead to a 400 error, but the app developer is not at fault here. They really need some way to provide a nice UX around "sorry, this item was already deleted" or similar. We don't provide a good way to do that if the error comes from "no matching handler".

The one way I can think of doing this well is having a single large form surrounding the entire list, and then for each item, have a different submit button, e.g.:

<form method="post" @formname="list-editor" @onsubmit="HandleAction">
    <AntiforgeryToken />
    <button name="deleteitem" value="1" type="submit">Delete item 1</button>
    <button name="deleteitem" value="2" type="submit">Delete item 2</button>
    <button name="deleteitem" value="3" type="submit">Delete item 3</button>
</form>

@code {
    [SupplyParameterFromForm] public int? DeleteItem { get; set; }

    void HandleAction()
    {
        if (DeleteItem.HasValue)
        {
            MyDb.DeleteById(DeleteItem.Value); // Or if it was already deleted, show some UI for that, etc.
        }
    }
}

AFAICT this works fine and is arguably much more useful since you can also round-trip other state in the single big form, and you only have to render AntiforgeryToken once.

My point with raising this is that we probably should not be advising people to generate form names programmatically based on IDs from data, unless they can truly guarantee the underlying data never changes in some way that would disrupt it.

I tried implementing a similar approach to this, with some data being loaded via an external API and ran into some unexpected behaviour.

Here's a minimal repro:
https://github.com/jonhilt/BlazorNet8Testing/blob/master/RTMTest/Components/Pages/Home.razor

In this example there's one form which enables the user to search some arbitrary data.

Then the results load and they are rendered inside another form, for the purposes of enabling interaction with each result (but still using forms and static SSR).

At the moment, when you click a button in the search results, the form is submitted, but I was hoping the search results would remain present on the screen.

As it is, they disappear as soon as you try to interact with any of the items.

(this is with data-enhance enabled).

ssrsearch

dotnet: 8.0.100-rtm.23503.6

@ghost
Copy link

ghost commented Oct 5, 2023

Hi @jonhilt. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
3 participants