Skip to content
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

Bug: cursor jumps to end of controlled <input> tag when value is modified #18404

Closed
iain-merrick-fanduel opened this issue Mar 27, 2020 · 11 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@iain-merrick-fanduel
Copy link

iain-merrick-fanduel commented Mar 27, 2020

React version: 16.13.1

Steps To Reproduce

  1. Make an <input> tag controlled, by setting its value in response to onChange
  2. Apply a transformation to the value (for example, replace spaces with underscores)
  3. Move cursor to the middle of the text and edit it

Link to code example:

https://gist.github.com/iain-merrick-fanduel/b9cea57baa9f20a5d288a0fcd6e7ee5e

Adapted from CodePen example (https://codepen.io/gaearon/pen/VmmPgp?editors=0010) on https://reactjs.org/docs/forms.html

The current behavior

If the transformation changes the value, the cursor is moved to the end of the input.

The expected behavior

Cursor should remain at the original position if possible (this is the behaviour of the TextInput component in React Native).

@iain-merrick-fanduel iain-merrick-fanduel added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Mar 27, 2020
@iain-merrick-fanduel
Copy link
Author

This is very similar to #14904, but doesn't look quite the same:

  • I'm updating the value locally, in the same component
  • I'm modifying the value

In case this is browser-specific, I'm using Chrome 80 on OS X 10.14.6.

@iain-merrick-fanduel
Copy link
Author

As far as I know, this is the intended usage, as the docs specifically give this example:

handleChange(event) {
  this.setState({value: event.target.value.toUpperCase()});
}

That example exhibits the same bug — you can't edit the text field in the middle.

@vkurchatkin
Copy link

This has nothing to do with React, that's normal browser behaviour

@iain-merrick-fanduel
Copy link
Author

In hand-written HTML, you'd normally fix this by saving the cursor position before changing the text, and restoring it afterwards. But React doesn't provide that low-level access (by design).

Maybe React could manage the cursor position automatically? This would bring parity with React Native, where the TextInput component behaves as expected.

@vkurchatkin
Copy link

But React doesn't provide that low-level access (by design).

Well, that's where you are wrong. It's not that hard to implement this in React. Here is a demo: https://codesandbox.io/s/boring-dirac-utq82

@iain-merrick-fanduel
Copy link
Author

iain-merrick-fanduel commented Mar 27, 2020

Oho, neat! I can probably use this, thanks!

Is this something that could/should be added as default behavior for the <input> component? I can't think of scenarios where you would not want this, and even if you want something different (maybe slightly more refined for your specific input transformation) you could override it anyway. The example in the documentation would benefit from this.

Alternatively, the example code could be updated, but it seems like a lot of boilerplate for a simple task.

@ryanstan1ey
Copy link

ryanstan1ey commented Mar 28, 2020

It's actually a little strange that the example docs use this approach. I've read that since React uses event pooling one needs to capture the event and pass the captured event to the setState function.

If you modify your handleChange function to the following, it should work as expected.
handleChange(event) { let value = event.target.value.replace('/ /g','_'); this.setState({value}); }

Event pooling allows reuse of the event and because the setState is asynchronous, there is no guarantee on when it will be called and what value it holds at that instance.

@vkurchatkin
Copy link

Is this something that could/should be added as default behavior for the component?

I think it is a bit against React philosophy. React is about making DOM transformations declarative, not about changing how it works. If you need such behaviour, you can make your own <Input/> component which does just that (or more).

The example in the documentation would benefit from this.

I agree, there probably should be a warning in the docs explaining that it's not as _ straightforward_ to modify input this way.

If you modify your handleChange function to the following, it should work as expected.

No, your modified code is strictly equivalent to original code. What you can't do is use the event object asynchronously, including setState(() => ...) form, without calling e.persist().

@gaearon
Copy link
Collaborator

gaearon commented Apr 1, 2020

As far as I know, this is the intended usage, as the docs specifically give this example

Woah. This advice in docs seems pretty bad. I don't think this is supported in that sense.

This behavior is expected (React can't know where to put the selection after you've modified it).

I'll remove the misleading docs section.

@cefn
Copy link

cefn commented Aug 7, 2022

React can't know where to put the selection after you've modified it

I don't agree with @gaearon for the common case that the react state was actually driven by the change in the event.target.value to begin with. If that's the case, React could omit to set the value, or set the value AND preserve the selection and it CAN know where to put the selection (basically leave it alone as that cursor was what triggered the state change in the first place). Possibly this is what people were complaining about in #955 or #5386

UPDATE: After re-reading this thread the OPs case is different from the referenced issues in #955 and #5386 which do not transform the value like the OP's case for this issue. This comment is therefore misplaced. I've left the original comment below for context...

ORIGINAL COMMENT >>>

Where an edit in the control caused the state change, the contents of the control are suited to preserving the selection. The value held in the control is already the value react is attempting to set. It originated in the control and may be taking the long way round owing to some async state-propagation mechanism. For this case, preserving the selection is a well-defined behaviour, and the 'loss' of selection may be considered an artefact of the recommended 'controlled' component binding approach combined with current react reconciliation behaviour.

One of two alternative strategies seem reasonable for reconciling an already-identical DOM value:
a) Don't set the value at all, the selection (cursor) will be implicitly preserved.
b) DO restore the selection (cursor) after redundantly setting the value and therefore 'breaking' the selection. This is guaranteed to be a well-defined behaviour (and probably what was intended).

This assumes that state changes originated from a controlled component can't 'back up'. If a queue of changes sends a value from three keypresses ago, then two keypresses ago, then one keypress ago it will encounter a the dom value which is from the future, and can't copy across the selection meaningfully.

I've shared the workaround below for discussion. It embodies selection-preserving strategy a) in the form of a hook.

The workaround was effective for my async state engine in simple interactive testing. This avoids treating the value as managed state. Treating it as managed state ensures every edit (even edits made by an actual cursor within the control) causes a loss of cursor position. However like a managed state, if the value is NOT the value found in the DOM, then it overwrites the dom value with the value currently in the render function. I'd be curious if the approach works for others.

It can be used like...

<input type="text" {...useBinding(label)} />
import { useEffect, useRef } from "react";

export function useBinding(value: string) {
  const ref = useRef<HTMLInputElement>(null);
  useEffect(() => {
    const current = ref.current;
    if (current) {
      if (current.value !== value) {
        // handle case where value DIDN'T originate from this control
        // e.g. should overwrite because state was set outside
        current.value = value;
        current.setSelectionRange(current.value.length - 1, null);
      }
    }
  }, [ref, value]);
  return {
    ref,
    value,
  };
}

@tantaman
Copy link

tantaman commented May 19, 2023

The proposed solutions seem a bit convoluted and still have visual artifacts (such as the cursor jumping to the end and back over two frames).

Copying the state into a synchronous variable to act as a cache for the async store seems more straightforward and general:

function useCachedState<T>(propValue: T): [T, (value: T) => void] {
  const [lastValue, setLastValue] = useState(propValue);
  const [currValue, setCurrValue] = useState(propValue);
  if (propValue !== lastValue) {
    setLastValue(propValue);
    setCurrValue(propValue);
  }

  return [
    currValue,
    (value: T) => {
      setCurrValue(value);
    },
  ];
}

function EditableItem({
  id,
  value,
}: {
  id: string;
  value: string;
}) {
  const [cachedValue, setCachedValue] = useCachedState(value);
  const onChange = async (e: React.ChangeEvent<HTMLInputElement>) => {
    setCachedValue(e.target.value);
    return updateSomeAsyncStore(e.target.value);
  };

  return <input type="text" value={cachedValue} onChange={onChange} />;
}

This will make the update synchronous to the component and keep the caret position in the input. If the async store replies back with something that doesn't match the optimistic synchronous update, the async store will override the synchronous state.

This approach is also general and works not just for text inputs but also for draggables or anything else.

Ideally, though, the "someAsyncStore" would have a reactive system that can update synchronously rather than pushing this concern out into components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

6 participants