elmish / react

Elmish React extensions for writing SPA and Native apps
https://elmish.github.io/react
Other
104 stars 21 forks source link

Jumping cursor in input elements glitch #12

Closed alfonsogarciacaro closed 6 years ago

alfonsogarciacaro commented 6 years ago

This has been reported in other places (for example, see https://github.com/fable-elmish/react/pull/10#issuecomment-347731283), 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:

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.

jumping

et1975 commented 6 years ago

Another workaround is to use Ref to update the value after the element has been rendered. The 1.0.1 release of Fable.Elmish.React adds valueOrDefault function that can be used to set the value that way.

et1975 commented 6 years ago

Still an open question: RAF should make no difference at the rate of human typing and yet it does? So what's different?

alfonsogarciacaro commented 6 years ago

My gut feeling is somehow React controlled form elements need onChange and value edits are connected synchronously and the requestAnimationFrame callback breaks that, no matter how short the time is it, but that's just a wild guess 🤔

et1975 commented 6 years ago

I think we're OK to close this?

alfonsogarciacaro commented 6 years ago

Yes, sorry. Let's close it!

alfonsogarciacaro commented 6 years ago

@et1975 I know we had this settled but @vbfox reasoning in https://github.com/fable-elmish/elmish/pull/131#discussion_r164292811 makes lot of sense.

Essentially i'd love to see withReactUnoptimized become withReact and withReact to become withReactAnimationFrameOptimized that would have a comment listing the consequences.

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?

et1975 commented 6 years ago

The range of applications that need anything that update so fast is small

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.

et1975 commented 6 years ago

@vbfox

They currently work in Elmish because MailboxProcessor is executing Post synchronously

How do you figure?

alfonsogarciacaro commented 6 years ago

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 Post is running synchronously. I guess @vbfox is talking about that.

There's only a setTimeout triggered when you run async recursively more than 2000 times to prevent stack overflows.

vbfox commented 6 years ago

How do you figure?

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).

vbfox commented 6 years ago

See REPL for example : [MailBox](http://fable.io/repl/#?code=open%20Fable.Core%0Aopen%20Fable.Core.JsInterop%0A%0Alet%20inbox%20%3D%20MailboxProcessor.Start(fun%20(mb%3AMailboxProcessor%3Cstring%3E)%20-%3E%0A%20%20%20%20let%20rec%20loop%20(state%3Aunit)%20%3D%0A%20%20%20%20%20%20%20%20async%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20let!%20msg%20%3D%20mb.Receive()%0A%20%20%20%20%20%20%20%20%20%20%20%20printfn%20%22Very%20async%20receive%20%25s%22%20msg%0A%20%20%20%20%20%20%20%20%20%20%20%20return!%20loop%20()%0A%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20loop%20())%0A%0Aprintfn%20%22BEFORE%22%0Ainbox.Post(%22a%22)%0Ainbox.Post(%22b%22)%0Aprintfn%20%22AFTER%22&html=%3Chtml%3E%0A%20%20%20%20%3Chead%3E%0A%20%20%20%20%20%20%20%20%3Cmeta%20http-equiv%3D%22Content-Type%22%20content%3D%22text%2Fhtml%3Bcharset%3Dutf-8%22%3E%0A%20%20%20%20%3C%2Fhead%3E%0A%20%20%20%20%3Cbody%3E%0A%20%20%20%20%3C%2Fbody%3E%0A%3C%2Fhtml%3E).

et1975 commented 6 years ago

There's only a setTimeout triggered when you run async recursively more than 2000 times to prevent stack overflows.

Exactly! And that's what dispatch loop is doing.

alfonsogarciacaro commented 6 years ago

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 IObservable for Elmish.React. This should be no problem as there're no asynchronous operations running within the dispatch loop at the moment.

vbfox commented 6 years ago

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 msgcount%2000=0

It would be fun to test with a button to pre-send 1999 messages :)

rkosafo commented 6 years ago

Hi. From which version is this fixed in because I still experience this. How do I check that it is happening after 2000 iterations since it seem to happen all the time for me? Any workaround?

alfonsogarciacaro commented 6 years ago

Hi @rkosafo. It's not exactly "fixed", but you should be able to work around it by using Program.withReactUnoptimized (which doesn't use the requestAnimationFrame optimization) instead Program.withReact.

et1975 commented 6 years ago

@rkosafo I'd recommend usingvalueOrDefault helper intead of running unoptimized if you are concerned with performance.

rkosafo commented 6 years ago

@alfonsogarciacaro I only see Program.withReact. I cannot see withReactUnoptimized. I'm using Elmish.React 1.0.0. I'll upgrade to 1.0.3 and try again. Is that the correct version?

@et1975 I'm using custom react components and well as local input. For example, I'm using SemanticUi.React and the code below is an example of a helper function. In this case, how do I use valueOrDefault since it is external. Performance is a major concern.

  sui2' Input.Input
    { createEmpty with
        placeholder = placeholder
        value = value
        onChange = f2 (fun _ x -> onChange <| unbox<string> x.value) } []

Changing to PReact fixes the issue but breaks popups with SemanticUi so it is not an option.

MangelMaxime commented 6 years ago

@rkosafo Implement your own version of valueOrDefault if this is needed

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 ]
xdaDaveShaw commented 5 years ago

I'm struggling to get this working on my projects. I'm using:

(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...

alfonsogarciacaro commented 5 years ago

@xdaDaveShaw Sorry, what do you mean? You still get the jumping glitch when using valueOrDefault? Have you tried to use withReactUnoptimized?

xdaDaveShaw commented 5 years ago

Sorry, it is the jumping glitch, but...

When I use the Elmish > Simple input sample in the REPL it doesn't jump. That example doesn't use valueOrDefault or withReactUnoptimized

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.

alfonsogarciacaro commented 5 years ago

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 syncDispatch fix this?

xdaDaveShaw commented 5 years ago

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 valueOrDefault you find one here.

It doesn't look like syncDispatch fixes this - I've just upgraded to Fable.Elmish 3.0.2-beta and the problem still happens without a workaround.

chadunit commented 5 years ago

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?

MangelMaxime commented 5 years ago

The REPL use a custom version of Elmish.React and seems like @alfonsogarciacaro implemented by default the equivalent version of withReactUnoptimized.

Source: https://github.com/fable-compiler/repl/blob/master/src/Lib/Elmish.React.fs

alfonsogarciacaro commented 5 years ago

@chadunit You need to convince @et1975 for that ;) We've discussed this several times, but users' feedback seems to show that applying an optimization (requestAnimationFrame) that has unexpected effects by default may not be the best option.

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 (withReact, withReactUnoptimized) and use names that convey more info?

/// 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 withReactUnoptimized was synchronous depended on Fable's implementation of MailboxProcessor. However, now with the new ring buffer the renders are guaranteed to happen synchronously after the update, so the name fits very well. Although we can also use another option like withReactImmediate.

et1975 commented 5 years ago

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.

chadunit commented 5 years ago

In my opinion the default behavior of the library, using standard documented "getting-started" options, should be to have a working Value prop, period, irrespective of any performance or optimization considerations or whether it's React's fault or Elmish's fault or everybody's fault or nobody's fault.

Has it ever been considered to somehow special-case Value into applying valueOrDefault automatically behind the scenes rather than making the user do it manually (assuming this is possible with how the libraries work)? Perhaps conditionally behind withReactBatched if concerned about doing it globally. And maybe add ValueRaw (or some better name) for the current untouched behavior in case that's needed.

alfonsogarciacaro commented 5 years ago

@chadunit Hmm, not sure how easy would be to patch Fable.React Value (note it's a different library). Shadowing it's possible but it depends on how user puts the open statements. Also it may happen Ref is being used for something else.

@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.

cmeeren commented 5 years ago

Just want to weigh in that I agree with @chadunit: IMHO the default behaviour should be to make Value work correctly at the expense of optimizations for a >60Hz update cycle, allowing the user to choose the (well-commented/documented) optimization if needed (e.g. change withReact and withReactUnoptimized to withReactAnimationFrameOptimized and withReact, respectively, as previously suggested).

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.

forki commented 4 years ago

which mode is withReactHydrate using? Is it similar to withReactSynchronous?

et1975 commented 4 years ago

it's sync

forki commented 4 years ago

thanks