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

Input's onChange Event Handler Misses Key Strokes? #684

Closed
rmorshea opened this issue Feb 22, 2022 Discussed in #668 · 8 comments · Fixed by #694 or #696
Closed

Input's onChange Event Handler Misses Key Strokes? #684

rmorshea opened this issue Feb 22, 2022 Discussed in #668 · 8 comments · Fixed by #694 or #696
Labels
priority-1-high Should be resolved ASAP. type-bug About something that isn't working
Milestone

Comments

@rmorshea
Copy link
Collaborator

Discussed in #668

Originally posted by jkrobicki February 14, 2022
Description
When I fill in the input fast it reacts slowly and miss some letters. In the example I try to write "write here the name" .
name iput

@rmorshea rmorshea added priority-3-low May be resolved one any timeline. type-bug About something that isn't working labels Feb 22, 2022
@rmorshea rmorshea added this to the 1.0 milestone Feb 22, 2022
@rmorshea
Copy link
Collaborator Author

#694 does not appear to have fixed this issue.

@rmorshea rmorshea reopened this Feb 28, 2022
@Archmonger
Copy link
Contributor

My opinion is that if this limitation exists within React, there isn't a major reason to resolve this limitation within v1.

@rmorshea
Copy link
Collaborator Author

rmorshea commented Mar 1, 2022

That's what I thought, but it turns out this is only problematic for React in the case where the onChange handler responds asynchronously. When the handling is sync, then everything works.

@Archmonger
Copy link
Contributor

Archmonger commented Mar 1, 2022

Do we handle this situation correctly if binding a sync IDOM event handler?

Or is it only React that behaves properly under those circumstances?

@rmorshea rmorshea added priority-1-high Should be resolved ASAP. and removed priority-3-low May be resolved one any timeline. labels Mar 2, 2022
@rmorshea
Copy link
Collaborator Author

rmorshea commented Mar 2, 2022

Only React behaves correctly in that circumstance.

The issue is unrelated to the server-side event handler. This has to do with the fact that the server's response to an event is async - the fact that, to trigger a re-render, we have to go to a websocket and asynchronously await a response from the server is analogous to the case where React has problems.

@Archmonger
Copy link
Contributor

Archmonger commented Mar 2, 2022

Would buffering received events within a queue help mitigate this? But we may already be doing this if I recall correctly.

rmorshea added a commit that referenced this issue Mar 2, 2022
To do this we keep a buffer of changed values client-side.
Then, as the server pushes renders to the client, we check
whether the server-rendered value matches the what was put
in the buffer. If it does, then we simply use the latest
value in the buffer and ignore the rendered value. If the
rendered value is different than the rendered value, then
we clear the buffer.
rmorshea added a commit that referenced this issue Mar 2, 2022
Because we handle events asynchronously, we must leave the value
uncontrolled in order to allow all changed committed by the user to be
recorded in the order they occur. If we don't, the user may commit
multiple changes before we render resulting in prior changes being
overwritten by subsequent ones. Instead of controlling the value, we set
it in an effect. This feels a bit hacky, but I can't think of a better
way to work around this problem.
@rmorshea
Copy link
Collaborator Author

rmorshea commented Mar 2, 2022

That wouldn't really solve the problem. We'd have to buffer until the server responded with the next render before sending the next event. That doesn't work though since the server doesn't have to respond to events at all. That is, we don't strictly have to alternate between render, event, render, event, ...

I think I've found a way to solve this a different way though in #696

rmorshea added a commit that referenced this issue Mar 2, 2022
Because we handle events asynchronously, we must leave the value
uncontrolled in order to allow all changed committed by the user to be
recorded in the order they occur. If we don't, the user may commit
multiple changes before we render resulting in prior changes being
overwritten by subsequent ones. Instead of controlling the value, we set
it in an effect. This feels a bit hacky, but I can't think of a better
way to work around this problem.
rmorshea added a commit that referenced this issue Mar 2, 2022
* actually fix #684

Because we handle events asynchronously, we must leave the value
uncontrolled in order to allow all changed committed by the user to be
recorded in the order they occur. If we don't, the user may commit
multiple changes before we render resulting in prior changes being
overwritten by subsequent ones. Instead of controlling the value, we set
it in an effect. This feels a bit hacky, but I can't think of a better
way to work around this problem.

* update changelog
@rmorshea
Copy link
Collaborator Author

rmorshea commented Mar 3, 2022

I'm still observing some weird flicker in Brave, but Chrome and Firefox seem to work fine. Not quite sure why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-1-high Should be resolved ASAP. type-bug About something that isn't working
Projects
None yet
2 participants