-
Notifications
You must be signed in to change notification settings - Fork 21
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
Jumping cursor in input elements glitch #12
Comments
Another workaround is to use |
Still an open question: RAF should make no difference at the rate of human typing and yet it does? So what's different? |
My gut feeling is somehow React controlled form elements need |
I think we're OK to close this? |
Yes, sorry. Let's close it! |
@et1975 I know we had this settled but @vbfox reasoning in elmish/elmish#131 (comment) makes lot of sense.
As there'll be breaking changes for next version of Elmish, would you consider implementing the proposal above for next version of Fable.Elmish.React? |
I don't know if I agree with that assessment - anything that generates updates faster than a certain (user-specific!) rate will freeze the UI, I've seen it happen with websocket messages, when displaying complex graphs and when reconstructing some state by replay. Combine some of those in one app and you're guaranteed a freeze up at some point. However, since we're breaking things anyway, I'd be ok with the renames. |
How do you figure? |
Fable's implementation of Async is just callback nesting so it will actually run synchronously unless you call an asynchronous API (JS is single-threaded so you cannot do anything asynchronous by yourself), so it's true that in this case
|
First I guessed because if it wasn't it wouldn't work as far as I understand react (Cursor would always jump). I also took a react sample, added a setTimeout, saw that it failed (to verify, I had already experienced it but needed to check that it was still happening in 16 with all fiber related changes) Then I checked to see if there was anything in the path doing RAF or setTimeout (JS being single threaded there isn't a lot of possibilities here) and I didn't found it (Seem that I missed that hijack was doing that as @alfonsogarciacaro points out). |
See REPL for example : MailBox. |
Exactly! And that's what dispatch loop is doing. |
Ah, yes! Well, it will only happen every 2000 iterations, that's probably why we haven't noticed. Still, not a good thing having a glitch that may happen every 2000 other messages (if the user happens to be editing in the middle of a message or using asian characters at that point). An alternative could be to use a synchronous implementation of |
Ah didn't realize that the consequence is a bug that only happens if the user is typing in the middle of a text field when It would be fun to test with a button to pre-send 1999 messages :) |
Hi. |
Hi @rkosafo. It's not exactly "fixed", but you should be able to work around it by using |
@rkosafo I'd recommend using |
@alfonsogarciacaro I only see @et1975 I'm using custom react components and well as local input. For example, I'm using
Changing to |
@rkosafo Implement your own version of For example, for Fulma input I added to my project: module Input =
let inline valueOrDefault value =
Input.Ref <| (fun e -> if e |> isNull |> not && !!e?value <> !!value then e?value <- !!value) Usage: Input.text [ Input.Disabled false
Input.OnChange (getValue >> ChangeName >> dispatch)
Input.valueOrDefault formData.Name
Input.Disabled formData.IsWaitingServerReply
inputColor "name" formData.Errors ] |
I'm struggling to get this working on my projects.
(Generally the latest Fable/Elmish and the same js-deps as the Repl) I've no idea what else to try, I'm using the code from the basic Reset Repl state. The Repl works fine, but running that code locally (both dev and prod builds) has this issue still. I can post a repo with the code in if that helps... |
@xdaDaveShaw Sorry, what do you mean? You still get the jumping glitch when using |
Sorry, it is the jumping glitch, but... When I use the Elmish > Simple input sample in the REPL it doesn't jump. But using the same exact same code locally, I see it jumping. My initial thought was that I was running something out of date locally, but AFAICT I have all my .NET and JS deps up to date now and it still happens. I'm at work now, so don't have the code running locally. |
I don't remember exactly but the libraries in the REPL have a couple of hacks so you should take the results with a grain of salt. Please follow the advice in this thread to prevent the jumping glitch with the "actual" libraries :) BTW @et1975 Can the new |
Yeah, both workarounds work just fine - I'd just assumed as the issue was closed and things worked in the REPL that I wouldn't need to use either or them. For anyone else looking for a non-Fulma It doesn't look like |
If this is fixed at the library level in the REPL, could that be ported to the published libraries to reduce need for user workarounds? |
The REPL use a custom version of Elmish.React and seems like @alfonsogarciacaro implemented by default the equivalent version of Source: https://github.com/fable-compiler/repl/blob/master/src/Lib/Elmish.React.fs |
@chadunit You need to convince @et1975 for that ;) We've discussed this several times, but users' feedback seems to show that applying an optimization ( I know people like sane defaults, but when there are different opinions on what these defaults should be, the best option is to have clear semantics in the API. Maybe the release of Elmish 3 is an opportunity to mark the current methods ( /// Uses `requestAnimationFrame` to batch updates if they are happening
/// faster than the current frame rate
let withReactBatched = (* same as current `withReact` *)
/// New renders are triggered immediately after an update
let withReactSynchronous = (* same as current `withReactUnoptimized` *) The best part is that, before Elmish 3, the fact that |
I still think we don't really understand the problem - it works with Preact correctly, so you can't in all honesty assume that its due to semantics, aka "that's how it's supposed to be", but I like the naming. |
In my opinion the default behavior of the library, using standard documented "getting-started" options, should be to have a working Has it ever been considered to somehow special-case |
@chadunit Hmm, not sure how easy would be to patch Fable.React @et1975 I think React and Preact just behave differently so it's possible Preact doesn't need synchronous renders in controlled inputs. In any case, I have sent a proposal for the name change #31, please have a look and give your feedback. |
Just want to weigh in that I agree with @chadunit: IMHO the default behaviour should be to make I may be biased from my own usage, but I would expect there to be far more Elmish users that need correct cursor/caret behavior in input fields than there are Elmish users that have >60Hz update cycles. |
* Probably needs some refactor to something more generic Change to use ValueOrDefault on inputs * Just value caused cursor jumps due to async rendering, see fable issue elmish/react#12
which mode is withReactHydrate using? Is it similar to withReactSynchronous? |
it's sync |
thanks |
This has been reported in other places (for example, see #10 (comment)), but I'm posting this to sum up our findings so far.
When using an
input
element with elmish-react, if you move the cursor to the middle of the text, after entering a character the cursor jumps to the end of the text. This is obviously annoying to the user and also prevents the use of Asian characters (CJK with IME). So far we've found two possible solutions:DefaultValue
instead ofValue
in the input element. Unfortunately this is not intuitive to users and I'm not sure if there're other problems (once I saw a React warning asking to usevalue
but I cannot reproduce now).requestAnimationFrame
optimization. The drawback is this will likely reduce performance in scenarios with too frequent updates (e.g. a websocket pushing updates at higher rate than 60FPS).We need to investigate the issue to understand the cause of the problem and see if there's a better solution than the two above.
The text was updated successfully, but these errors were encountered: