Caret moves to the end when editing a TextArea in reagent #17
Comments
|
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)))}]))) |
|
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! |
|
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. |
|
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). |
|
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. |
|
Okay, thanks for a info. I updated readme. I'll try to look at this in more depth when I find more time. |
|
I think the problem is that a Material UI textfield is a not a |
|
This (extremely ugly) workaround works: (defn textfield []
(reagent/create-class
{:display-name "textfield"
:reagent-render (fn [props]
[rui/text-field props])})) |
|
Oh, really? Thank you a lot for this!! |
|
@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 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")))) |
|
Yes sorry meant to type |
|
Ok pushed changes, this should be resolved now |
|
Fix works, but seems to have some side effects. After switching to With |
|
This fix usually works, except when |
|
Okay guys, you were both right. 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. |
|
Thanks for the fix with optimisations! Looks like the latest release on clojars is still |
|
So there are 3 aspects to this bug.
If we were to hack Reagent to workaround 1 and 2 directly, it might look like this: However, this exact patch is probably not the right way to structure a generic library. What if Reagent provided opportunities through |
|
@arbscht thank you very much for your reply. Your patch is against |
|
@madvas Reagent master has I'm not actually sure where |
|
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 |
|
Be careful with
Otherwise it will misbehave as you see. (Indeed there are other incomplete aspects to this patch. It incorrectly detects radio/check inputs in |
|
@madvas Try this, paste and invoke ;; 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)) |
|
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. |
|
@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 Then cljs-react-material-ui can modify its 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? |
|
The fix in Reagent 0.8-alpha can be used like this: https://github.com/Deraen/problems/blob/reagent-material-ui-text-field/src/hello_world/core.cljs#L11-L15 (not sure why those options have to be provided, as these "defaults" probably work usually.) |
|
Great, thanks for this example @Deraen! |
|
After further testing, I noticed the code didn't update the input value correctly. This is now fixed, by selecting the real DOM input node from the TextField Div node. But this solution still has some problems, for cases where TextField has some logic based on if the value is set (hintText hidden, floatingLabel). Seems like hintText and floatingLabel have to be also updated manually on |
|
@Deraen In my earlier draft I dealt with the value-based logic differently. See: #17 (comment) for the full snippet. I can't test Reagent 0.8-alpha right now but it looks like it should still work. My approach was to update (next ... {:on-write (fn [new-value]
;; `blank?` is effectively the same conditional as Material-UI uses
;; to update its `hasValue` and `isClean` properties, which are
;; required for correct rendering of hint text etc.
(if (clojure.string/blank? new-value)
(.setState component #js {:hasValue false :isClean false})
(.setState component #js {:hasValue true :isClean false}))) |
|
@arbscht Calling |
|
@arbscht Do you remember how this should work? My current understanding is that it might be possible to get access to the real component by passing That would make it possible to call |
|
@Deraen @arbscht Tested in Edge, Chrome, Firefox. It doesn't require patching Reagent and taking in account MaterialUI implementation details (i.e. |
|
@Deraen Upon closer examination, I think you're right that |
|
@metametadata I like your solution, it's easier to understand. I used a similar approach as a workaround to this problem in a project. However, it had some tradeoffs:
|
|
@arbscht great that we're on the same page.
I should say at this stage my main concern is to revert the MR about |
|
Reagent can't detect wrapped inputs. I fear wrapping the components, like in @metametadata's example, is not going to be bulletproof no matter how much it is improved. There are just too many corner cases and differences between components. Only fixing TextField would require much more code than the example, and this code wouldn't work for different input components. The reason for the problem is Reagent implementation, so it makes sense that the Reagent has the fix. I'll test my idea for fixing synthetic input. |
Personally I don't care much about Reagent not being able to detect the wrapped inputs and warn users. Because incorrect rendering of inputs in async React wrappers is a common problem and non-beginners know about it. Beginners will bump into it themselves pretty quickly.
But we can say the same thing about the solutions based on new hooks.
But isn't it already fully fixed in my example? As you can see, wrapping MaterialUI's
And even if there are corner cases then they can still be dealt with without any hooks exposed by Reagent lib itself.
Yes, the root reason of the problem is async rendering. The solution can be a part of Reagent project of course. But I don't think that the hooks are better because:
Tl/dr: let's not touch Reagent core because:
|
|
@Deraen Here's what I've got working, based on your finding about needing to reference the inner component rather than the In reagent template.cljs: Usage: (def text-field
(r/adapt-react-class
(.-TextField js/MaterialUI)
{:synthetic-input
{:on-update (fn [input-node-set-value node rendered-value dom-value this]
;; node is the element rendered by TextField, i.e. div.
;; Update input dom node value
(let [node (aget (.getElementsByTagName node "input") 0)]
(input-node-set-value node rendered-value dom-value this
{:on-write (fn [new-value]
(let [inner-component (aget this "innerComponent")]
(if (clojure.string/blank? new-value)
(.setState inner-component #js {:hasValue false :isClean false})
(.setState inner-component #js {:hasValue true :isClean false}))))})))
:on-change (fn [on-change e]
(on-change e (.. e -target -value)))}}))What do you think of using refs for this purpose? |
|
@metametadata On further examination, I would still like a simpler solution (and the hooks code could certainly be improved) but I'm not sure that the
Async rendering is the root problem, but a complicating factor is that Reagent makes brittle assumptions about the shape of input components. Whatever solution we choose for controlling async rendering, we still ought to clean up Reagent core's buggy assumptions and partially failing code paths. Whether we warn users or silently abort or add documentation or something else, I don't know. But we can't revert to how it was and just build more workaround layers upon a broken foundation. So we'll have to touch Reagent either way.
Actually I think this is important for users. Having to use a "fixer" library just to make text fields work is kind of a bad smell. Also, users may not easily know when to use Reagent normally and when to switch to this "fixer" API or pattern. It would be better to make Reagent work in the general case even if it adds some complexity via discoverable optional parameters at the use site.
In the hooks-based solution, all requirements for special knowledge are clearly separated.
I have encountered this use case, which is what led me to the approach involving hooks in Reagent in the first place. Filtering user input for validation or triggering help etc. turned out to be tricky when generically wrapping |
I'm not sure I understand what are the partially failing code paths and what is currently broken in the foundation. Reagent only promises to render "raw"
The bad smell will stay anyways as the problem is inherent to async rendering. I.e. I can as well say that using hooks to make text fields work is a bad smell :) It will still have to be explained when to use hooks and how. And anyways not many people read docs that thoroughly until they see bugs in their apps. And if they do read then we can just put a link to "fixer" lib or any other non-hook solution in the docs for them. So IMHO adding optional public API to give users a hint about potential problem is a weak argument for hooks.
I think that's the worst part of the hook-based solution. As a user of the thrid-party lib I don't want ever to depend on its implementation details. In the next version they decide to rename
OK, it's hard to reason about it for me without looking at some code. I did have some validation/filtering in custom inputs but maybe my use-case was different. Can you maybe provide a simple use case so that I could think how wrapper fix could be accomodated for such scenario? I will try to create an example demo. But let's imagine it will require changing the contract of |
It's not clear to me this is the case. Maybe I missed something in the docs, but the behaviour seems to be undefined/quirky rather than well-defined as only supporting "raw" inputs. This bug in c-r-m-ui arose in the first place because it tried to use The mess in Reagent is that certain code paths make an effort to support complex input components by attempting to render, set state, trigger events, etc. Some of these behaviours succeed and others do not. It can be misleading and confusing since a basic use of a complex input field seems to work at first, and then certain features don't, but there is no explanation why. I don't have time to walk through all the code that tries to adapt complex inputs only to fail slowly. If you dig into reagent-project/reagent#282 you can see the different code paths described (which I'm suggesting should either execute correctly, or not execute at all).
Yes, I agree it's unpleasant and can be improved. But the question for me is which option is worse.
There is a key difference though:
I think this is a good point and we should probably add a hint or assertion that the adapted class is not functioning correctly along the code paths I mentioned above. But I think it's a nice-to-have if there are documented hooks provided, and more important if the user is supposed to rely on an external workaround or pattern.
In
One thing I've realized in this discussion is that in the universe where hooks are the better option, it shows that certain aspects of Material-UI's internals to do with DOM structure as well as Regardless of that point, I think we still need to consider which is the worse option between the two before us if upstream did nothing. One option is to depend on particular supported combinations of Material-UI+c-r-m-ui in order to use Reagent's hooks. The other option is to depend on a particular supported combination of the "fixer" library (or the This is a pretty complex issue because of integration across multiple projects and stuff falling through the cracks, so there are lots of combinations for solutions. I don't know if adding hooks is really the best answer but I'm increasingly sure an external workaround wrapper which will alter the contract of an important callback (just for this one weird upstream bug or limitation)... is not obviously a better one. |
+1. I had to read issues in Reagent and other projects to get that Reagent won't try to fix complex inputs but only its "own"/"raw" inputs. Specifically, this is the issue from Reagent: reagent-project/reagent#79.
Now I understand, thanks.
So in this case c-r-m-u-i will have to depend on the way TextField is implemented in MaterialUI.js. It's bad because no code should depend on implementation details of some other code. To me it's a very pretty strong argument against the "hooks" solution. "Hooks" are dangerous because the resulting code will be error-prone by design. On the other hand, "wrapper" solution is always safe by design. Introducing "hooks" in Reagent would contribute to spreading bad habits amongst programmers for the sake of supporting masking/filtering/etc. in inputs.
It looks like React itself doesn't support the filtering/masking scenario you'd like to have, see facebook/react#955: "this is not a bug because React cannot guess the cursor position if you manipulate the input value in some way" Thus it's an overambitious task to make Reagent fix inputs and complex inputs to behave differently from what React does by default. I'm totally fine if Reagent wouldn't help with this problem at all and (even more so) I'm fine if wrapper libs such as c-r-m-u-i wouldn't try to solve it either.
Sounds unlikely that the authors of MaterialUI or any other complex inputs would ever bother with providing such public API only because Reagent decided to fix the problem not solved in React itself :) TL/DR:
|
|
Actually it looks like Om provides a "wrapper" solution officially: https://github.com/omcljs/om/blob/7ab33e8dee1133cf031df93a08787531a2f1985d/src/main/om/dom.cljs#L20 |
There are couple of important nuances in the discussion around reagent-project/reagent#79 and reagent-project/reagent#126 and our current topic:
I'm not sure this holds, given that Reagent actually does make an effort to manage caret positions because of 79 and 126. Also, quoting from 126:
So I figure this scenario is intended to be supported after all. I don't think it is overambitious to make the behaviour pluggable so Reagent's existing caret-managing code paths can successfully complete executing with respect to regular HTML
The particular implementation details which are interesting are:
To be clear, it is a problem that Reagent has decided to fix already, since 79 and 126, including apparently the filtering/validation use case (if I understand the issue log correctly). The hooks extension is not about expanding the scope of this behaviour to custom HTML tags or diverging any further from original React. It is to allow already-supported HTML components to be correctly located within any arbitrary wrapping of a third-party component, and to callback to the original component in order to sync/reset using a common UI pattern, using the code paths that Reagent is already providing for caret management.
That's interesting. I need to review that code more closely when I have time, but so far it looks to me like Om would have the same limitations as the wrapper solution here, which diverges from the direction Reagent has gone towards since 79 and 126. |
|
Got it. So Reagent tries to fix cursor jumps when one applies what I called a "naive" filtering/masking pattern and it cannot do the same with adapted components (which I erroneously called "complex inputs") unless "hooks" API is exposed. |
|
I implemented change to use |
|
Some new developments on reagent-project/reagent#351. The fix I found won't work with MaterialUI 1.0-beta. |
|
In case someone is looking for a simple fix to caret problem with MaterialUI 1.0 and re-frame, providing following InputProps to TextField component seems to do the trick: {:InputProps
{:inputComponent
(r/reactify-component
(fn [props]
[:input (dissoc props :inputRef)]))}}For some reason replacing the default implementation with Tested with Hope this helps someone. |
|
@vharmain Nice! "For some reason", the reason being that in this case the input element is created through Reagent logic which will wrap the component in Reagent input component with the fixes. Reagent doesn't have any control if the component is created directly using React. This seems like very good solution to this problem, no hacks required anywhere, just use of proper option in Material-UI Input component: https://material-ui.com/api/input/ |
|
Cool, that makes sense. Thanks for the explanation @Deraen. |
|
This is now documented in Reagent repo, with example project. Some additional logic is required for https://github.com/reagent-project/reagent/blob/master/doc/examples/material-ui.md |
|
I've just stumbled upon this issue after trying to switch from re-com to Material UI. An update: the solution with |
When I render this component:
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
The text was updated successfully, but these errors were encountered: