facebookarchive / draft-js

A React framework for building text editors.
https://draftjs.org/
MIT License
22.58k stars 2.64k forks source link

Lost position of the cursor when editing #989

Open cestrensem opened 7 years ago

cestrensem commented 7 years ago

Do you want to request a feature or report a bug? Bug

What is the current behavior? User already has some text in the text area. He puts cursor with his mouse somewhere and quickly types something, then again quickly puts cursor to other place in the text area and again quickly types something. After these several "puts and types" the place where the cursor is and the place where the typed text appears are not in sync anymore. The cursor is in one place, the text appears in another place of the text area.

**If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. https://youtu.be/RZUzLyvLgnY

What is the expected behavior? Text appears on the same position where the cursor is.

Which versions of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of Draft.js? Draft js 0.10.0 & 0.9.1.

Browsers:

flarnie commented 7 years ago

Hi @cestrensem - thanks for reporting this and making the recording of the problem! This definitely does not look good, and I will look into it as soon as I get a chance. I'd also be happy to review any PR that has a solution.

It looks like that is a large area with many blocks, and since we have seen performance issues with rendering a large number of text blocks I wonder if that is contributing to this problem also.

colinjeanne commented 7 years ago

Having a large number of blocks seems to make this easier to repro but I find it pretty easy to repro with just two blocks with a few words each (and a decorator that does a small amount of work). I'm trying to investigate this and it seems - so far - that the issue is that a call to getUpdatedSelectionState appears to be returning the wrong selection when typing quickly causing Draft and the browser to disagree where the caret actually is.

This repros only when native insertion is allowed, which makes sense. The issue is fundamentally that Draft and the browser are disagreeing about where selection is. The browser inserts text in one location and Draft inserts it in another.

This is not, however, the result of the setImmediate call in editOnBeforeInput. We have a local branch which removes this call due to another issue which we have yet to file on Facebook's Draft repo and this issue still occurs in that branch.

colinjeanne commented 7 years ago

feb-01-2017 14-26-37

The bug repros at frame 49 of this GIF. The Before input active logs on this line, the DOM selection is being logged on this one and selection unchanged means that this line was hit.

colinjeanne commented 7 years ago

Here's what I believe is happening:

getUpdatedSelectionState is claiming that that the old and new selections are equal because it's being given the nodes for the previously selected block. These nodes are ultimately coming from the DOM selection in getDraftEditorSelection so it seems that the DOM's selection hasn't been updated yet. The third z is added to the first block by the browser during its native insertion and the third z is added by Draft in editOnBeforeInput where it thinks the browser is adding the z.

I don't yet know why the data from the block that has the browser's selection is copied over the corresponding data in the second block but that is a common occurrence I see in similar issues.

dreamcog commented 7 years ago

@colinjeanne I have a simlar problem like this, so how can i slove it? thanks a lot

colinjeanne commented 7 years ago

@dreamcog I don't yet know. I haven't had a chance to dig further into this since I wrote the above post and am hoping that someone else is able to use that information to come up with a fix since I'm unlikely to be able to get to it soon.

Milos5611 commented 7 years ago

I have almost same problem. When type fast my text is broken and go to another line. As i see DOM is breaking and text appear in parent DOM element. I have same problem on FF in 0.9.0 but no in 0.10.0...in this version only IE 11 is buggy. Does anyone have a clue what could it be ?

colinjeanne commented 7 years ago

Looking at the playbacks I notice that the bug repros after I click in a block in the frame immediately following my log in editOnBeforeInput. At this point Draft appears to be in a bad state and so the next character press causes it to improperly copy text from a different block while rectifying its state with what's in the DOM. @max-winderbaum confirmed separately that adding a call to editOnSelect in editOnSelect to handle a change in selection between the onBeforeInput and onInput events does fix this bug at the expense of another bug we are currently attempting to fix.

Further, this bug can only repro when native insertion is enabled.

For those playing at home, onBeforeInput is not a real browser event but is one that React is emulating. This appears to leave open two windows for the selection to change

  1. After onBeforeInput is handled but before the native insertion is processed (not shown in this bug)
  2. After the native insertion but before editOnInput (this bug)

There is further work to come up with the right fix but this seems to be a good working model for what's happening and implies avenues to create a proper fix.

colinjeanne commented 7 years ago

