Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Typed char gets duplicated if also changing block type in onChange #92

Closed
Pajn opened this issue Feb 25, 2016 · 15 comments · Fixed by #667
Closed

Typed char gets duplicated if also changing block type in onChange #92

Pajn opened this issue Feb 25, 2016 · 15 comments · Fixed by #667
Assignees

Comments

@Pajn
Copy link

Pajn commented Feb 25, 2016

I want to change the block type in onChange, however the typed character gets duplicated when doing that. Backspaces is not duplicated however so I can still remove one character at a time, not just type them.

Doesn't the editor support pushing another content change before being rerendered with its updated one?

@hellendag
Copy link

This sounds like a use case for handleBeforeInput. When the user types a certain character, you can handle the state change as needed, and return true to kill the event.

Does that work for you?

@Pajn
Copy link
Author

Pajn commented Feb 26, 2016

Unfortunately not, I don't want to cancel the default behavior, just add too it.

My use case is a markdown editor that styles the text as a way of syntax highlighting, but I still want the actual markdown shown to the user.
So when the user types # on a new row it becomes a header but when the user types # again to create a second level, ## gets inserted so the text is now a header level three.

@wcjohnson
Copy link

I'd like to chime in on this one as well. I'm trying to make a Twitter post input (starting from the hashtag decorator example) and I'm looking to duplicate the behavior of Twitter's clients where it will highlight characters past 140 in red. After many fumbles, I settled on the technique of applying an inlineStyle to the overflow text. It works, except for the fact that every character I type into the control is duplicated.

EDIT: On further experimentation, it's not consistently EVERY typed character that's duplicated. It appears the first typed character in a new block will not be duplicated.

Here's my handleChange:

    handleChange: (editorState) ->
        content = editorState.getCurrentContent()
        okRange = makeSelectionStateFromRange(content, 0, 140)
        console.log { okRange: okRange?.serialize() }
        if okRange
            content = Modifier.removeInlineStyle(content, okRange, 'OVERFLOW')
        tooLongRange = makeSelectionStateFromRange(content, 141, 9999)
        console.log { tooLongRange: tooLongRange?.serialize() }
        if tooLongRange
            content = Modifier.applyInlineStyle(content, tooLongRange, 'OVERFLOW')
        @setState { editorState: EditorState.set(editorState, { currentContent: content }) }

@sophiebits
Copy link
Contributor

@wcjohnson I think the simplest is to use a decorator for that purpose. Something like:

function past140Strategy(contentBlock, callback) {
  const text = contentBlock.text();
  if (text.length > 140) {
    callback(140, text.length);
  }
}

@wcjohnson
Copy link

@spicyj Your code above was the very first thing I tried, actually. I spent a lot of time trying to get it to work with decorators and I found that if the user puts a carriage return in the input, thus creating a new block, I couldn't make it work.

I even tried writing a decorator strategy that would close over the contentState, and then I added up all the block lengths inside the strategy. That approach failed because the immutable.js contentState isn't updated until after all the strategies are already called...

@Pajn
Copy link
Author

Pajn commented Feb 26, 2016

On further experimentation, it's not consistently EVERY typed character that's duplicated. It appears the first typed character in a new block will not be duplicated.

That seems consistent with my behavior that the first # isn't duplicated either as a header is always in a new block.

EDIT: Maybe it's not the changed state that become duplicated but instead the one after? Could explain the above sighting but I don't really know how to test and verify that :/

@wcjohnson
Copy link

Further information: When I mutate the editorState in handleChange(), handleChange is called TWICE per keystroke -- the mutation inside handleChange is somehow provoking another change event.

If I don't mutate the editorState in handleChange(), handleChange only gets called once.

It appears Draft's internal model is expecting it to be in state X after calling onChange(X), and when it finds itself in X' instead of X, it fires another onChange. If that's really what's happening, it would seem to be a breakage of the standard 'controlled component' pattern.

@hellendag
Copy link

On further experimentation, it's not consistently EVERY typed character that's duplicated. It appears the first typed character in a new block will not be duplicated.

