Caret moves to the end when editing a TextArea in reagent

[euccastro]

When I render this component:

(defn simple-text-field [text]
  (let [text-state (r/atom text)]
    (fn []
      [rui/text-field
       {:id "example"
        :value @text-state
        :on-change (fn [e] (reset! text-state (.. e -target -value)))}])))

It seems to work well until I place the caret in the middle of the text and type again. Invariably, the caret jumps to the end after the insertion. The root cause seems to be that reagent re-renders components asynchronously. See http://stackoverflow.com/questions/28922275/in-reactjs-why-does-setstate-behave-differently-when-called-synchronously/28922465#28922465

[madvas]

Try it out like this

(defn simple-text-field [text]
  (let [text-state (r/atom text)]
    (fn []
      [rui/text-field
       {:id "example"
        :default-value @text-state
        :on-change (fn [e] (reset! text-state (.. e -target -value)))}])))

[euccastro]

:default-value fixes it! I have no idea why.

I'll close this since I imagine this is an upstream issue and the workaround is simple and painless. But unless this is already documented in a prominent place I've missed, I think this deserves a big warning in the README. The workaround isn't obvious at all to me, and without that the issue is really crippling.

Thank you very much!

[edannenberg]

This is really just a work around, and one that will sooner or later lead to problems as the input becomes uncontrolled. For any serious work controlled inputs are a must IMHO. I remember Reagent had the same issue before this fix. By the looks of it this should also work with Material-UI inputs as the fix just checks for the type property which is correctly set. Did you already investigate why this is not the case?

How do you solve the simple use case of form fields rendering data that arrives over the network, i.e. shortly after the view is already rendered for the first time? Seems like a lot of hoops with uncontrolled inputs.

[madvas]

I haven't really investigated what's situation like in latest reagent with this. But you're right, it's a workaround. I remember having some issues with this back in a day, probably in a scenario you described (when wanted to pre-fill form with delayed data from network). Honestly, right now, I pretty much have no clue what this library could do in order to solve this in any reasonable way (no crazy workarounds).

[edannenberg]

From what I gathered a clean fix is not possible due to the async rendering used by Clojure React wrappers like Om or Reagent. Reagent (after much discussion) resorted to manual caret repositioning. No idea though if this is doable from your library. Maybe some thin wrapper around input components that takes care of this?

Also agreeing with @euccastro that this should be mentioned in the docs.

[madvas]

Okay, thanks for a info. I updated readme. I'll try to look at this in more depth when I find more time.

[superstructor]

I think the problem is that a Material UI textfield is a not a reagent.impl.template/input-component? so it bypasses all of the special handling in reagent-input, input-spec, input-handle-change, input-set-value (which does manual caret repositioning), input-render-setup etc. The condition is based on the name of the component, which in the case of Material UI is not input or textarea.

[superstructor]

This (extremely ugly) workaround works:

(defn textfield []
  (reagent/create-class
    {:display-name   "textfield"
     :reagent-render (fn [props]
                       [rui/text-field props])}))

[madvas]

Oh, really? Thank you a lot for this!!

[madvas]

@superstructor have you actually tried that, because it doesn't really work that way for me. Could you provide example with atom? I assume you made a typo in :display-name and meant to write there either input or textarea.

Anyways here's my piece of code which doesn't work:

(defn text-field []
  (r/create-class
    {:display-name "input"
     :reagent-render (fn [props]
                       [rui/text-field props])}))

(defn simple-text-field [text]
  (let [text-state (r/atom text)]
    (fn []
      [text-field
       {:display-name "input"
        :id "my-textfield"
        :value @text-state
        :on-change (fn [e] (reset! text-state (.. e -target -value)))}])))

But it works when I override checking function in following way:

(set! reagent.impl.template/input-component?
      (fn [x]
        (or (= x "input")
            (= x "textarea")
            (= (reagent.interop/$ x :name) "TextField"))))

[superstructor]

Yes sorry meant to type textarea. Overriding the checking function is probably a better solution ?

[madvas]

Ok pushed changes, this should be resolved now

[edannenberg]

Fix works, but seems to have some side effects. After switching to 0.2.23 text-fields don't update anymore unless I'm actually typing in the field:

[:input {:value (:foo @state)}]
[rui/text-field {:value (:foo @state)}]

With 0.2.22 both fields update if state changes by external means after mounting, with 0.2.23 this is only true for [:input].

[superstructor]

This fix usually works, except when (reagent.interop/$ x :name) is, for example, t instead of TextField with :simple optimisations. Not 100% verified, will comment further when I have verified.

