Input components two way binding + InputBase handling of unparsable inputs

[hakenr]

Input components two way binding + InputBase handling of unparsable inputs

Hello @SteveSandersonMS, this PR is derived from PR #40234 and proposes one of the possible solutions where we will remember the value coming from input-element in _currentValueAsString backing field plus keep the _parsingFailed result in another field.

This solution

• satisfices all the tests (old and new ones),
• does not change the API,
  • does not expose the _parsingFailed - we might consider exposing this as protected property,
  • does not expose the _currentValueAsString (visible input-value), except the call to TryParseValueFromString() method, where the derived component can save the input-value for its own usage,

I believe this does not add much complexity to the InputBase implementation, I'm just not sure about naming of the new fields.

[mkArtakMSFT]

Thanks for your PR, @hakenr. @pranavkm can you please review this? Thanks!

[pranavkm]

@SteveSandersonMS this looks like a follow up to https://github.com/dotnet/aspnetcore/pull/40234. I'll let you handle it.

[hakenr]

Hi @SteveSandersonMS, the @msftbot already closed the underlying #40326 for inactivity.

I have no problem with my proposal being rejected as inappropriate and I can try to look for a better solution (if you give me the direction you would like to see). However, I would be sorry if it was discarded for inactivity. ;-)

(BTW: I saw your current work on WASI and I'm really impressed. I know you have lots of more valuable things to do and I don't want to push you. Just want to protect my contribution from being discarded without being seen.)

[SteveSandersonMS]

Thanks @hakenr. I appreciate your contribution here and definitely hope we can merge either this or something similar to it.

Please don't worry about the bot closing #40234. That happened because it has the pr: pending author input label. This PR 40326 isn't pending your input (it's pending ours) so won't be auto-closed, and even the closure of 40234 doesn't really change anything as it's still there for us to refer to while reviewing this one.

The team is pretty focused on MAUI at the moment but we will try to give some clear attention to this PR soon. Hope that's OK!

[hakenr]

@SteveSandersonMS Thank you for the explanation. I look forward to your feedback.

[SteveSandersonMS]

Thanks for submitting this, @hakenr, and apologies it's taken so long to dig into it. Partly that's because we've been implementing some new @bind modifiers, and we were expecting those to impact our range of options here.

On investigation, I do appreciate what you're doing here and recognize how it would make the <Input*> components bind in a way that's more consistent with a regular <input @bind=...>. However it does still appear to be a nontrivial breaking change, in that:

• Anyone who was previously subclassing an <Input*> component might be relying on the idea that CurrentValueAsString would only ever return a valid value, i.e., something that's definitely parseable as the input's value type. This PR would make it possible for that property to give an unparseable value, which might trip up people's existing logic.

To resolve this, we could:

• Put CurrentValueAsString's getter back to what it was before (so it only returns the last good value) but then change the rendering logic so it emits _currentValueAsString if _parsingFailed.

I think that should give the desired effect, in that we retain the existing behavior that unparseable values don't get auto-reverted, and that CurrentValueAsString represents a last-accepted value, and introduces the desired new behavior that if you bind to a custom setter that might revert the change then the reversion is no longer ignored.

Unless I'm missing something, it also looks like we'd need some E2E test updates:

• We should have a test to cover the effects of setting _parsingFailed = false inside the CurrentValue setter. It doesn't look like the existing E2E tests exercise that line at all.
• It would be ideal if most of the new E2E tests didn't create subclasses of the Input* components, but instead worked by binding against a model that has a custom setter, because that's a much more common usage pattern. I think it's possible to represent the desired scenarios that way.

It's rather late to introduce such a change for .NET 7 now we're already locked down on the RC1 release, and it's unlikely we'd be allowed to make such a change in RC2. I know that's mostly our fault for not reviewing this earlier. How satisfactory would it be for you if this went into .NET 8 preview 1?

[hakenr]

@SteveSandersonMS There is no time pressure on my side to have this PR released ASAP, I think the .NET8 is OK. Do you want me to implement the proposed changes and E2E tests (I can find some time in approx. one month) or will you continue on this PR on your own (or someone from your team)?

[SteveSandersonMS]

@hakenr Thanks for understanding!

Do you want me to implement the proposed changes and E2E tests (I can find some time in approx. one month) or will you continue on this PR on your own (or someone from your team)?

Either way is fine with me. If you're able to do it, that will speed things along. If not, this is still definitely a worthwhile improvement so I'll make sure we attempt it early in .NET 8.

We love community contributions so I'd count it as ideal if you're able to do it, but please don't feel pressured.

[hakenr]

It would be ideal if most of the new E2E tests didn't create subclasses of the Input* components, but instead worked by binding against a model that has a custom setter, because that's a much more common usage pattern. I think it's possible to represent the desired scenarios that way.

@SteveSandersonMS Can you please provide me some guidance on this?

I tried to rearrange the InputNumber test this way, but in such case the original issue is not covered (#40097) and the test passes although I remove the SetUpdatesAttributeName() fix. (I think the ".NET model not updating from DOM" issue does not affect the scenario with custom setter.)

<InputNumber @bind-Value="@model.IntValue" id="derived-input-number" />

@code
{
    private class Model
    {
        public int? IntValue
        {
            get => _intValue;
            set
            {
                if (value == 1)
                {
                    _intValue = 0;
                }
                else
                {
                    _intValue = value;
                }
            }
        }
        private int? _intValue;
    }
}

(Shortened for clarity. See the full change in this commit: https://github.com/hakenr/aspnetcore/commit/3962bff6c02c6d08a60d9d1e32506b15f5650d02)

[hakenr]

@SteveSandersonMS

• added some E2E tests regarding the _parsingFailed logic
• moved the logic out of CurrentValueAsString_get (reverted) => had to introduce new protected properties to make the context accessible from rendering code (derived components)

Please review the proposed code (esp. the two new protected properties which extend the PublicAPI).

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

[SteveSandersonMS]

Merged via https://github.com/dotnet/aspnetcore/pull/46434