@flarnie: We have a fix for this issue - https://github.com/textioHQ/draft-js/pull/39

The diff here may seem a bit unfamiliar because it builds off of https://github.com/textioHQ/draft-js/pull/24 which is yet another variant of #667. There is a bug that we haven't yet filed in this repository that affects Draft in general and which motivated this change (Edit: now filed as #1018). That's unrelated to this bug, though.

The idea behind this change is that since selection may change between editOnBeforeInput and editOnInput we need to first recover the correct selection upon entry into editOnInput. If the selection changed then we also know that the block we had preemptively updated in editOnBeforeInput is not the block that was updated by the browser and so we replace it with that block's state from before editOnBeforeInput and let editOnInput fix up the block that actually was updated.

max-winderbaum commented 7 years ago

@flarnie https://github.com/facebook/draft-js/pull/1042 should fix this issue

mcnewbk commented 7 years ago

Any updates on this issue. I get the same problem, but I can only edit/add text to the end of lines. If it is in the middle of the line, my cursor will select the text at that location. I cannot add spaces in the middle of the lines either.

cestrensem commented 7 years ago

Hey guys, any updates???

blakeperdue commented 7 years ago

+1 would – kindly and politely – like an update :)

marlonmleite commented 7 years ago

any solution @max-winderbaum? In last release this error is present.

marlonmleite commented 7 years ago

Video bug: https://www.youtube.com/watch?v=EV-U8CS2qdk

This error only occurs when I initialize the state from my store with this:

componentWillReceiveProps(props) {
    const content = stateFromHTML(props.value || '')

    this.setState({
      editorState: EditorState.createWithContent(content)
    })
  }
colinjeanne commented 7 years ago

@marlonmleite: The original solution we had for this issue will need to be updated to account for changes in this area. We have not had time to integrate the latest Draft updates into our fork.

marlonmleite commented 7 years ago

@colinjeanne you have guidance or examples for me to do that workaround you said and solve my problem in this moment?

marlonmleite commented 7 years ago

I fix this bug in my project, with:

  componentWillMount() {
    const { value } = this.props

    this.state = {
      editorState: this.getCreatedEditorState(value)
    }
  }

  componentWillReceiveProps(nextProps) {
    const { value } = nextProps

    if (!this.props.value && value) {
      this.setState({ editorState: this.getCreatedEditorState(value) })
    }
  }

  getCreatedEditorState(value) {
    const contentState = stateFromHTML(value || '')

    return EditorState.createWithContent(contentState)
  }
againksy commented 7 years ago

Hello, @cestrensem @flarnie @colinjeanne @max-winderbaum @mcnewbk, is there any solution? anybody fixed this? @marlonmleite where can i get stateFromHTML function?

againksy commented 7 years ago

@cestrensem @flarnie @colinjeanne @max-winderbaum @mcnewbk, @marlonmleite Dears, the cursor jumps when beginning to enter text. Carret jumps before first entered letter. Also if we press backspace - it jumps to beginning of line , so what is the solution, maybe anyone knows?

marlonmleite commented 7 years ago

@againksy the imports are:

import { stateFromHTML } from 'draft-js-import-html'
import { stateToHTML } from 'draft-js-export-html'

I use stateToHTML in:

  onChange(editorState) {
    const content = editorState.getCurrentContent()
    const text = content.hasText() ? stateToHTML(content) : ''

    this.setEditorState(editorState, text)
  }

Seems to be the best solution for me.

againksy commented 7 years ago

@marlonmleite what if i have not html in content? I have an draft object with entities and blocks ...

colinjeanne commented 7 years ago

