Input field reverts to old value while db updates

brjann commented 3 years ago

Also posted in Clojureverse https://clojureverse.org/t/input-field-blinks-before-update-in-re-frame/6758

I'm using the re-com/input-text element. The value is dispatched to the db when the input loses focus, which is the default behavior. However, after leaving the input field, the old value is shown for a short time, and then the new value is shown again.

Basically, the code looks like this

(re-frame/reg-sub :test-sub
  (fn [db]
    (:test-sub db)))

(re-frame/reg-event-db :test-event
  (fn [db [_ value]]
    (assoc db :test-sub value)))

(defn main-panel []
  (let [test (re-frame/subscribe [:test-sub])]
    [re-com/v-box
     :height "100%"
     :children [[:span @test]
                [re-com/input-text
                 :model test
                 :on-change #(re-frame/dispatch [:test-event %])]]]))

Can I prevent this behavior somehow? I've made a quick demonstration of the phenomenon based on the re-frame Leiningen template available at https://github.com/brjann/re-frame-test

prook commented 3 years ago

This problem has been bugging me for months now, so I spent last night debugging and playing around with input-text, and I'm quite confident I see what the problem is. IMHO, it's these external-model resets:

https://github.com/day8/re-com/blob/master/src/re_com/misc.cljs#L127 https://github.com/day8/re-com/blob/master/src/re_com/misc.cljs#L134 https://github.com/day8/re-com/blob/master/src/re_com/misc.cljs#L141

The thing with re-frame events is that they are dispatched asynchronously -- they will be handled (very) soon, but not right now. By resetting external-model before calling on-change, the component 1) triggers an update on itself, and 2) makes a false assumption about the outside world state -- appdb will be updated eventually, but for now, it's still at the old value.

Let's replay what happens, in slow motion:

Starting with {:internal-model "" :external-model "" :latest-ext-model ""}, user types "HELLO" in the text-input
And thus: {:internal-model "HELLO" :external-model "" :latest-ext-model ""}
Next, the user leaves the field, triggering on-blur event, which reset!s external-model to internal-model, and dispatches a re-frame event, that will eventually update appdb, so: {:internal-model "HELLO" :external-model "HELLO" :latest-ext-model ""}
That reset! in the previous step triggers input component update, and this condition here is now false, since external-model says "HELLO", but latest-ext-model is still "". So the component resets itself back to the actual external state: {:internal-model "" :external-model "" :latest-ext-model ""}
Just about when everything above has happenned, that dispatched re-frame event is handled, and appdb is updated, triggering yet another update. At this moment, we have {:internal-model "" :external-model "" :latest-ext-model "HELLO"}, and the same condition is false again, which results in another round of resets and updates, finally settling on {:internal-model "HELLO" :external-model "HELLO" :latest-ext-model "HELLO"}

Transition from 3 to 4 to 5 causes that oscillation we see.

Omitting external-model resets I pointed out seems to work just fine - in step 3, we avoid the component update, dispatch a command to update the outside wold, and once the world updates, it will let us know by a subscription change. Only then the component updates its internal state {:internal-model "HELLO" :external-model "" :latest-ext-model "HELLO"} to {:internal-model "HELLO" :external-model "HELLO" :latest-ext-model "HELLO"}. There is still some oscillation involved, but it does not cause a visible flicker this issue is about.

I may be missing something though, so before cooking up a pull request, I'd welcome some feedback -- would removing them resets break something I don't see? I'm rudely pinging @Gregg8, since he seems to be the last one to touch the code in question (although it's been 6 years).

prook commented 3 years ago

Upon further inspection, the resets are there for a reason. Hm...

Edit: The reasons do not hold, imho, see the mention below.

brjann commented 3 years ago

Great that someone else is bothered by this! :-)

Have you been able to fix this bug while preserving the bugfix in #187?

If not, I have been thinking about this and my thought is that there may be an underlying problem, namely when does the input change from showing the (edited) internal value to the (persisted) external value from app-db?

It can't do it immediately on blur, because then we see this blinking behavior.
Rather, it has to wait until the onblur function call, presumably an event dispatch, is finished (very soon)
But how does it now when it is finished? It can't just check if the external value has changed. Because it might not change if the user made an illegal edit and the event dispatch changes it back to its old value. If that would happen, the edited value would become "stuck" and actually not represent the app db value.

Does that make any sense? Or is there a reliable way for the input to detect when "very soon" has happened?

prook commented 3 years ago

Well, I think there was no bug to fix in #187 in re-com, the problem was in the example code itself. It was twisted and wrong, and I've explained it there and sketched out a more fitting solution.

I've implemented a text input component that uses external/internal model mechanism, it works exactly like the re-com one, minus the resets. It works just fine, it updates itself whenever the external model changes (given the model actually changes), and is flicker-free when sending changes to the outer world via on-change.

brjann commented 3 years ago

Wow, great! Are you able to share the source for that component?

prook commented 3 years ago

Just copy and paste the re-com one, and delete those three resets I've pointed out in my first comment here. :)

superstructor commented 3 years ago

Thanks @brjann for the bug report and @prook for the subsequent investigation.

I've tested the proposed change locally with a few different options; e.g. :change-on-blur? both false and true. It appears to work fine.

I agree its a reasonable trade-off since the example in #187 can be written in the way suggested by https://github.com/day8/re-com/issues/187#issuecomment-729825978 vs it is harder to work around the issue described here without a change to the component itself.

brjann commented 3 years ago

It looks like this fix leads to the problem that I predicted above, that if the change is rejected by the DB, the text in the input still does not change. So if I change the text to some illegal value and the dispatched event simply reverts the value back to its old value (i.e., does not update app-db), the value does not change in the text field.

I've updated my demo of the bug to reflect this https://github.com/brjann/re-frame-test

brjann commented 3 years ago

Thanks for reopening @superstructor!

I have an idea on how it could be solved, but it feels very hackish. After the on-blur function is called, a "dummy event" is dispatched with the only purpose of resetting the subscription to the db / external value. The idea is that the dummy event will be handled after the event that is supposedly dispatched in the on-blur function. So then it should be safe to rely on the db / external value.

prook commented 3 years ago

if I change the text to some illegal value and the dispatched event simply reverts the value back to its old value (i.e., does not update app-db), the value does not change in the text field.

But why should it change? To what should it change? To the original app-db value, throwing the user input away? That would be bad, wouldn't it? When the input is invalid, the app should ask the user to fix it, not toss it away and make them start over. :)

brjann commented 3 years ago

Hm, this is a tricky situation. You are right that if the value is reverted, the app should inform the user that something is wrong and not necessarily throw the value away. Is there no conceivable situation where the app should just "silently" revert the value?

prook commented 3 years ago

At the risk of sounding arrogant, I'm afraid this discussion has little to do with the original issue. However, the snippet in https://github.com/day8/re-com/issues/187#issuecomment-729825978 should provide an idea how to trigger a reset/re-render of a component using signal other than model change.

brjann commented 3 years ago

If the input no longer reflecting the value of the subscription can be considered an unwanted effect of the fix, is not the discussion relevant?

brjann commented 3 years ago

Regardless, I can accept that the edited value doesn't change if the edit is reverted, since the app should handle that situation. So I'm fine with closing the issue again @superstructor. Thanks for the fix @prook!

superstructor commented 3 years ago

Thanks @brjann and @prook for your discussion and input on this issue.

Myself and @Gregg8 were not quite ready to give up so I re-opened (again) and we found a solution @brjann

First, let us define the problems.

Problem 1: "Invalid Value Displayed"

If the user types a value that is subsequently modified in :on-change to the prior value of :model, such as validation or filtering, then :model is reset! to the same value as it was prior then the value the user typed (not the filtered value of :model after reset!) will remain displayed to the user as no change is detected. To fix this we force an update via (reset! external-model @internal-model). This was the original bug fixed in #187 and was fixed prior to my revert of that in 27fd1b3

Problem 2: "Flicker Between Values"

The fix to Problem 1 causes this problem. If there is a delay in processing on-change before reset! of :model, such as if on-change is asynchronous, the displayed value will 'flicker' between the prior value of :model before the user had typed anything and the eventual reset! of :model to a new value.

The root cause here is essentially we are forcing a render too early before :model has the new value. We also cannot possibly know how long to wait until the :model reaches a 'steady state'.

Solution

We maintain the fix from #187 as the default behavior. Problem 1 is unacceptable, its clearly a bug, so this fix remains the default.
We provide an 'escape hatch' for developers to optionally solve Problem 2 (while maintaining the fix for Problem 1) by 'signalling' when :model has reached a steady state via a 2-arity variant of on-change. E.g.

:on-change (fn [v done]
                      (js/setTimeout (fn []
                                                  (reset! model v)
                                                  (done))
                       2000))

This means that (reset! external-model @internal-model) (fix for Problem 1) will not be called until after the reset! of :model.

Input text detects the number of arguments received by on-change so you only need to call done if the function is 2-arity. This works for anonymous fns and all other forms of fns.

day8 / re-com