Add support for preventing native character insertion

[paulyoung]

@hellendag this intends to address the issue we discussed in #427 and #448.

The intended use is something like this:

onChange(editorState) {
  const contentState = editorState.getCurrentContent();
  const blockMap = contentState.getBlockMap();
  const newBlockMap = blockMap.map((contentBlock) => {
    // perform some updates here
    return contentBlock;
  });
  const newContentState = contentState.set('blockMap', newBlockMap);
  const newEditorState = EditorState.push(editorState, newContentState, 'insert-fragment');
  this.setState({
    editorState: EditorState.preventNativeInsertion(newEditorState, true),
  });
}

Please review and let me know if you have any feedback.

Thanks!

[paulyoung]

I'll fix this and force push.

[paulyoung]

Come to think of it, this probably doesn't need to be re-set every time onChange is called.

Instead, I think it could be set when providing the initial editorState in which case preventNativeInsertion: boolean could also be added to the Editor props for convenience.

[paulyoung]

FWIW – I considered an alternative approach which allowed setting a new prop on Editor:

handleBeforeNativeInput(chars: string): boolean

This would be called much like this.props.handleBeforeInput in editOnBeforeInput but around the same place as editorState.mustPreventNativeInsertion() is now.

Ultimately, I was ignoring the input chars and returning true based on whether I had previously updated editorState in onChange (I see now that perhaps I could have always returned true in my situation)

[sophiebits]

Once you set this, it's on forever? Does it make sense to automatically set it to false after an update?

[paulyoung]

Once you set this, it's on forever?

Currently, whatever value is set remains set until it is explicitly changed. I'm not at all opposed to that changing.

Does it make sense to automatically set it to false after an update?

@spicyj where do you think that should happen?

[sophiebits]

Not sure, sorry. Just seemed weird from a first glance.

[paulyoung]

@hellendag (and @spicyj) – if this at least inspires a more desirable solution from either of you I'd be happy to work on that if I could get some direction.

[nikgraf]

@paulyoung What about having a fourth argument in EditorState.push which by default is false? This way you don't have to deal we automatic resets.

I wonder what implications this has for the undo/redo history. Have you tried them in combination with this change?

[paulyoung]

@nikgraf – thanks for the input. I really appreciate it.

I wonder what implications this has for the undo/redo history. Have you tried them in combination with this change?

With the current changeset, it does appear that things aren't fully taken care of. It seems that:

• both the native and programatic insertion still create an entry (2 items in the stack for 1 change)
• it's not possible to undo (and likely redo)

What about having a fourth argument in EditorState.push which by default is false? This way you don't have to deal we automatic resets.

I'll explore that tomorrow. Thanks again for the idea!

[paulyoung]

• it's not possible to undo (and likely redo)

To clarify – ⌘ + z doesn't do anything

[paulyoung]

I just pushed a couple of commits in line with @nikgraf's comments.

I still had to update the original code example to update the selection state:

onChange(editorState) {
    const contentState = editorState.getCurrentContent();
    const selectionState = editorState.getSelection();
    const blockMap = contentState.getBlockMap();
    const newBlockMap = blockMap.map((contentBlock) => {
      // perform some updates here
      return contentBlock;
    });
    const newContentState = contentState.set('blockMap', newBlockMap);
    const newEditorState = EditorState.push(editorState, newContentState, 'insert-fragment', true);
    this.setState({
      editorState: EditorState.acceptSelection(newEditorState, selectionState),
    });
  }

This almost works as expected, the only exceptions being a couple of anomalies with the cursor position on undo/redo.

If anyone could take a look and has any advice, I'd really appreciate it.

Once again, my use case is described in #448 (comment)

[paulyoung]

@hellendag – I'd love to keep this going and arrive at an acceptable solution, but think I may need some help/input from you (or someone who has more experience with Draft.js) to get there.

If there's any way I can make that easy for you, or you have any suggestions, please let me know.

[hellendag]

I don't think this belongs on EditorState. This setting concerns the component and its event management, not the model. This seems like it belongs as a prop on the component.

[sophiebits]

It seems similar to forceSelection to me – don't you think?

[hellendag]

It seems similar to forceSelection to me – don't you think?

Interesting point. EditorState does encode a handful of properties that are based on the behavior of the rendered editor rather than being purely model state. So maybe it's fine, though in hindsight it feels awkward and I wonder if there's a better way to represent that kind of state. Oh well.

A question on usage, whether it's a state value or a prop: how will I know when to set this? I need to set it before onBeforeInput fires, but how can I be sure when I am going to need it? Would one typically just turn it on and leave it on?

FWIW – I considered an alternative approach which allowed setting a new prop on Editor:

This actually sounds like a pretty good approach to me. The function prop doesn't handle the insertion, but it determines whether the value should be allowed natively. So that way, rather than setting a boolean value in the overall state of the editor, you would essentially perform filtering on values as they arrive, or use some other information to determine how to handle the character. A contrived example:

allowNativeInsertion(chars: string): boolean {
  if (chars === 'c') {
    // we want to handle `c` manually, no native insertion allowed
    return false;
  }
  return true;
}

// within your `Editor`
<Editor
  ...
  allowNativeInsertion={allowNativeInsertion}
  editorState={editorState}
/>

// within `onBeforeInput`
if (
  !this.props.allowNativeInsertion(chars) ||
  ...
) {
  // handle manually
  return;
}

This seems okay to me, and it gives you an opportunity to selectively decide how to manage the incoming insertion.

[paulyoung]

Thanks for the thoughtful replies! I'll go back and try this approach this afternoon.

[paulyoung]

I've removed the functionality around this that I added to EditorState and added a function called allowNativeInsertion to the Editor props as discussed.

The way this is called in editOnBeforeInput may require some discussion.

This appears to work well except for undo/redo again. I attempted to handle this in 30791d6 via EditorState but now I'm not sure how to approach it.

Now, in conjunction with the code in my original comment #453 (comment), items on the undoStack have a blockMap where the text is an empty string ("").

[paulyoung]

I may have bandwidth to take a stab at fixing undo/redo with the latest changes soon.

If anyone has any ideas, I'd love to hear them!

[paulyoung]

I took a quick look at this again and I'm having a bit of trouble seeing how to affect undo/redo now that EditorState isn't involved.

[rygine]

This issue comes up a lot in our Slack. Would be nice to have this implemented soon.

[hellendag]

@paulyoung Would you mind clarifying a bit what the undo/redo issues are for your current solution?

When you perform an undo, what happens? Is the issue that the ContentState with the character insertion becomes an unwanted boundary state?

I appreciate the simplicity of this change, so it would be great to get this to work. :)

[paulyoung]

@hellendag I decided to first examine the undo stack before applying the changes in this pull request.

This appears to exhibit the same behavior, so while it doesn't seem like a problem caused by the code I've introduced it's a problem that needs to be addressed somehow nonetheless.

Below is the onChange handler I used to log the undo stack. I decided that selection state wasn't relevant to the discussion just yet.

_onChange(editorState) {
    const contentState = editorState.getCurrentContent();
    const selectionState = editorState.getSelection();
    const blockMap = contentState.getBlockMap();
    const newBlockMap = blockMap.map((contentBlock) => {
      // perform some updates here
      return contentBlock;
    });
    const newContentState = contentState.set('blockMap', newBlockMap);
    const newEditorState = EditorState.push(editorState, newContentState, 'insert-fragment', true);
    console.log(JSON.stringify(newEditorState.toJS().undoStack.map((x) => x.blockMap), null, 2));
    this.setState({
      editorState: EditorState.acceptSelection(newEditorState, selectionState),
    });
  }

If there's a way that I can change this to "fix" the undo/redo stack that would be great, although I think a baked-in solution that "just works" would be nice.

Here is what was logged for each action (expand to view):

Initial page load

[
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  }
]

Focus

[
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  }
]

Type "a"

[
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "a",
      "characterList": [
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  }
]

Type "b" (note that "b" is repeated because my changes aren't applied)

[
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "ab",
      "characterList": [
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "a",
      "characterList": [
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "a",
      "characterList": [
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  }
]

[
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "abb",
      "characterList": [
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "ab",
      "characterList": [
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "ab",
      "characterList": [
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "a",
      "characterList": [
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "a",
      "characterList": [
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  }
]

Blur

[
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "abb",
      "characterList": [
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "abb",
      "characterList": [
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "ab",
      "characterList": [
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "ab",
      "characterList": [
        {
          "style": [],
          "entity": null
        },
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "a",
      "characterList": [
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "a",
      "characterList": [
        {
          "style": [],
          "entity": null
        }
      ],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  },
  {
    "5msnk": {
      "key": "5msnk",
      "type": "unstyled",
      "text": "",
      "characterList": [],
      "depth": 0
    }
  }
]

[paulyoung]

It seems to me that since the approach taken with the latest changes doesn't support a way to address the issues with the undo/redo stack, I should revert to the forceSelection-style method I tried earlier and see if I can figure out undo/redo from there.

[hellendag]

Still not completely sure I understand, but it seems like in the logged output, for each step there is an extra ContentState object in the undo stack that it would be preferable not to have. Am I understanding correctly? Is it that you would like to discard the top of the undo stack before pushing on the new ContentState in your onChange?

[paulyoung]

I'm going to get back to this soon, I promise 🙇

[paulyoung]

Quick update on this – discarding the top of the undo stack does appear to address the issues I was having with duplicate characters, when paired with this changes in this branch.

I still can't get undo/redo to work, although that appears to be a problem with the way I'm handling onChange, so going to look into that.

[sophiebits]

My #667 should fix this cleanly so let's go with that.

[paulyoung]

Oh, awesome @spicyj! Would love to see this land soon!

[intentionally-left-nil]

@paulyoung I still would like this PR to go through. Half of the point of draftjs is that contenteditable is essentially unspecced. I am very hesitant to rely on the browser to do the right thing here. I would prefer to at the very minimum give the ability for the caller to disable native behavior.