I have a few moments to look at this again. It seems like it is even easier to hit this bug in the recent versions of Draft. This GIF is in Chrome on draftjs.org and the repo is simple: have the caret on the top line and press a key while simultaneously clicking on the other line. That indicates to me that the original fix we had created likely still applies (but I will test more completely once I bring our fork in sync with what's in the main repository).

nov-06-2017 08-21-57

myyellowshoe commented 7 years ago

Was having the same issue, but in my case this ended up being related the https://www.draft-js-plugins.com/plugin/focus and how I was setting focus.

The focus plugin wants you to wrap the editor with div with onClick that sets focus. In my case I just commented out the focus onClick and voila the bug went away!

I don't quite understand the depth of the need for the onClick, with the focus plugin, but can't find any negative effects yet.

Hope this helps some others.

thibaudcolas commented 6 years ago

Same issue as #424 and #910 it seems.

roundrobin commented 6 years ago

Any updates on this issue? @cestrensem @flarnie @colinjeanne @max-winderbaum @mcnewbk, @marlonmleite ? Did the changes from the textio fork land into the Draft repo?

colinjeanne commented 6 years ago

I have a PR into the Facebook's repo, #1609, but it has not yet been checked in.

roundrobin commented 6 years ago

@flarnie is there a plan to merge this PR anytime soon?

roundrobin commented 6 years ago

@flarnie any update on this? Fixing this issue would really improve the user experience.

roundrobin commented 6 years ago

@colinjeanne Do you know if it's possible to use this PR in my app so that it works with the Draft.js plugins?

I tried referencing this PR in my package.json but then the Draft.js plugins were complaining.

colinjeanne commented 6 years ago

@roundrobin if you mean https://www.draft-js-plugins.com then yes, this PR should work with at least the base editor. My fork of Draft with this change is sync'd to somewhere between the 10.1 and 10.2 Draft releases but I do use draft-js-plugins for extended editor component. I don't use any of the other plugins.

I am curious, though, what the complaints are?

roundrobin commented 6 years ago

I'm trying to reproduce this again, but know I'm struggling to install this PR with NPM. Do you know the command to add this specific PR to my node_modules?

colinjeanne commented 6 years ago

You likely can't install this PR with npm. Instead you'd have to build Draft locally and sync to this change or otherwise merge this PR.

colinjeanne commented 6 years ago

I do not but I don't think it's related to this PR.

roundrobin commented 6 years ago

You're right, this was unrelated to this PR, so I removed the comment because it didn't help the discussion.

For others: at the end I was able to include your PR in my workflow (npm + webpack), by including the dist folder in my forked version that includes the PR 1609, which webpack needs as as an entry point.

Thanks @colinjeanne for your contribution.

Tarun1987 commented 5 years ago
  componentWillMount() {
    const { value } = this.props

    this.state = {
      editorState: this.getCreatedEditorState(value)
    }
  }

  componentWillReceiveProps(nextProps) {
    const { value } = nextProps

    if (!this.props.value && value) {
      this.setState({ editorState: this.getCreatedEditorState(value) })
    }
  }

  getCreatedEditorState(value) {
    const contentState = stateFromHTML(value || '')

    return EditorState.createWithContent(contentState)
  }

Worked for me as well (Y)

dasm30 commented 5 years ago

componentWillReceiveProps(nextProps) { const { value } = nextProps if (!this.props.value && value) { this.setState({ editorState: this.getCreatedEditorState(value) }) } }

I'm also having this issue and no way to resolve, can you elaborate further on your solution?

sandeepreddy19 commented 5 years ago
  const newState = EditorState.createEmpty()
        this.setState({
          editorState: EditorState.moveFocusToEnd(newState)
        })

This worked for me

camsjams commented 5 years ago

It appears as though the PRs from @max-winderbaum and the adjustments that @colinjeanne have made have gone unnoticed.

I used the fork that the Textio team created and can also confirm that the bug is no longer present in their build.

@claudiopro @fabiomcosta @gkz @dsainati1 seem to be the recent contributors nowadays on this repo: Sorry to bug you all but is there anything we can do to speed up this long open PR?

I can help code review or perhaps there is some business reason why your team isn't accepting the PR.

I would just like some idea of why. Otherwise it seems like the Textio fork has some bugs fixed and those entering this thread in 2019 can look there as an option (the code samples above are not quite what is desired for me).

colinjeanne commented 5 years ago

@camsjams, the PR hasn't gone unnoticed. The issue is that our fork is based on a very old version of Draft, approximately 0.10.2 if I recall correctly. There has been a lot of code change in this area since then and when I tried to update the PR to 0.10.5 I could no longer convince myself that the original solution was doing the thing I believed it should be doing. I don't believe it's insurmountable but it just takes time to do the investigation and I haven't had time to do it.

Internally to Textio we are unlikely to invest time here because our needs have changed significantly as well and we may not be able to continue to use Draft in the same way we are today.

camsjams commented 5 years ago

@colinjeanne Thank you for the response, this is good closure on this thread :+1: .

I understand that its a reasonably large change, and I appreciate the candor regarding the future of this library in your team's stack.

I may do some exploratory work here on the source code to see if there is anything I can tweak in the newer code.

colinjeanne commented 5 years ago

If you'd like to take a look, the PR I was working on was https://github.com/facebook/draft-js/pull/1609. The issue that I couldn't resolve is primarily restricted to editOnBeforeInput.js: the trick is to allow mustPreventNative to be false in as many cases as possible to increase performance but allowing native insertion is what opens up the race condition that causes this issue. I was unable to determine if my change was allowing native insertion in the correct situations.

cdow commented 5 years ago

As far as I can tell, this issue is caused by the editor state selection having hasFocus: false when the editor actually has focus. Normally, the DraftEditorLeaf maintains cursor position, after text updates, by moving the cursor to roughly where it was before the update. However, it only does this if it thinks it is rendering a block that has focus. If it doesn't think it has focus, then it will just update the rendered text which causes the cursor to jump back to the start.

The cause of this, in any particular application, can vary. It can occur because there can be an async step between the editor onChange and updating the editorState prop for the editor. This can cause a delay between when the editor actually has focus and when it thinks it has focus. It can also occur because a block has focus and the editor state selections is manually updated but drops hasFocus: true.

I don't think this a bug per se, but more of a poor tolerance for bad input.

operfildoluiz commented 4 years ago

There any news at this point?

Struggling my head out in here 😆

wdfinch commented 4 years ago

I agree with @cdow's explanation. Would be great to improve this.

brennancheung commented 3 years ago

The other solutions were causing it to always have focus at the end. If the user changed the cursor to the middle it would still jump to the end. This is the solution I came up with:

  const handleChange = editorState => {
    // For some reason after typing the first character Draft.js resets the
    // focus to the start.  We need to work around this.
    const newState = convertToText(editorState).length === 1
      ? EditorState.moveFocusToEnd(editorState)
      : editorState
    setEditorState(newState)
  }
celestemartins commented 3 years ago

@flarnie Are you guys working on the fix?

SubhasisDebsharma commented 2 years ago

You have to use class component to have better control over component updates. You need to maintain the content HTML as a string in the state to compare new props and the current state easily.

class TextEditor2 extends React.Component {
    constructor(props) {
        super(props);
        this.state = {
            editorState: getEditorState(props.content),
            content: props.content
        }
    }
    handleEditorStateChange = (editorState) => {
        const content = draftToHtml(convertToRaw(editorState.getCurrentContent()));
        this.setState({editorState, content})
        this.props.onChange(content);
    }

    componentDidUpdate(prevProps) {
        if (prevProps.content !== this.props.content && this.props.content !== this.state.content) {
            this.setState({
                editorState: getEditorState(this.props.contnet),
                content: this.props.content
            })
        }
    }

    render() {
        const {editorState,} = this.state;
        const {placeholder, readOnly, classes, className} = this.props;
        return (<Editor
            editorState={editorState}
            wrapperClassName={clsx(classes.wrapper, className)}
            toolbarClassName={classes.toolbar}
            editorClassName={classes.editor}
            readOnly={readOnly}
            stripPastedStyles={true}
            placeholder={placeholder}
            onEditorStateChange={this.handleEditorStateChange}
            toolbarOnFocus
            toolbar={{
                options: ['fontFamily', 'fontSize', 'inline', 'colorPicker', 'link', 'list', 'textAlign', 'remove'],
                inline: {
                    options: ['bold', 'italic', 'underline', 'strikethrough'],
                },
                colorPicker: {
                    className: undefined,
                    component: undefined,
                    popupClassName: undefined,
                    colors: Colors,
                },
            }}
        />)
    }
}
export default withStyles(styles)(TextEditor2);
mitevdimitar commented 2 years ago

This little trick solved the issue with the duplicated rows for me. It seemed that the unwanted change of the state comes with lastChangeType called "apply-entity". I couldn't reproduce other cases where this change type occurred, so I isolated it by not updating the state in those cases:

onChange = (newState) => {
    const lastChangeType =
      newState && newState._immutable && newState._immutable.lastChangeType;
    if (lastChangeType === "apply-entity") return;
    this.setState({
      editorState: newState
    });
}