In cases like key entry at the start of a block, the native event is prevented and rendering is performed strictly by the Editor component. (We generally try to let regular typing occur natively, to preserve spellcheck highlighting and avoid unnecessary re-renders.)

I found that if the user puts a carriage return in the input, thus creating a new block, I couldn't make it work.

You can use handleReturn to insert a soft newline instead via RichUtils.insertSoftNewline, thereby disallowing the creation of new blocks.

EditorState.set(editorState, { currentContent: content })

You should avoid using set/merge directly on EditorState (http://facebook.github.io/draft-js/docs/api-reference-editor-state.html#content). Use push instead.

I should probably guard against this.

the mutation inside handleChange is somehow provoking another change event.

Hmm, this should not be the case. Creating new state objects will have no impact on event handlers.

Just to be clear, this is an onChange handler, right? Not a handleBeforeInput?

@wcjohnson
Copy link

@hellendag Yes, this is an onChange handler. Here is render:

    render: ->
        <div className="post-editor">
            <div className="inner-container" onClick={=> @refs?.editor?.focus()}>
                <Editor ref="editor" editorState={@state.editorState} onChange={@handleChange}
                    placeholder="Enter content..." spellCheck stripPastedStyles
                    customStyleMap={styleMap} />
            </div>
            <input
                onClick={this.logState}
                type="button"
                value="Log State"
            />
        </div>

With handleChange exactly as above, it is called twice per keystroke. With the following handleChange it is only called once per keystroke:

    handleChange: (editorState) ->
        console.log "handleChange"
        content = editorState.getCurrentContent()
        # okRange = makeSelectionStateFromRange(content, 0, 140)
        # console.log { okRange: okRange?.serialize() }
        # if okRange
        #   content = Modifier.removeInlineStyle(content, okRange, 'OVERFLOW')
        # tooLongRange = makeSelectionStateFromRange(content, 141, 9999)
        # console.log { tooLongRange: tooLongRange?.serialize() }
        # if tooLongRange
        #   content = Modifier.applyInlineStyle(content, tooLongRange, 'OVERFLOW')
        @setState { editorState: EditorState.set(editorState, { currentContent: content }) }

@wcjohnson
Copy link

@hellendag Your suggestion about handleReturn will likely solve my problem. Thank you. I still feel there's something odd going on here, but if I don't have to deal with multiple blocks, then a decorator will work for me.

@hellendag
Copy link

I think I've got the answer here.

I think the issue here is that the natively-rendered ContentState is the one associated with the editorState you are receiving in your handler. (This is recorded as nativelyRenderedContent in the EditorState object itself.) We mark this state as being natively-rendered in order to allow the native event to occur, skipping re-renders for the reason I noted above.

When you changed to a new ContentState within handleChange, the nativelyRenderedContent value and your new ContentState did not match up. When this occurs, the component will re-render, because it believes that the current ContentState is not natively rendered. The event still occurs and the character is natively inserted, but your re-render also takes place. The character thus appears twice.

API-wise, you shouldn't have to know about this stuff. That's my fault.

I think the best thing to do here is to use a decorator, but I'll see if I can think of a better way to deal with this in the API.

@hellendag hellendag self-assigned this Feb 26, 2016
@hellendag
Copy link

One straightforward option would be to offer a boolean prop to indicate that all rendering must be performed via React, with no native keystroke deferrals. Then you could do whatever you want in onChange.

Drawbacks would be that spellcheck highlighting would flicker and we would pay re-rendering costs. Also that it's a bit of a hammer in the API.

Continuing to think about it.

@Pajn
Copy link
Author

Pajn commented Feb 26, 2016

Maybe another handler that if specified will be called before the native behavior is allowed.
If that handler returns an updated editorState the native behavior is disallowed, if that handler is not passed, returns null or the returns current editorState the native behavior is allowed.
Then you pass the editorState to handleChange as usual and forbid modifications of the editorState in there.

Just a thought, maybe a bit clumpy but better than flickering for applications that only occasionally will do style changes.

@paulyoung
Copy link
Contributor

FWIW, I'm attempting to address this in #453.

@dgcoffman
Copy link

Highlighting characters over a limit would be a good example. Many people want this (many Stack Overflow questions and jQuery plugins etc), and it requires a couple non-obvious tricks, like using RichUtils.insertSoftNewline.

Attempting to do this any way other than with a decorator gets very messy and unless you know that inserting a soft newline prevents returns from creating new content blocks, you will have a bad time.

sophiebits added a commit to sophiebits/draft-js that referenced this issue Dec 6, 2016
Fixes facebookarchive#92.

If an onChange handler replaces every "x" with an "abc", then when you type "x", we trigger onChange from beforeinput and rerender immediately with the content "abc", then the browser still inserts an "x" character!

This fix is surprisingly simple: instead of triggering an update during beforeinput, we simply wait until the input event to do so. This means that when we rerender, the browser would have already inserted the "x" character and React will correctly delete it when rerendering.

Test Plan: In plaintext example, change the handler to

```
this.onChange = (editorState) => {
  var content = editorState.getCurrentContent();
  return this.setState({
    editorState: EditorState.set(editorState, {
      currentContent: Modifier.removeInlineStyle(content, content.getSelectionAfter(), 'OVERFLOW');
    }),
  });
};
```

then type. Characters don't get duplicated like they did before.
sophiebits added a commit that referenced this issue Dec 6, 2016
Fixes #92.

If an onChange handler replaces every "x" with an "abc", then when you type "x", we trigger onChange from beforeinput and rerender immediately with the content "abc", then the browser still inserts an "x" character!

This fix is surprisingly simple: instead of triggering an update during beforeinput, we simply wait until the input event to do so. This means that when we rerender, the browser would have already inserted the "x" character and React will correctly delete it when rerendering.

Test Plan: In plaintext example, change the handler to

```
this.onChange = (editorState) => {
  var content = editorState.getCurrentContent();
  return this.setState({
    editorState: EditorState.set(editorState, {
      currentContent: Modifier.removeInlineStyle(content, content.getSelectionAfter(), 'OVERFLOW');
    }),
  });
};
```

then type. Characters don't get duplicated like they did before.
acusti pushed a commit to brandcast/draft-js that referenced this issue Dec 14, 2016
…karchive#667)

Fixes facebookarchive#92.

If an onChange handler replaces every "x" with an "abc", then when you type "x", we trigger onChange from beforeinput and rerender immediately with the content "abc", then the browser still inserts an "x" character!

This fix is surprisingly simple: instead of triggering an update during beforeinput, we simply wait until the input event to do so. This means that when we rerender, the browser would have already inserted the "x" character and React will correctly delete it when rerendering.

Test Plan: In plaintext example, change the handler to

```
this.onChange = (editorState) => {
  var content = editorState.getCurrentContent();
  return this.setState({
    editorState: EditorState.set(editorState, {
      currentContent: Modifier.removeInlineStyle(content, content.getSelectionAfter(), 'OVERFLOW');
    }),
  });
};
```

then type. Characters don't get duplicated like they did before.
ouchxp pushed a commit to ouchxp/draft-js that referenced this issue Apr 7, 2017
…karchive#667)

Fixes facebookarchive#92.

If an onChange handler replaces every "x" with an "abc", then when you type "x", we trigger onChange from beforeinput and rerender immediately with the content "abc", then the browser still inserts an "x" character!

This fix is surprisingly simple: instead of triggering an update during beforeinput, we simply wait until the input event to do so. This means that when we rerender, the browser would have already inserted the "x" character and React will correctly delete it when rerendering.

Test Plan: In plaintext example, change the handler to

```
this.onChange = (editorState) => {
  var content = editorState.getCurrentContent();
  return this.setState({
    editorState: EditorState.set(editorState, {
      currentContent: Modifier.removeInlineStyle(content, content.getSelectionAfter(), 'OVERFLOW');
    }),
  });
};
```

then type. Characters don't get duplicated like they did before.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants