Fixed: for #5162, The beginning of a TextNode with canInsertTextBefore false in … #5363
Conversation
…Asian languages It is not replaced when it is replaced
matsuyama-k1
requested review from
zurfyx,
fantactuka and
acywatson
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
|
When multiple strings are selected at the beginning of a TextNode where $canInsertTextBefore is set to false, and input is made in languages that use composing input methods, such as Japanese and Korean, the text input is not reflected, and the editor does not accept further input until Enter is pressed. I have addressed this issue. The problem may arise because, in languages that use composing, a COMPOSITION_START_CHAR is automatically entered, resulting in what is interpreted as a two-character input. As I understand it, the onInput function is triggered when a node is created and a second character is input. However, in languages like Japanese, it activates with a single character input. Therefore, canInsertTextBefore might be inadvertently engaged. Directly preventing the problem when two characters are input was not feasible for me, so I added a condition to the if statement to address this recent issue. |
There was a problem hiding this comment.
Thanks!
From a correctness perspective this PR is not great, the canInsertBefore was meant to prevent any input at boundaries which is what you're doing here.
Unfortunately, composition is not flexible, and we can't reposition the cursor when that happens, and with the MutationObserver logic combined is the reason why you're not seeing any text.
I believe the best solution would be to swap DOM nodes but that is something we don't support now and might get tricky with custom extended nodes.
Given that this change improves a11y, I'm inclined to accept it despite the correctness trade-off.
| @@ -650,7 +650,8 @@ export function $updateTextNodeFromDOMContent( | |||
| prevSelection.anchor.offset === 0) || | |||
| (prevSelection.anchor.key === textNode.__key && | |||
| prevSelection.anchor.offset === 0 && | |||
| !node.canInsertTextBefore()) || | |||
| !node.canInsertTextBefore() && | |||
| !isComposing) || | |||
There was a problem hiding this comment.
Can we do the same on L657 for canInsertTextAfter?
There was a problem hiding this comment.
I have inserted the line for canInsertTextAfter and confirmed that the jest tests run correctly. Additionally, I attempted to run the npm run test-e2e-chromium command for end-to-end testing, but the execution time was excessively long, preventing the tests from completing. I haven't encountered any specific scenarios that cause bugs when the line is not inserted within canInsertTextAfter, but it appears to function properly when the line is included.
There was a problem hiding this comment.
Makes sense, yeah, some E2E tests are flaky and take multiple times to pass unfortunately
Fix for #5162