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

CJK Input stuttering with dispatchTransaction #41

Open
bentongxyz opened this issue Jun 25, 2023 · 13 comments
Open

CJK Input stuttering with dispatchTransaction #41

bentongxyz opened this issue Jun 25, 2023 · 13 comments

Comments

@bentongxyz
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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

@totorofly
Copy link

totorofly commented Nov 10, 2023

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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
Copy link

totorofly commented Nov 10, 2023

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
Copy link
Collaborator

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

@totorofly
Copy link

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
Copy link

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
Copy link
Collaborator

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
Copy link
Collaborator

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants