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

support multiple windows #5070

Merged
merged 8 commits into from
Feb 29, 2024
Merged

support multiple windows #5070

merged 8 commits into from
Feb 29, 2024

Conversation

robfig
Copy link
Contributor

@robfig robfig commented Sep 29, 2023

This commit fixes 4 issues

  1. The lastTextInputTimeStamp was not being updated on
    popout windows because the listener was not being added.

  2. The time origin of event.timestamp and performance.now
    was not the same without using the specific window.

  3. Event classes (KeyboardEvent, ClipboardEvent) are defined
    on each window, so instanceof checks need to check against
    the one defined on the window where the event originates.
    I converted these to use e.g. "clipboardData" in event instead.

  4. The rootElement.ownerDocument selectionchange listener
    needs to be added on each distinct document.

I encountered these bugs in our Electron app. 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.

I haven't found much documentation on multi-window setups online
besides this:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_realms

Fixes #5050

@vercel
Copy link

vercel bot commented Sep 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 20, 2024 9:30pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 20, 2024 9:30pm

@facebook-github-bot
Copy link
Contributor

Hi @robfig!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@@ -39,19 +39,27 @@ import {
const TEXT_MUTATION_VARIANCE = 100;

let isProcessingMutations = false;
let lastTextEntryTimeStamp = 0;
const lastTextEntryTimeStamp_ = new WeakMap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the practical use cases of tracking entry timestamp by window?

Copy link
Contributor Author

@robfig robfig Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event.timeStamp is milliseconds since the time origin of the window, as is performance.now(), it is NOT a unix timestamp or wall time.. I found that surprising too.

Different windows have different time origins, so the timestamp of a events in different windows are not comparable, and the performance object of the window where an event originated must be used to measure time elapsed from that timestamp. As a result, we need separate timestamps per window.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zurfyx do you mind reviewing this PR? It's not so many lines of code and fixes severe bugs for us. I have our version patched but would be great to remove the patch and be able to upgrade easily again. Thanks

@robfig
Copy link
Contributor Author

robfig commented Feb 12, 2024

Thank you for the review. I will look at merging in your comments and rebasing to get it current.

We have a multi-window Electron app, and we began by having each window be its own renderer process. That turned out to cause performance issues, and we found that single process + multiple windows was much faster / lighter. I'm afraid I don't have more information to share about it beyond what was in the description ^. The app I'm referring to is https://ro.am/

This commit fixes 3 issues

1. The lastTextInputTimeStamp was not being updated on
   popout windows because the listener was not being added.

2. The time origin of event.timestamp and performance.now
   was not the same without using the specific window.

3. Event classes (KeyboardEvent, ClipboardEvent) are defined
   on each window, so instanceof checks need to check against
   the one defined on the window where the event originates.
   There was an `objectKlassEquals` helper for this purpose already, so
   I updated a couple places to use it.

I encountered these bugs in our Electron app. 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.

I haven't found much documentation on multi-window setups besides this:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_realms
@robfig
Copy link
Contributor Author

robfig commented Feb 12, 2024

@zurfyx I rebased the PR on main and applied your comments in 0e141f9

@zurfyx
Copy link
Member

zurfyx commented Feb 13, 2024

Thank you for iterating on this! Seems like this test is consistently failing? [chromium] › packages/lexical-playground/__tests__/e2e/CopyAndPaste/html/TablesHTMLCopyAndPaste.spec.mjs:243:3 › HTML Tables CopyAndPaste › Copy + paste - Merge Grids

@robfig
Copy link
Contributor Author

robfig commented Feb 13, 2024

Thanks @zurfyx , it was because of a mistake I made when rebasing. Just pushed a commit which should fix the test. Thanks

: (event as ClipboardEvent).clipboardData;
if (
clipboardData != null &&
($isRangeSelection(selection) || $isTableSelection(selection))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to specify the type of selection now? We don't want to import @lexical/table here, we intentionally isolated this module so that you can only pay the bundle size cost when the product needs tables

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, we don't. The code it was originally based on had this condition, so I kept it. I see that it was subsequently removed. Removed it from my change as well.

@robfig
Copy link
Contributor Author

robfig commented Feb 29, 2024

Hi @zurfyx , gentle ping that this PR is fully updated for all comments you've made

Copy link
Contributor

@acywatson acywatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked to Gerard and he's good with this

@acywatson acywatson merged commit 3d5f788 into facebook:main Feb 29, 2024
37 of 44 checks passed
@robfig
Copy link
Contributor Author

robfig commented Feb 29, 2024

Woo hoo! Thanks guys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Lexical editors in multiple windows
4 participants