For two-way bindings, enforce consistency between .NET model and DOM by patching old tree. Fixes #8204 #11438

SteveSandersonMS · 2019-06-21T06:47:59Z

This is the rather nontrivial solution to a multitude of related async-binding and diff-inconsistency issues. As of this change, two-way bindings have a stricter behavior whereby they:

Find out from the JS code what the latest state in the DOM is, and apply that to the old render tree
Consequently, produce a diff output that may be smaller (no need to tell the client to perform changes it has already performed, which fixes the laggy typing thing), or may be larger (maybe we need to tell the client to revert its change, which fixes the "moving checkboxes" case we had reported by a customer)

In most ways this is purely advantageous. Various cases where the behavior was bizarre or broken before are now fixed (e.g., binding a checkbox to a property whose value is fixed as true or false before didn't reflect that in the UI - now the checkbox will forcibly revert itself to "checked" or "unchecked" as appropriate"). Also it's much safer to revert invalid edits to number fields, because now the user can see exactly how their input is being interpreted - previously they had no way to know if their edit was being ignored because it was considered invalid.

However the extra strictness will make developers go through extra hoops in a few cases. The most notable one is textboxes bound to non-nullable int/double/etc that use bind-value:event="oninput". These will no longer allow unparseable values such as being empty or just containing -, so developers will have to either:

Not use bind-value:event="oninput" for non-nullable numbers, since you don't want to prevent the invalid intermediate states. Just use the default change event which is "change".
Or, use a nullable version of your number, or even bind to a string so you can do whatever you want with the invalid intermediate states
Or, use <InputNumber> which already handles this nicely. In the future we might extend that to support bind-value:event="oninput", and it will do what you expect because behind the scenes it's binding to a string and handling parsing separately so it can record validation errors.

SteveSandersonMS · 2019-06-21T06:49:01Z

@dougbu This PR contains some of the commits from dotnet-maestro, because I'm waiting for #11307. As long as the build for this PR passes, I think it should be fine to merge those commits as part of this PR. Or if #11307 gets merged first, it will make no difference after being rebased.

SteveSandersonMS · 2019-06-21T14:40:43Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=233678

aspnet-hello · 2019-06-21T14:42:20Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2707

dougbu · 2019-06-22T04:15:40Z

@SteveSandersonMS I see you force pushed the branch and no dependency updates remain. Do you still need anything from me here?

SteveSandersonMS · 2019-06-22T05:53:55Z

@dougbu No, nothing else needed. Unless you’re part of the BRT or whatever the current equivalent to that is, because there is still https://github.com/aspnet/AspNetCore-Internal/issues/2707

SteveSandersonMS · 2019-06-24T12:52:16Z

@aspnet-hello Triage https://dev.azure.com/dnceng/public/_build/results?buildId=235994&view=logs&jobId=1bca1137-cedc-5e44-af9b-a27ff406bb70

javiercn · 2019-06-24T18:12:19Z

src/Components/Components/src/Rendering/RenderTreeUpdater.cs

+        {
+            // We only allow the client to supply string or bool currently, since those are the only kinds of
+            // values we output on attributes that go to the client
+            if (!(newFieldValue is string || newFieldValue is bool))


In which case can this not be either an int or bool, wouldn't that signal that something went terribly wrong? (Like could a number get in here somehow?)

Good point.

I'm not aware of any valid way that the value here might be something other than string/bool. If it was, we could just throw. Maybe that would be clearer.

Looking at your tests, it's fine if this simply returns as the value comes from the client and is untrusted

This might help a bit - #11517

javiercn · 2019-06-24T18:26:25Z

src/Components/Components/src/Rendering/Renderer.cs

        /// <returns>
        /// A <see cref="Task"/> which will complete once all asynchronous processing related to the event
        /// has completed.
        /// </returns>
-        public virtual Task DispatchEventAsync(int eventHandlerId, UIEventArgs eventArgs)
+        public virtual Task DispatchEventAsync(int eventHandlerId, EventFieldInfo fieldInfo, UIEventArgs eventArgs)


Is the expectation that this chain grows bigger than 1 when there are rapidly firing events?

The chain of replaced event handler IDs? If so, yes, it can grow beyond 1.

javiercn · 2019-06-24T18:50:11Z

src/Components/Components/test/RendererTest.cs

+                    Assert.NotEqual(0, attributeFrame.AttributeEventHandlerId);
+                    Assert.NotEqual(eventHandlerId, attributeFrame.AttributeEventHandlerId);
+                });
+            }


Would this for example be a case where there are multiple events fired with the old event handler (like typing fast or moving the cursor quick) and the render batches have not been produced yet?

Yes, exactly!

javiercn

Great set of changes.

I didn't get to follow all the details because its very low-level and likely requires some debugging to understand it fully, but the changes look solid and the test coverage is appropriate.

Great job!

…by patching old tree. Fixes #8204

pranavkm · 2019-06-24T22:00:00Z

src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs

+    {
+        public EventFieldInfo() { }
+        public int ComponentId { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
+        public object FieldValue { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }


Filed #11517 in case we have interest in doing the work here.

pranavkm · 2019-06-24T22:00:37Z

src/Components/Components/src/EventCallbackFactoryBinderExtensions.cs

-                // to process invalid input but avoid dirtying the state of the component if can't be converted. Imagine if
-                // we assigned default(T) on failure - this would result in trouncing the user's typed in value.
+                // We only invoke the setter if the conversion didn't throw, or if the newly-entered value is empty.
+                // If the user entered some non-empty value we couldn't parse, we leave the state of the .NET field


pranavkm · 2019-06-24T22:05:04Z

src/Components/Components/src/Rendering/RenderTreeUpdater.cs

+        {
+            // We only allow the client to supply string or bool currently, since those are the only kinds of
+            // values we output on attributes that go to the client
+            if (!(newFieldValue is string || newFieldValue is bool))


This might help a bit - #11517

SteveSandersonMS requested a review from dougbu as a code owner June 21, 2019 06:47

SteveSandersonMS requested review from javiercn, pranavkm and rynowak June 21, 2019 06:49

SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Jun 21, 2019

SteveSandersonMS added this to the 3.0.0-preview7 milestone Jun 21, 2019

SteveSandersonMS force-pushed the stevesa/fix-tree-mutations-during-events branch 2 times, most recently from 5547c3c to 57ff22d Compare June 21, 2019 14:02

SteveSandersonMS force-pushed the stevesa/fix-tree-mutations-during-events branch from 57ff22d to fbffba2 Compare June 21, 2019 14:50

SteveSandersonMS force-pushed the stevesa/fix-tree-mutations-during-events branch from fbffba2 to 6d88845 Compare June 24, 2019 09:10

javiercn reviewed Jun 24, 2019

View reviewed changes

javiercn approved these changes Jun 24, 2019

View reviewed changes

SteveSandersonMS added 3 commits June 24, 2019 20:02

For two-way bindings, enforce consistency between .NET model and DOM …

cc7320f

…by patching old tree. Fixes #8204

Refresh .js files following rebase

aa8c166

Refresh .js files following rebase AGAIN

1335f41

SteveSandersonMS force-pushed the stevesa/fix-tree-mutations-during-events branch from 6d88845 to 1335f41 Compare June 24, 2019 19:07

Regenerate AGAIN

ae02416

pranavkm approved these changes Jun 24, 2019

View reviewed changes

SteveSandersonMS merged commit f162ba1 into master Jun 24, 2019

SteveSandersonMS deleted the stevesa/fix-tree-mutations-during-events branch June 24, 2019 22:15

SteveSandersonMS mentioned this pull request Jun 26, 2019

Verify fast typing and bind parsing scenarios #11586

Closed

SteveSandersonMS mentioned this pull request Sep 23, 2019

Poor typing experience in Blazor Server app when manually binding to oninput event and using Azure SignalR Services #14242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For two-way bindings, enforce consistency between .NET model and DOM by patching old tree. Fixes #8204 #11438

For two-way bindings, enforce consistency between .NET model and DOM by patching old tree. Fixes #8204 #11438

SteveSandersonMS commented Jun 21, 2019

SteveSandersonMS commented Jun 21, 2019

SteveSandersonMS commented Jun 21, 2019

aspnet-hello commented Jun 21, 2019

dougbu commented Jun 22, 2019

SteveSandersonMS commented Jun 22, 2019

SteveSandersonMS commented Jun 24, 2019

javiercn Jun 24, 2019

SteveSandersonMS Jun 24, 2019

javiercn Jun 24, 2019

pranavkm Jun 24, 2019

javiercn Jun 24, 2019

SteveSandersonMS Jun 24, 2019

javiercn Jun 24, 2019

SteveSandersonMS Jun 24, 2019

javiercn left a comment

pranavkm Jun 24, 2019

pranavkm Jun 24, 2019

pranavkm Jun 24, 2019

For two-way bindings, enforce consistency between .NET model and DOM by patching old tree. Fixes #8204 #11438

For two-way bindings, enforce consistency between .NET model and DOM by patching old tree. Fixes #8204 #11438

Conversation

SteveSandersonMS commented Jun 21, 2019

SteveSandersonMS commented Jun 21, 2019

SteveSandersonMS commented Jun 21, 2019

aspnet-hello commented Jun 21, 2019

dougbu commented Jun 22, 2019

SteveSandersonMS commented Jun 22, 2019

SteveSandersonMS commented Jun 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment