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

Bug: Lexical editors in multiple windows #5050

Closed
robfig opened this issue Sep 25, 2023 · 9 comments · Fixed by #5070
Closed

Bug: Lexical editors in multiple windows #5050

robfig opened this issue Sep 25, 2023 · 9 comments · Fixed by #5070

Comments

@robfig
Copy link
Contributor

robfig commented Sep 25, 2023

Steps To Reproduce

I'm not sure how to make an easy reproduction, but I noticed this in our app which pops out a DOM-only Electron window. It opens a new window via window.open() and renders to it from the parent window. As a result, there are multiple window objects that can host Lexical editors and one JS environment.

The current behavior

Lexical tracks lastTextEntryTimeStamp in a way that does not account for multiple windows over the lifetime of the application. It adds an event listener to the first window and not subsequent ones.

let lastTextEntryTimeStamp = 0;

function updateTimeStamp(event) {
  lastTextEntryTimeStamp = event.timeStamp;
}

function initTextEntryListener(editor) {
  if (lastTextEntryTimeStamp === 0) {
    getWindow(editor).addEventListener("textInput", updateTimeStamp, true);
  }
}

There were a number of buggy behaviors which resulted from this, due to applying input mutations during beforeinput. The most obvious bug was the MarkdownShortcutPlugin would duplicate the closing tag.

The expected behavior

Lexical should support editors in multiple windows.

@robfig
Copy link
Contributor Author

robfig commented Sep 26, 2023

I patched lexical 0.12.2 to fix it like this:

diff --git a/node_modules/lexical/Lexical.dev.js b/node_modules/lexical/Lexical.dev.js
index bcc05e3..db20147 100644
--- a/node_modules/lexical/Lexical.dev.js
+++ b/node_modules/lexical/Lexical.dev.js
@@ -197,18 +197,26 @@ const TEXT_TYPE_TO_MODE = {
 
 const TEXT_MUTATION_VARIANCE = 100;
 let isProcessingMutations = false;
-let lastTextEntryTimeStamp = 0;
+const lastTextEntryTimeStamp_ = new WeakMap();
 function getIsProcesssingMutations() {
   return isProcessingMutations;
 }
 
-function updateTimeStamp(event) {
-  lastTextEntryTimeStamp = event.timeStamp;
+function lastTextEntryTimeStamp(window) {
+  return lastTextEntryTimeStamp_.get(window);
+}
+
+function updateTimeStamp(window) {
+  return function(event) {
+    lastTextEntryTimeStamp_.set(window, event.timeStamp);
+  }
 }
 
 function initTextEntryListener(editor) {
-  if (lastTextEntryTimeStamp === 0) {
-    getWindow(editor).addEventListener('textInput', updateTimeStamp, true);
+  const window = getWindow(editor);
+  if (!lastTextEntryTimeStamp_.has(window)) {
+    lastTextEntryTimeStamp_.set(window, window.performance.now())
+    window.addEventListener('textInput', updateTimeStamp(window), true);
   }
 }
 
@@ -257,7 +264,8 @@ function shouldUpdateTextNodeFromMutation(selection, targetDOM, targetNode) {
 
 function $flushMutations$1(editor, mutations, observer) {
   isProcessingMutations = true;
-  const shouldFlushTextMutations = performance.now() - lastTextEntryTimeStamp > TEXT_MUTATION_VARIANCE;
+  const window = getWindow(editor)
+  const shouldFlushTextMutations = window.performance.now() - lastTextEntryTimeStamp(window) > TEXT_MUTATION_VARIANCE;
 
   try {
     updateEditor(editor, () => {

@robfig
Copy link
Contributor Author

robfig commented Sep 26, 2023

Another multi-window issue, this time in LexicalLinkPlugin

event instanceof ClipboardEvent

returns false, I suppose because ClipboardEvent refers to the first window's class. This fixes the check:

event instanceof editor._window.ClipboardEvent

This is all fairly surprising to me, to be honest.

@robfig
Copy link
Contributor Author

robfig commented Sep 26, 2023

@acywatson
Copy link
Contributor

This is an interesting one - thanks for reporting.

Would you be interested in submitting that patch as a PR?

@robfig
Copy link
Contributor Author

robfig commented Sep 29, 2023

Sure, opened #5070

@robfig
Copy link
Contributor Author

robfig commented Oct 3, 2023

Tests are passing. I did not add a new test for multi-window, though. I'm not sure how to do that

@robfig
Copy link
Contributor Author

robfig commented Oct 10, 2023

Hi @acywatson , any feedback on the PR? The only X is the deployment to lexical-playground, but I'm not able to see the issue there.

@robfig
Copy link
Contributor Author

robfig commented Nov 3, 2023

Hi @acywatson , I found and fixed another multi-window issue and added it to the PR. Can you please have a look? I think it's ready to go.

@robfig
Copy link
Contributor Author

robfig commented Nov 30, 2023

One more ping @acywatson after the holiday

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

Successfully merging a pull request may close this issue.

2 participants