nytimes / react-prosemirror

A library for safely integrating ProseMirror and React.
Other
432 stars 15 forks source link

CJK Input stuttering with `dispatchTransaction` #41

Open bentongxyz opened 1 year ago

bentongxyz commented 1 year ago

I noticed that using the README.md example code:

import { ProseMirror } from "@nytimes/react-prosemirror";
import { useState } from "react";
import { EditorState } from "prosemirror-state";
import { schema } from "prosemirror-schema-basic";

export function ProseMirrorEditor() {
  const [mount, setMount] = useState<HTMLElement | null>(null);
  const [editorState, setEditorState] = useState(
    EditorState.create({schema})
  );

  return (
    <ProseMirror
      mount={mount}
      state={editorState}
      dispatchTransaction={(tr) => {
        setEditorState((s) => s.apply(tr));
      }}
    >
      <div ref={setMount} />
    </ProseMirror>
  );
}

the CJK input in browser is stuttering.

Screencast from 2023-06-26 02-37-59.webm

However, if I remove dispatchTransaction prop, i.e.

import { ProseMirror } from "@nytimes/react-prosemirror";
import { useState } from "react";
import { EditorState } from "prosemirror-state";
import { schema } from "prosemirror-schema-basic";

export function ProseMirrorEditor() {
  const [mount, setMount] = useState<HTMLElement | null>(null);
  const [editorState, setEditorState] = useState(
    EditorState.create({schema})
  );

  return (
    <ProseMirror
      mount={mount}
      state={editorState}
    >
      <div ref={setMount} />
    </ProseMirror>
  );
}

it behaves normally:

Screencast from 2023-06-26 02-38-52.webm

Any idea what is the source of problem?

smoores-dev commented 1 year ago

Ah, this is probably related to the IME composition issues that were brought up in #49? If so, I believe it should be resolved by the new architecture in #47! If you want to give it a shot, I published the latest as 0.3.0-beta.0 on npm.

totorofly commented 10 months ago

I conducted tests on version "@nytimes/react-prosemirror": "^0.3.0", and the issue still persists.

Ah, this is probably related to the IME composition issues that were brought up in #49? If so, I believe it should be resolved by the new architecture in #47! If you want to give it a shot, I published the latest as 0.3.0-beta.0 on npm.

smoores-dev commented 10 months ago

You'll have to specifically use "@nytimes/react-prosemirror": "0.3.0-beta.0" to get the new architecture!

totorofly commented 10 months ago

You'll have to specifically use "@nytimes/react-prosemirror": "0.3.0-beta.0" to get the new architecture!

image

I tried switching to version 0.3.0-beta.0. The input indeed became possible, but when I used an IME to input text and confirmed it with the space bar, the cursor remained in front of the entered text.

image image

Additionally, if I tried to move the cursor to the right, I found it couldn't move at all. Any attempt to select the text resulted in it being forcibly deselected, with the cursor always staying at the leftmost position of the entered text. Under these circumstances, if I turned off the IME and tried typing in English letters, an error occurred: 'Unhandled Runtime Error Not Found Error: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.'

image
smoores-dev commented 10 months ago

Darn, ok, thanks for the report! I'll take a look; it's possible that that version just doesn't have the latest code, because I did actually test with Japanese IME input specifically.

Actually, the latest code is deployed here: https://nytimes.github.io/react-prosemirror/. If you have a moment, would you give that a shot, and if you run into issues, could you take a screen recording of what you're seeing? I did just confirm that I can insert Japanese characters with IME input on my computer/browser at least.

totorofly commented 10 months ago

I am very willing to cooperate with testing. However, when I used the address you provided and turned off the IME to use the system's default input, entering any letter or number resulted in the following error:

image image
smoores-dev commented 10 months ago

Ah! I was able to reproduce this on Chrome; it seems to be a difference in how Chrome and Firefox handle... something! Not sure yet, but this is very helpful. I'll continue to dig into this; thank you for your assistance in pinpointing this!

totorofly commented 10 months ago

Ah! I was able to reproduce this on Chrome; it seems to be a difference in how Chrome and Firefox handle... something! Not sure yet, but this is very helpful. I'll continue to dig into this; thank you for your assistance in pinpointing this!

On the address you provided, whether I use IME or turn it off, I can input and output content normally on lines that already have text. I can insert a Chinese characters with IME input.

image
smoores-dev commented 10 months ago

Oh great, that's good to know! Starting a new line is definitely its own edge case in this situation

totorofly commented 10 months ago

Oh great, that's good to know! Starting a new line is definitely its own edge case in this situation

Based on the current test results, 1) if a new blank line is added by pressing Enter, the same error as mentioned above will occur, regardless of whether the IME is turned on or off, as long as there is input; 2) if a line break is made from the middle of the content in the second paragraph, creating a line with content, then no problem occurs.

totorofly commented 10 months ago

Is there going to be a new release coming soon? I've been using Tiptap, but I've found that it supports Vue better than React, and I'm using Next.js based on React. So, I've been looking for alternatives. If there's a new release, even a Beta version, could you provide a version for internal testing?

smoores-dev commented 10 months ago

That's the plan! But I can't make any promises about timing, unfortunately. We're still working out some significant high level decisions about the approach.

smoores-dev commented 10 months ago

Hey @saranrapjs, while you're at it: @ilyaGurevich took a stab at this a little while back but it seems like there are still issues with IME input in the main branch. I haven't sat down to try to think through this, but this seems to be another thing that works fine with NYT's custom implementation ("simple" node views) but isn't working here!