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.

Sign up for GitHub

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

Jump to bottom

Links added to wrong place when DecoupledEditor is attached to an iframe #12400

Closed
semiaddict opened this issue Sep 2, 2022 · 9 comments
Closed

Links added to wrong place when DecoupledEditor is attached to an iframe #12400

semiaddict opened this issue Sep 2, 2022 · 9 comments
Labels
resolution:expired This issue was closed due to lack of feedback. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@semiaddict
Copy link

📝 Provide detailed reproduction steps (if any)

  1. Copy the following reproduction snippet to an html file:
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <title>CKEditor 5 – Document editor</title>
    <script src="https://cdn.ckeditor.com/ckeditor5/35.0.0/decoupled-document/ckeditor.js"></script>
</head>
<body style="margin: 50px;">
    <h1>Document editor</h1>
    <div id="toolbar-container"></div>
    <iframe src="about:blank" style="width: 100%;"></iframe>

    <script>
        const iframe = document.querySelector("iframe");
        const iframeDocument = iframe.contentDocument;
        const onLoad = function() {
            const iframeDocument = iframe.contentDocument;
            const iframeBody = iframeDocument.body;

            iframeBody.style.margin = "0";
            iframeBody.style.height = "100%";

            const div = document.createElement("div");
            div.classList.add("editor");
            div.style.height = "100%";
            iframeBody.appendChild(div);

            DecoupledEditor
                .create( div )
                .then( editor => {
                    const toolbarContainer = document.querySelector( '#toolbar-container' );

                    toolbarContainer.appendChild( editor.ui.view.toolbar.element );
                } )
                .catch( error => {
                    console.error( error );
                } );
        };

        if (iframeDocument.readyState === "complete") {
            onLoad();
        } else {
            iframe.addEventListener("load", onLoad);
        }
    </script>
</body>
</html>
  1. Open html page in FireFox
  2. Type some text
  3. Select a portion of the text
  4. Add a link to the selected text using the link button

✔️ Expected result

The link is added on the selected text.

❌ Actual result

The link is prepended to the paragraph with the URL as its text.

firefox_cJXkqoEHTH

📃 Other details

This used to work just fine with CKEditor 34.x, and seems to be broken in all 35.x versions.

  • Browser: FireFox 104.0.1
  • OS: Windows 10
  • First affected CKEditor version: v35.0.0

Chrome doesn't seem to be affected.

If you'd like to see this fixed sooner, add a 👍 reaction to this post.
@semiaddict semiaddict added the type:bug This issue reports a buggy (incorrect) behavior. label Sep 2, 2022
@semiaddict semiaddict changed the title Links added to wrong place when used in iframe Links added to wrong place when DecoupledEditor is attached to an iframe Sep 2, 2022
@semiaddict semiaddict mentioned this issue Sep 20, 2022
Closed
@semiaddict semiaddict mentioned this issue Nov 3, 2022
Closed
@semiaddict
Copy link
Author

After checking the differences between v34.2.0 and v35.0.0, I suspect the behavior described above might be caused by commit a0244b0 .
I can't seem to find any other changes between 34 and 35 that would cause this behavior.

@niegowski, can you please indicate why the "document" argument was removed from most calls in ckeditor5-engine ?
Do you think this could indeed lead to the behavior described here ?

Thank you for your help.

@semiaddict
Copy link
Author

After more investigation, it seems that the changes done in ckeditor5-engine are not the ones responsible for the bug.
I am trying to track down the source by slowly reverting changes, and will report back as soon as I get more information.

@semiaddict
Copy link
Author

I finally managed to pinpoint the source of the issue.
It is due to a small change done in 9908c72, which removed the "preventDefault" call on buttonview mousedown events.

This seems to alter the editor's selection when clicking on a toolbar button, by moving it to the beginning of the active paragraph (see below screen capture).

This time I am 100% sure it's caused by the above mentioned commit as I setup a manual test (attached below) and tested commits made between v34.2.0 (813e062) and 35.0.1 (19e6094) using a "binary search" methodology.

The test passes with commit 4f1f472, but fails with the one just after (9908c72).

@mmotyczynska, @oleq, what was the purpose of that commit?
Did it target a particular bug?

firefox_yU8kzNTzOk

decoupled-iframe-manual-test.zip

@Reinmar
Copy link
Member

Reinmar commented Nov 14, 2022

I'm sorry to hear that something's changed in your use case due to recent change made by us. However, we never wanted to support iframed-editables in CKE5. This was a nightmare thing to support in CKE4 and we specifically dropped that. It might've worked up to some point, but recently we worked more on focus management in our UI. It makes sense that this affected iframes because, in general, iframes create their own set of focus and selection related problems.

Out of curiosity, though, why do you use an iframe?

@semiaddict
Copy link
Author

Thank you for your response @Reinmar.

Out of curiosity, though, why do you use an iframe?

We are using CKEditor in metaScore, a fairly complex SPA that allows authors to edit interactive multimedia applications using an editor that embeds the application to be edited in an iframe to isolate it from the editor.

This has worked flawlessly until the introduction of that small change, so I'm wondering what the purpose of that change was.

@semiaddict semiaddict mentioned this issue Nov 26, 2022
Closed
@semiaddict
Copy link
Author

BTW, our use case could likely be dealt with using Shadow DOM, but there seems to be similar issues with that as well.
It's a pity that scoped CSS didn't survives, as that would have made things easier for such a use case!

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added resolution:expired This issue was closed due to lack of feedback. and removed status:stale labels Dec 29, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 29, 2023
@DEfusion
Copy link

@semiaddict and anyone else who hits this, it wasn't the removal of preventDefault that was causing the issue it's _showFakeVisualSelection in LinkUI. Luckily we were already extending LinkUI to create our own link plugin so I added this to prevent it running when the element is in an iframe (in our case the editing element is in the iframe with the controls & balloons in the parent window).

_showFakeVisualSelection() {
    if (this.editor.sourceElement.ownerDocument !== document) return;

    super._showFakeVisualSelection();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
resolution:expired This issue was closed due to lack of feedback. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@DEfusion @Reinmar @semiaddict @CKEditorBot