Skip to content
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

Components bind-value-oninput can lose cursor position with fast typing #8204

Closed
SteveSandersonMS opened this issue Mar 5, 2019 · 9 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Mar 5, 2019

Repro:

  • <input bind-value-oninput="@SomeString" />
  • Type X into the input
  • Position the cursor at the start (before the X)
  • Mash the keyboard

Expected: Keystrokes typed should appear only at the cursor position, so the X should remain at the end

Actual: A few keystrokes appear before the X, but then the cursor jumps to the end, and subsequent keystrokes are placed after the X

I think this happens because of how bind does a 2-way binding. On each keystroke, it not only (1) reads the updated value and writes it to SomeString, but also (2) tries to re-apply the value from SomeString to the input. When it does (2), as long as the value in the input already matches the string, it's a no-op and gets skipped, so normally we don't do anything that moves the cursor.

However, because of the asynchrony with Razor Components, there might be more keystrokes between (1) and (2), so the value we're applying in (2) no longer matches the contents of the input. We don't lose the keystrokes (we already did some work on that), but the fact that input.value !== SomeString means we do write both the previous and then current value to the input. The fact that we're no longer no-opping this write means now the cursor implicitly moves to the end.

Suggested fix: In BrowserRenderer.ts, in tryApplyValueProperty, we should add more logic to preserve the selectionStart/selectionEnd properties of input/textarea when assigning value. This would also improve things in other scenarios, like if you had a collaborative textarea bound to a value that multiple users could edit simultaneously.

@SteveSandersonMS SteveSandersonMS added this to Triage in Blazor via automation Mar 5, 2019
@SteveSandersonMS SteveSandersonMS added bug This issue describes a behavior which is not expected - a bug. area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-blazor Includes: Blazor, Razor Components labels Mar 5, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview5 milestone Mar 6, 2019
@danroth27 danroth27 moved this from Triage to To do in Blazor Mar 11, 2019
@rynowak rynowak removed this from To do in Blazor Mar 16, 2019
@mkArtakMSFT mkArtakMSFT added this to To do in Blazor Apr 15, 2019
@mkArtakMSFT mkArtakMSFT removed this from To do in Blazor Apr 17, 2019
@mkArtakMSFT mkArtakMSFT added this to To do in Blazor May 1, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented May 20, 2019

This issue was assigned to me so I would write up some thoughts on resolution, not with the expectation of a fix in preview 6. So here are my thoughts.

@javiercn has already written up some thoughts on the general issue, but in this post I want summarise it from the point of view of typing, since that's probably the worst case.

The problem

This is a general problem affecting any case where the DOM can get out of sync with the underlying rendertree (e.g., because the user modified the state of a form input).

But it's worse for server-side Blazor, because given the async-ness, there are extra opportunities to be out of sync due to time spent in flight. Common example which is at the root of this issue here:

  1. There's an <input bind-value-oninput=@MyProperty /> where MyProperty is just a normal { get; set; } property.
  2. User presses "a"; client sends "a" to server
  3. User presses"b"; client sends "ab" to server
  4. Server receives "a" (from step 2), diffs the UI and sees the change, so replies with a message to update the textbox contents with "a". Client receives this an updates the textbox to "a".
  5. User presses "c"; client sends "ac" to server.
  6. Server receives "ab" (from step 3). Again it responds and makes the client update the textbox value to "ab".
  7. Server receives "ac" (from step 5). Again it responds and makes the client update the textbox value to "ac".

So not only does it flicker ("ac" then "ab" then "ac"), it entirely lost the "b" keystroke.

The solution

I think there are two major different approches we could take: one-way bindings, or correcting the render tree.

One-way binding

For any form element where you don't want to have custom logic that mutates the state, you only want the state to be mutated directly by UI events. It makes no sense to have the server keep sending back instructions to update the form field state when it should already be in the correct state.

If it was possible to do a one-way binding (client-to-server, or you might say JS-to-.NET), the problem would disappear in the typical typing case.

  • Pro: completely obvious and bulletproof
  • Con: is it a new syntax? Do we default to different one-way/two-way settings depending on what type of thing you're binding?
  • Con: may lead to bugs, e.g., if people try to change the entire "model" object associated with a form, it would suck if only some of the UI elements updated to reflect the new model's data

Overall I don't think this is a good solution, mostly because it's nasty that developers would have to start making decisions about this for every form field. I'm only writing it up here because a while ago I speculated it might be the right solution, and I no longer think so.

Correcting the render tree

This is basically @javiercn's original idea with a few tweaks.

Basically, we have some specialised behavior for the known primitive form elements. When an event comes in, the JavaScript-side code can optionally attach some info to describe what the latest state of the associated DOM element is (e.g., checkbox checkedness, text field text value). Then the .NET-side code uses the event handler ID to find the corresponding element data in the previous rendertree, and uses the incoming info to actually mutate the state of that old tree to represent the actual state in the UI now.

Then, when the diff algo does its thing, it will automatically determine whether the UI element is already in the right state (which for normally-bound text fields, checkboxes, etc. it will be) and in that case won't even issue instructions to update it. But if somehow we find it's not in the right state now (e.g., custom setter logic), then the diff will produce an instruction to update the UI.

  • Pro: as well as fixing things in common cases, it makes us use bandwidth more efficiently for free
  • Con: nontrivial to implement. Not even certain how we "find the corresponding element data in the previous rendertree" in the case where it's on a different component than the one raising the event. It's probably possible but might involve tracking more data alongside event handlers.
  • Con: there's a more general case where this still isn't enough (but this more general case is beyond any automatic solution - see below)

Overall, this sounds promising enough at least to be worth prototyping.

But we still can't escape the laws of mathematics

If the server has custom logic that is meant to modify the form field state, and the updates occur asynchronously, there can be cases where it's impossible to know how to reconcile the changes. Example:

  1. There's an <input bind-value-oninput=@MyProperty />, where MyProperty has a custom setter that always appends 2 to whatever value you're trying to set.
  2. User types "a"; client sends "a" to server
  3. User types "b"; client sends "ab" to server
  4. Server replies with "a2" (in response to 2); client replaces textbox contents with "a2"
  5. User types "c"; client sends "a2c" to server.
  6. Server replies with "ab2" (in response to 3); client replaces textbox contents with "ab2"
  7. Server replies with "a2c2" (in response to 5); client replaces textbox contents with "a2c2"

So again, the b keystroke was lost. But it's worse now. What should the outcome from step 3 actually be? Somehow we have to merge two changes: the user says the value should be ab, and the server says it should be a2. Does the client combine them to produce a2b or ab2, or something else? It's a classic merge conflict. The "right" merge depends on application-specific rules about what sorts of values are preferred for this specific data.

I'm extremely unkeen on introducing APIs for resolving merge conflicts. For our initial release at least, I'd rather either:

  • Tell people to just not create UIs like this where you mutate the textbox state while a user is typing. Fundamentally it's not a good fit for an asynchronous remote application.
  • Or, if they must, have some means of blocking user input while event processing is in progress. Typing in this textbox won't be fun, so you'd only do it in very unusual cases. Things will stay consistent though.

Also cc @rynowak for his input.

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented May 20, 2019

Unassigning myself and sending on to preview 7.

@rynowak
Copy link
Member

rynowak commented May 30, 2019

Tell people to just not create UIs like this where you mutate the textbox state while a user is typing. Fundamentally it's not a good fit for an asynchronous remote application.

I think we have to do this for server-side blazor. With the amount of time we have left in the release, we have to look at the minimal viable solution for each issue unless it's going to prevent us from doing something better in the future.

@javiercn
Copy link
Member

Unifying all the related issues into this one
#8202

@mkArtakMSFT mkArtakMSFT added this to To Do in Blazor Jun 7, 2019
@SteveSandersonMS SteveSandersonMS moved this from To Do to In progress in Blazor Jun 12, 2019
@SteveSandersonMS
Copy link
Member Author

I've put a working proof-of-concept fix for this here: https://github.com/aspnet/AspNetCore/compare/stevesa/poc-handle-fast-events. It's a combination of Javier's idea about patching the existing render trees, plus a new idea about tracking the chain of replacements for event handler IDs.

This fixes fast typing as well as a whole range of similar "async events" diffing issues. It's hugely beneficial for making server-side Blazor behave like you'd expect in high-latency situations.

Turning this into a production-ready implementation will be 2-3 days work, assuming good test coverage.

@rynowak
Copy link
Member

rynowak commented Jun 13, 2019

That's great news.

@javiercn
Copy link
Member

Turns out you could beat the laws of physics 😄

@SteveSandersonMS
Copy link
Member Author

Turns out you could beat the laws of physics

Unfortunately my solution only addresses the thing that's possible to solve. It doesn't do anything for the "merge conflict" case, which will continue to operate on a last-write-wins basis. This only occurs if you have server-side code that mutates the data the user is editing while they are editing it, so not in default @bind cases.

@javiercn
Copy link
Member

Unfortunately my solution only addresses the thing that's possible to solve. It doesn't do anything for the "merge conflict" case, which will continue to operate on a last-write-wins basis. This only occurs if you have server-side code that mutates the data the user is editing while they are editing it, so not in default @bind cases.

Yeah, I think that in the end we will have to give guidance to people building server-side apps

@rynowak rynowak mentioned this issue Jun 18, 2019
56 tasks
SteveSandersonMS added a commit that referenced this issue Jun 21, 2019
SteveSandersonMS added a commit that referenced this issue Jun 21, 2019
SteveSandersonMS added a commit that referenced this issue Jun 21, 2019
SteveSandersonMS added a commit that referenced this issue Jun 24, 2019
SteveSandersonMS added a commit that referenced this issue Jun 24, 2019
Blazor automation moved this from In progress to Done Jun 24, 2019
@SteveSandersonMS SteveSandersonMS added Done This issue has been fixed and removed Working labels Jun 24, 2019
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 3, 2019
@jaredpar jaredpar removed this from Done in Blazor Jan 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

4 participants