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

Fix crash when enabling Watermark plugin #1221

Merged
merged 6 commits into from
Aug 30, 2022
Merged

Conversation

ianeli1
Copy link
Member

@ianeli1 ianeli1 commented Aug 30, 2022

Steps to repro:

  1. Open Rooster demo, don't type anything
  2. Open side panel
  3. Plugins
  4. Toggle Watermark Plugin

domToContentModel gets called with an undefined root, causing a crash.
image
Instead of using the experimental editor's contentDiv, we can access the editor core's directly, which seems to fix the problem.

@ianeli1 ianeli1 requested a review from JiuqingSong August 30, 2022 02:37
@JiuqingSong
Copy link
Collaborator

Do you know why the original way not work?

@JiuqingSong
Copy link
Collaborator

Although the change may fix the issue, but I'm afraid it also hides the real root cause.

@ianeli1
Copy link
Member Author

ianeli1 commented Aug 30, 2022

Do you know why the original way not work?

Fun debugging session 😄
It's a timing issue, we're assuming the properties of the experimental editor get assigned immediately

    constructor(private contentDiv: HTMLDivElement, options?: EditorOptions) { //this.contentDiv gets assigned
        super(contentDiv, options); //Editor gets initialized
        this.getDarkColor = options?.getDarkColor; //then this, all built
    }

In reality the super gets called first, which means the plugins get initialized, ending in the Lifecycle plugin emitting PluginEventType.EditorReady, which then triggers the Watermark plugin to show the watermark.
Showing the watermark changes the content and makes the editor create a new content model, which won't be possible as this.contentDiv hasn't been assigned yet (this all happens within the super call).

Conveniently, none of this happens if we have content in the editor:

} else if (!hasFocus && !isShowing && this.editor.isEmpty()) {
            insertEntity(

@JiuqingSong
Copy link
Collaborator

Do you know why the original way not work?

Fun debugging session 😄 It's a timing issue, we're assuming the properties of the experimental editor get assigned immediately

    constructor(private contentDiv: HTMLDivElement, options?: EditorOptions) { //this.contentDiv gets assigned
        super(contentDiv, options); //Editor gets initialized
        this.getDarkColor = options?.getDarkColor; //then this, all built
    }

In reality the super gets called first, which means the plugins get initialized, ending in the Lifecycle plugin emitting PluginEventType.EditorReady, which then triggers the Watermark plugin to show the watermark. Showing the watermark changes the content and makes the editor create a new content model, which won't be possible as this.contentDiv hasn't been assigned yet (this all happens within the super call).

Conveniently, none of this happens if we have content in the editor:

} else if (!hasFocus && !isShowing && this.editor.isEmpty()) {
            insertEntity(

I tried and I can repro. So it seems the problem is in ContentModel side pane plugin only. In that case, we can just to a null check in that plugin and if content DIV is not ready yet, just skip creating content model.

We'd like to hide the detail of editor as much as possible.

@ianeli1 ianeli1 merged commit d8f854b into master Aug 30, 2022
@JiuqingSong JiuqingSong deleted the u/ianeli/fix-watermark-toggle branch October 31, 2022 19:23
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 this pull request may close these issues.

2 participants