Continuation of PR 40097 (Input components should use SetUpdatesAttributeName)

[SteveSandersonMS]

This is a continuation of #40326. I need to make it into a new PR because the original one is on a branch I can't directly write to. For the details about what this fixes, consider reading the conversations on the earlier PRs and the original issue that led to all this. As a very brief summary:

• InputText and the other Input* components do not currently use SetUpdatesAttributeName during rendering, so the diffing system doesn't know what text is currently in the DOM, and hence it emits updates to the value if and only if the bound .NET value changes (regardless of what's in the DOM)
• This leads to a problem if the .NET property setter reverts an incoming change. Doing so means the .NET value is effectively unchanged, and hence the diffing system thinks there's nothing to do, so the DOM value is left alone even though it has changed. The UI is then out of sync with the .NET state.
• An earlier PR tried to fix this by just adding SetUpdatesAttributeName, but that led to a different problem whereby the Input components lost the ability to represent invalid states (e.g., an InputNumber being blank even though it's bound to an int which cannot have no value).
• To avoid breaking existing apps and losing the ability to represent invalid states, the newer PR keeps track of the possibly-unparseable incoming values and reflects those back out to the DOM instead of just emitting the latest-validly-parsed model state.

@hakenr, thanks again for contributing this! I know it's take a long time to get this in. Hopefully that doesn't come as a surprise because we previously discussed aiming to do this in an early .NET 8 preview, and that's where we are now.

I think the PR at #40326 is already good and almost ready to merge. However there's a bit of remaining work to rebase on main and a couple of tweaks I want to make the the new public API, so I'll do that directly here if that's OK.

[SteveSandersonMS]

As an open design question, we have to decide where to put the possibly-unparseable incoming value so that subclasses can use it in their rendering logic. I think there are three main options:

• Most risk-averse: We leave CurrentValueAsString alone (so it continues only to contain the most recent valid value) and introduce a new property LatestUserInputWhichMightNotEvenBeParseable, with some better name. Then we recommend that people subclassing InputBase switch to use LatestUserInputWhichMightNotEvenBeParseable if they can.
• Middle option: We change CurrentValueAsString to represent the latest user input which may or may not be parseable. But in case someone was relying on getting the last-good-value, we add a new property MostRecentParsedValueAsString so people can switch to that if they need it for back-compat.
• Most pro-risk: We avoid introducing any new API at all, and instead just change CurrentValueAsString to mean the latest user input, whether or not it was parseable. This is technically a breaking change, but in the vast majority of cases it would improve the behavior of existing InputBase subclasses. In the very rare case where someone really does need the "most recently parsed value as a string", they can use CurrentValue and stringify it themselves.

I was previously advocating a risk-averse approach here, but on further consideration I am now siding with the pro-risk option because I don't want to have multiple different kinds of "current value" that component authors would have to understand and distinguish. It feels better just to make the existing property represent the value people should most likely actually be using. Then we have no new API and existing subclasses will just start behaving better. But we would have to announce it as a breaking change, even if for the vast majority of people no reaction would be required.

[hakenr]

@SteveSandersonMS As you are touching the CurrentValueAsString property and thinking about the best design, let me remind you of this issue #44105 that might affect your current design decision. I think we should shape the solution of this issue in a direction that keeps in mind the need for future asynchronous handling of input's onchange event.

[SteveSandersonMS]

As you are touching the CurrentValueAsString property and thinking about the best design, let me remind you of this issue #44105 that might affect your current design decision

Thanks for pointing that out. Looking into it, I think we can treat that as a separate enhancement. Whether or not we do that (and I suspect we will), it doesn't stop this PR being valid in its own right. All the test cases being added here, for example, would remain valid. Since #44105 will require some new design, I think we should address that on its own. We will know whether or not any design for #44105 still works with the requirements for this PR based on whether the E2E tests added here still pass. So in summary, I don't think we need to this PR on solving #44015.

[javiercn]

Side note since you mentioned #44105

Do you think it's reasonable to move the logic for invoking ValueChanged as well as EditContext.NotifyFieldChanged to the event handler? (Essentially manually hand-coding @bind:after for the input element binding)

That was my original idea/direction for the issue, but it is breaking-ish. That said, since we are doing a somewhat breaking change here, I think it would be a good idea to tackle both things at the same time (not in this PR) if my suggestion is something we want to follow.

[SteveSandersonMS]

That sounds like a similar suggestion to the one raised at #46434 (comment)

I'm definitely open to doing that but (as per this) would rather keep it separate.

[SteveSandersonMS]

Essentially manually hand-coding @Bind:after for the input element binding

Ideally we might even be able to just use @Bind:get/@Bind:set and change the Input* components to be written as .razor files. I started doing that but it derailed the PR and introduced too many independent changes so I think we should tackle it separately.

[akorchev]

@SteveSandersonMS how would one implement the equivalent of SetUpdatesAttributeName when using .razor files? I just discovered that method today why investigating an issue. It turns out this is what I need but I don't know how to use SetUpdatesAttributeName in a razor file. Is there any way or should I abandon the razor file and use BuildRenderTree?

[hakenr]

AFAIK, <html-element @bind-Value="..." /> in Razor generates the SetUpdatesAttributeName() as part of generated BuildRenderTree() method.

[akorchev]

Hi @hakenr! I will check if this addresses the issue.

[javiercn]

I was previously advocating a risk-averse approach here, but on further consideration I am now siding with the pro-risk option because I don't want to have multiple different kinds of "current value" that component authors would have to understand and distinguish.

I agree, let's do the "right" thing since we are early in the release.

[javiercn]

Looks great!

[SteveSandersonMS]

Thanks again, @hakenr! I hope you don't mind, but our branch rules require PRs to be squash-committed, so I had no choice but to do that, which is why your original commits don't show as separate commit entries in the main branch.

[hakenr]

Hi @SteveSandersonMS, no problem with that. My intention was to help make Blazor better, not to hunt for position in any contribution leaderboard.

(Consider updating your workflow policies and start using Co-authored-by: in such cases ;-). See GitHub: Commit together with co-authors.)

[ghost]

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