[madvas]

Okay guys, you were both right. @superstructor I resolved issue with optimisations in recently pushed 0.2.24 Hoveever, @edannenberg your issue is far more serious. Yes value can't be changed once set. So basically we're back where we were except we can set :value now instead of :default-value.

I tried to hack around reagent a bit, but couldn't get around this. So I opened issue at reagent here explaining issue and asking for a help.

[superstructor]

Thanks for the fix with optimisations! Looks like the latest release on clojars is still 0.2.23 ?

[radhikalism]

So there are 3 aspects to this bug.

1. The text-field component isn't detected by Reagent as an input component. (The input-component? patch solves this.)
2. When there are updates, Reagent targets the wrong DOM node to set :value.
3. Both of these are caused by the Material text-field's DOM structure being wrapped in a div, whereas Reagent expects find-dom-node to return an input element.

If we were to hack Reagent to workaround 1 and 2 directly, it might look like this:

diff --git a/src/reagent/impl/template.cljs b/src/reagent/impl/template.cljs
index 17e3a9f..1040acc 100644
--- a/src/reagent/impl/template.cljs
+++ b/src/reagent/impl/template.cljs
@@ -116,7 +116,10 @@
     ($! this :cljsInputDirty false)
     (let [rendered-value ($ this :cljsRenderedValue)
           dom-value ($ this :cljsDOMValue)
-          node (find-dom-node this)]
+          node (find-dom-node this)
+          node (if-not (= "INPUT" ($ node :tagName)) ;; Is this a wrapped component?
+                 (.querySelector node "input") ;; Locate a child input inside the wrapper
+                 node)]
       (when (not= rendered-value dom-value)
         (if-not (and (identical? node ($ js/document :activeElement))
                      (has-selection-api? ($ node :type))
@@ -195,9 +198,10 @@
   ($! this :cljsInputLive nil))

 (defn ^boolean input-component? [x]
-  (case x
-    ("input" "textarea") true
-    false))
+  (boolean (or (= x "input")
+               (= x "textarea")
+               (when-let [prop-types ($ x :propTypes)] ;; Is this a wrapped Material input?
+                 (aget prop-types "inputStyle")))))

 (def reagent-input-class nil)

However, this exact patch is probably not the right way to structure a generic library.

What if Reagent provided opportunities through adapt-react-class for users to override how input components are detected and located?

[madvas]

@arbscht thank you very much for your reply. Your patch is against master version of reagent, how would it look like against version 0.6.0? couldn't figure it out somehow

[radhikalism]

@madvas Reagent master has b65afde4 which uses find-dom-node. Not sure if targeting 0.6.0 without find-dom-node will be buggy. Nevertheless, something like this might work:

diff --git a/src/reagent/impl/template.cljs b/src/reagent/impl/template.cljs
index fc6b0c2..a9c13b7 100644
--- a/src/reagent/impl/template.cljs
+++ b/src/reagent/impl/template.cljs
@@ -108,8 +108,16 @@
   [input-type]
   (contains? these-inputs-have-selection-api input-type))

+(defn get-input-node [this]
+  (let [outer-node ($ this :cljsInputElement)
+        inner-node ($ outer-node :input)]
+    (if (and inner-node
+             (= "INPUT" ($ inner-node :tagName)))
+      inner-node
+      outer-node)))
+
 (defn input-set-value [this]
-  (when-some [node ($ this :cljsInputElement)]
+  (when-some [node (get-input-node this)]
     ($! this :cljsInputDirty false)
     (let [rendered-value ($ this :cljsRenderedValue)
           dom-value ($ this :cljsDOMValue)]
@@ -186,9 +194,10 @@
         ($! :ref #($! this :cljsInputElement %1))))))

 (defn ^boolean input-component? [x]
-  (case x
-    ("input" "textarea") true
-    false))
+  (boolean (or (= x "input")
+               (= x "textarea")
+               (when-let [prop-types ($ x :propTypes)] ;; Is this a wrapped Material input?
+                 (aget prop-types "inputStyle")))))

 (def reagent-input-class nil)

I'm not actually sure where :input comes from, but it's there in 0.6.0 and apparently not in master.

[madvas]

Thanks! But it seems like it's not complete solution. I've tried to patch reagent like this:

(set! reagent.impl.template/input-component?
      (fn [x]
        (boolean (or (= x "input")
                     (= x "textarea")
                     (when-let [prop-types ($ x :propTypes)] ;; Is this a wrapped Material input?
                       (aget prop-types "inputStyle"))))))

(defn get-input-node [this]
  (let [outer-node ($ this :cljsInputElement)
        inner-node ($ outer-node :input)]
    (if (and inner-node
             (= "INPUT" ($ inner-node :tagName)))
      inner-node
      outer-node)))

(set! reagent.impl.template/input-set-value
      (fn [this]
        (when-some [node (get-input-node this)]
          ($! this :cljsInputDirty false)
          (let [rendered-value ($ this :cljsRenderedValue)
                dom-value ($ this :cljsDOMValue)]
            (when (not= rendered-value dom-value)
              (if-not (and (identical? node ($ js/document :activeElement))
                           (reagent.impl.template/has-selection-api? ($ node :type))
                           (string? rendered-value)
                           (string? dom-value))
                ;; just set the value, no need to worry about a cursor
                (do
                  ($! this :cljsDOMValue rendered-value)
                  ($! node :value rendered-value))

                ;; Setting "value" (below) moves the cursor position to the
                ;; end which gives the user a jarring experience.
                ;;
                ;; But repositioning the cursor within the text, turns out to
                ;; be quite a challenge because changes in the text can be
                ;; triggered by various events like:
                ;; - a validation function rejecting a user inputted char
                ;; - the user enters a lower case char, but is transformed to
                ;;   upper.
                ;; - the user selects multiple chars and deletes text
                ;; - the user pastes in multiple chars, and some of them are
                ;;   rejected by a validator.
                ;; - the user selects multiple chars and then types in a
                ;;   single new char to repalce them all.
                ;; Coming up with a sane cursor repositioning strategy hasn't
                ;; been easy ALTHOUGH in the end, it kinda fell out nicely,
                ;; and it appears to sanely handle all the cases we could
                ;; think of.
                ;; So this is just a warning. The code below is simple
                ;; enough, but if you are tempted to change it, be aware of
                ;; all the scenarios you have handle.
                (let [node-value ($ node :value)]
                  (if (not= node-value dom-value)
                    ;; IE has not notified us of the change yet, so check again later
                    (batch/do-after-render #(reagent.impl.template/input-set-value this))
                    (let [existing-offset-from-end (- (count node-value)
                                                      ($ node :selectionStart))
                          new-cursor-offset (- (count rendered-value)
                                               existing-offset-from-end)]
                      ($! this :cljsDOMValue rendered-value)
                      ($! node :value rendered-value)
                      ($! node :selectionStart new-cursor-offset)
                      ($! node :selectionEnd new-cursor-offset))))))))))

And when you try 2 inputs like this:

(defn simple-text-field [text]
  (let [text-state (r/atom text)]
    (js/setInterval (fn []
                      (reset! text-state (str (rand-int 99999))))
                    1000)
    (fn []
      [:div
       [rui/text-field
        {:id "my-textfield"
         :value @text-state
         :on-change (fn [e] (reset! text-state (.. e -target -value)))}]
       [:input
        {:id "my-input"
         :value @text-state
         :on-change (fn [e] (reset! text-state (.. e -target -value)))}]])))

you'll see :input value is being correctly changed in intervals, but rui/text-field value doesn't change

[radhikalism]

Be careful with set! — I think it doesn't update all references to the function from callers in closures that were already defined (as the old version of the bound fn appears to be captured/cached/inlined?). To solve, either:

• set! all Vars in the chain that depends on input-set-value (recursively), just so that the correct versions are called all the way in reagent.impl.template.
• Or literally patch template.cljs in a clone of Reagent and depend on that build.

Otherwise it will misbehave as you see.

(Indeed there are other incomplete aspects to this patch. It incorrectly detects radio/check inputs in input-component?. And get-input-node isn't sufficiently nil-safe. But this should be enough to get your test project to work.)

[radhikalism]

@madvas Try this, paste and invoke set-overrides!:

;; A better but idiosyncratic way for reagent to locate the real
;; input element of a component. It may be that the component's
;; top-level (outer) element is an <input> already. Or it may be that
;; the outer element is a wrapper, and the real <input> is somewhere
;; within it (as is the case with Material TextFields).
;; Somehow the `:input` property seems to be pre-populated (in version
;; 0.6.0 of Reagent) with a reference to the real <input>, so we use
;; that if available.
;; At the time of writing, future versions of Reagent will likely
;; change this behavior, and a totally different patch will be
;; required for identifying the real <input> element.
(defn get-input-node [this]
  (let [outer-node ($ this :cljsInputElement)
        inner-node (when (and outer-node (.hasOwnProperty outer-node "input"))
                     ($ outer-node :input))]
    (if (and inner-node
             (= "INPUT" ($ inner-node :tagName)))
      inner-node
      outer-node)))

;; This is the same as reagent.impl.template/input-set-value except
;; that the `node` binding uses our `get-input-node` function. Even
;; the original comments are reproduced below.
(defn input-set-value
  [this]
  (when-some [node (get-input-node this)]
    ($! this :cljsInputDirty false)
    (let [rendered-value ($ this :cljsRenderedValue)
          dom-value      ($ this :cljsDOMValue)]
      (when (not= rendered-value dom-value)
        (if-not (and (identical? node ($ js/document :activeElement))
                  (reagent.impl.template/has-selection-api? ($ node :type))
                  (string? rendered-value)
                  (string? dom-value))
          ;; just set the value, no need to worry about a cursor
          (do
            ($! this :cljsDOMValue rendered-value)
            ($! node :value rendered-value))

          ;; Setting "value" (below) moves the cursor position to the
          ;; end which gives the user a jarring experience.
          ;;
          ;; But repositioning the cursor within the text, turns out to
          ;; be quite a challenge because changes in the text can be
          ;; triggered by various events like:
          ;; - a validation function rejecting a user inputted char
          ;; - the user enters a lower case char, but is transformed to
          ;;   upper.
          ;; - the user selects multiple chars and deletes text
          ;; - the user pastes in multiple chars, and some of them are
          ;;   rejected by a validator.
          ;; - the user selects multiple chars and then types in a
          ;;   single new char to repalce them all.
          ;; Coming up with a sane cursor repositioning strategy hasn't
          ;; been easy ALTHOUGH in the end, it kinda fell out nicely,
          ;; and it appears to sanely handle all the cases we could
          ;; think of.
          ;; So this is just a warning. The code below is simple
          ;; enough, but if you are tempted to change it, be aware of
          ;; all the scenarios you have handle.
          (let [node-value ($ node :value)]
            (if (not= node-value dom-value)
              ;; IE has not notified us of the change yet, so check again later
              (reagent.impl.batching/do-after-render #(input-set-value this))
              (let [existing-offset-from-end (- (count node-value)
                                               ($ node :selectionStart))
                    new-cursor-offset        (- (count rendered-value)
                                               existing-offset-from-end)]
                ($! this :cljsDOMValue rendered-value)
                ($! node :value rendered-value)
                ($! node :selectionStart new-cursor-offset)
                ($! node :selectionEnd new-cursor-offset)))))))))


;; This is the same as `reagent.impl.template/input-handle-change`
;; except that the reference to `input-set-value` points to our fn.
(defn input-handle-change
  [this on-change e]
  ($! this :cljsDOMValue (-> e .-target .-value))
  ;; Make sure the input is re-rendered, in case on-change
  ;; wants to keep the value unchanged
  (when-not ($ this :cljsInputDirty)
    ($! this :cljsInputDirty true)
    (reagent.impl.batching/do-after-render #(input-set-value this)))
  (on-change e))


;; This is the same as `reagent.impl.template/input-render-setup`
;; except that the reference to `input-handle-change` points to our fn.
(defn input-render-setup
  [this jsprops]
  ;; Don't rely on React for updating "controlled inputs", since it
  ;; doesn't play well with async rendering (misses keystrokes).
  (when (and (some? jsprops)
          (.hasOwnProperty jsprops "onChange")
          (.hasOwnProperty jsprops "value"))
    (let [v         ($ jsprops :value)
          value     (if (nil? v) "" v)
          on-change ($ jsprops :onChange)]
      (when (nil? ($ this :cljsInputElement))
        ;; set initial value
        ($! this :cljsDOMValue value))
      ($! this :cljsRenderedValue value)
      (js-delete jsprops "value")
      (doto jsprops
        ($! :defaultValue value)
        ($! :onChange #(input-handle-change this on-change %))
        ($! :ref #($! this :cljsInputElement %1))))))


;; This version of `reagent.impl.template/input-component?` is
;; effectively the same as before except that it also detects Material's
;; wrapped components as input components. It does this by looking for
;; a property called "inputStyle" as an indicator. (Perhaps not a
;; robust test...)
;;
;; By identifying input components more liberally, Material textfields
;; are permitted into the code path that manages caret positioning
;; and selection state awareness, in reaction to updates. This alone
;; is necessary but insufficient.
(defn input-component?
  [x]
  (or (= x "input")
    (= x "textarea")
    (when-let [prop-types ($ x :propTypes)]
      ;; Material inputs all have "inputStyle" prop
      (and (aget prop-types "inputStyle")
        ;; But we only want text-fields, so let's exclude radio/check inputs
        (not (aget prop-types "checked"))
        ;; TODO: ... and other non-text-field inputs?
        ))))


;; This is the same as `reagent.impl.template/input-spec` except that
;; the reference to `input-render-setup` points to our fn.
(def input-spec
  {:display-name "ReagentInput"
   :component-did-update input-set-value
   :reagent-render
   (fn [argv comp jsprops first-child]
     (let [this reagent.impl.component/*current-component*]
       (input-render-setup this jsprops)
       (reagent.impl.template/make-element argv comp jsprops first-child)))})


;; This is the same as `reagent.impl.template/reagent-input` except
;; that the reference to `input-spec` points to our definition.
(defn reagent-input []
  (when (nil? reagent.impl.template/reagent-input-class)
    (set! reagent.impl.template/reagent-input-class (reagent.impl.component/create-class input-spec)))
  reagent.impl.template/reagent-input-class)


;; Now we override the existing functions in `reagent.impl.template`
;; with our own definitions.
(defn set-overrides!
  []
  (set! reagent.impl.template/input-component? input-component?)
  (set! reagent.impl.template/input-handle-change input-handle-change)
  (set! reagent.impl.template/input-set-value input-set-value)
  (set! reagent.impl.template/input-render-setup input-render-setup)
  (set! reagent.impl.template/input-spec input-spec)
  (set! reagent.impl.template/reagent-input reagent-input))

[madvas]

This works great, thanks! But I'm not sure if we can include it like this in library. If people will use different versions of reagent, it will easily break.

[radhikalism]

@madvas I agree, this snippet is just a rough proof of the bug.

I don't think cljs-react-material-ui should patch Reagent. But I also don't think Reagent should be specifically aware of Material quirks for cljs-react-material-ui.

My current view is that Reagent should allow adapt-react-class to take some extra parameters that tag the class with a flag or predicate that would let the component pass the input-component? check, and also associate an optional fn that "locates" the node for a component in input-set-value (default being to just find-dom-node (> 0.6.0) or get :cljsInputElement (0.6.0)).

Then cljs-react-material-ui can modify its text-field definition to set those optional parameters when it invokes r/adapt-react-class.

What do you think? I'm open to other approaches.

Also, how would you like to co-ordinate with Reagent releases? Target 0.6.0 and re-visit with the next release, or just target the next release?

[madvas]

Great, I think extra params to adapt-react-class is fairly elegant approach. We'll see what reagent guys will think. If you feel like doing pull request to reagent, go for it, since you seem to understand problem very well. If not, I can do it. I think just targeting next release is good enough. We'll leave it as it is for 0.6.0.

[radhikalism]

Quick update: still working on a more correct solution. My earlier snippet does update <input> element values, but then input state does not always match the TextField component's state. In particular, Material-UI TextFields keep a state flag hasValue to determine if the value is empty/null/undefined (and show hint text etc). If it was previously :hasValue false, and we directly write to the inner <input> element, it will render overlapping hint text and value text. To solve this, we can call setState from within input-set-value to sync the TextField component, so this will be another component-specific function for cljs-react-material-ui to supply to Reagent.

[euccastro]

I tried @arbscht's set-overrides! snippet with reagent 0.6.0, and the caret works fine when I type in text. Alas, I can't set the value of my TextField programmatically (ie., in a reaction). Is this expected?

[madvas]

@arbscht What's status on this, have you made any progress?

[radhikalism]

Nope, only got as far as a very specific monkey-patch that suits the application I'm working on, which would probably break Reagent for non-crmui users if factored out.

Haven't made a working proof-of-concept of my proposed enhanced adapt-react-class yet. It's a little tricky to properly expose the API I sketched out and pass the params all the way down the chain in reagent.impl.template. Either involves mutable js values or more significant code changes (and regression risk) than I'd like...

I can't think of a better solution, just needs some care and time to test Reagent thoroughly, which is beyond me for the moment! Might have a go at it over the holidays.

[radhikalism]

FYI something else I was looking at last time: maybe we can reshape the component being exposed to Reagent so that its <input> is findable by the existing code that tries to locate it ((find-dom-node this)). I don't know how feasible this is but it would have the benefit of leaving Reagent unchanged and containing any custom glue hacks to crmui only, which may be appropriate.