Support editor functions after reattach to DOM

[sculpt0r]

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

• Unit tests
• Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

• PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4462](https://github.com/ckeditor/ckeditor4/issues/4462): Support editor functions after reattaching to DOM.

What changes did you make?

Give an overview…

Which issues does your PR resolve?

Closes #4462 .

[sculpt0r]

@ckeditor/qa-team Could you please look at this?
There is a main manual test at: http://0.0.0.0:1030/tests/plugins/wysiwygarea/manual/detached
The main reason, why it may fail is an iframe that may receive onload event and then tries to recreate the edit area.

Also, after recreation the edit area: random functions may fail, like selection etc.

For sure: any IE will fail (including 11) - so there is no need to verify this browser for now. Edge should be fine.

Please ask me anything if some more explanation is needed.

[sculpt0r]

I go through many issues related to Permission denied IE and the best I've figured out for now simply ignore it also on IE. It is already ignored on Edge.

Without an opened console, it should work also on IE.

[LukaszGudel]

On Safari toggling the editor in some cases removes content and makes the editor unfocusable. 

Steps to reproduce:

Scenario 1:

1. Open the manual test in Safari.
2. Double click "Toggle" button.

CKE4_detached_safari_scenario1.mp4

---

Scenario 2:

1. Open the manual test in Safari.
2. Add some content.
3. Double click "Source" button.
4. Double click "Toggle" button.

CKE4_detached_safari_scenario2.mp4

---

Scenario 3:

1. Open the manual test in Safari.
2. Click on "Set Data" button.
3. Double click "Toggle" button.

CKE4_detached_safari_scenario3.mp4

---

Result:

The content of the editor is missing and the editor is unfocusable. Toggling the editor once again or toggling source mode is fixing the issue.

• My Safari version: 14.0.2
• My OS: Big Sur 11.1
• There are no errors in the console

[sculpt0r]

Rebase onto newest major.

[LukaszGudel]

There still some weird things happening on Safari. Redoing steps from my previous comment results in blinking content and high CPU usage:

CKE4-detached_safari_4462-1.mp4

Edit: This behavior was caused by some cache in the browser. I've tested this case on a few different devices and everything is working correctly.

[sculpt0r]

@LukaszGudel - thank you. This is something I just can't catch on BrowserStack...

[LukaszGudel]

I've retested this feature on all browsers and everything seems to work correctly 👍

[sculpt0r]

CI - green, QA -verified on various browsers, so do I.
Code was cleaned, ready for the real review.

[github-actions]

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

[Dumluregn]

Editor can be detached and reattached with the same content, also undo steps are preserved, also integrations with Angular, React and Vue still work, so 👍🏻 Especially that the fix doesn't look very obvious.

OTOH, in manual test if I detach editor and then click setData() button, I've got an error. So I'm wondering - is this expected (if yes then we should rename this issue to indicate that editor can be detached and reattached, but not suggest that it works when detached) or not (and we still have to fix it). I guess either of you @sculpt0r and @f1ames can specify this as you together decided to extract #4462 so please clarify what is the goal of this issue.

Did you encounter some issues trying to write unit tests? I think they would be useful here to easily check some cases with executing commands (TBH manual test could also be automated, but it is also useful as is since you can control more things about editor's behaviour).

As for the manual tests, I'd like to have another one checking if undo steps are preserved (they are now so 👍🏻 ). So the steps would be something like:

1. Type anything.
2. Change formatting.
3. Detach.
4. Reattach.
5. Check if there are two undo steps and you can go back to the beginning.

Besides that - please take a look at inline comments.

[Dumluregn]

Some of the comments may be hidden like this:

Please make sure you find them all 🔍

[sculpt0r]

OTOH, in manual test if I detach editor and then click setData() button, I've got an error.

That's something we know about. Truth is, the original issue should be called: Support editor functions after reattach to DOM. So I also renamed #4462 . We talked with @f1ames about supporting editor even when it's detached, but it's a really huge feature. It will require a lot of changes inside the core, which partially operates on document search. After detach it could be hard to workaround. 
This PR mainly should fix a few issues mentioned in #4462 .

[sculpt0r]

@Dumluregn - CI is still green - so ready for another review :)

[Dumluregn]

We talked with @f1ames about supporting editor even when it's detached, but it's a really huge feature.

Yeah, I can imagine the amount of work to fix all issues with selection etc., so it makes sense 👍

@sculpt0r could you please also refer to this part of my comment:

Did you encounter some issues trying to write unit tests? I think they would be useful here to easily check some cases with executing commands (TBH manual test could also be automated, but it is also useful as is since you can control more things about editor's behaviour).

[Dumluregn]

The code needs only a little bit of polish, good job 👍🏻 We could use some more testing though.

As for the unit tests, we already talked and agreed that we want to have at least the basic scenarios covered.

As for the manuals, as I've mentioned at the end of the previous review, we need another test for undo and perhaps some other core integrations (like elementspath, formatting, links etc.). Besides that, we need to cover also other editor types (divarea and inline). I've briefly tested them and they seem to work, but still separate tests for each of them is a must (just copy detached test and change editor type - don't add more editors there as it already has a long test description). And also a test with two editors where we reattach one of them to check if the correct data is bound to it.

So to sum up, we want to have the following manual tests:

1. General test for reattaching and preserving data - already covered in detached 👍🏻
2. Same test as 1 for divarea editor.
3. Same test as 1 for inline editor.
4. Test for undo integration with classic editor.
5. Test for two editors in the same page and reattaching one of them.
6. Open test (kind of monkey clicking) for the integration of at least part of the editor features.

If you think that some of those scenarios would be a good fit for a unit test instead, feel free to propose it.

[sculpt0r]

Added requested changes:

• Manual detached test for divarea editor
• Manual detached test for inline editor
• Manual undo test: 78e97e8
• Manual test for two editor instances (wysiwygarea plugin). I'm not sure what exactly we want to test here?
• Open 'monkey clicking' test with some plugins that might be somehow affected by editable recreation. (3f09942)

Did you encounter some issues trying to write unit tests? I think they would be useful here to easily check some cases with executing commands (TBH manual test could also be automated, but it is also useful as is since you can control more things about editor's behaviour).

I added an automatic test for the detached wysiwygarea editor. I have to use some tricky-reference to elements combination. Also, it's impossible to just use casual CKEditor API, because sometimes at API-level all data are correct, but not displayed to the user properly. That is why I go more into manual tests. Also, I have to add this ugly setTimeout before iframe data verification. If I skip iframe data is not ready after recreation. Also, I remember, that at the very beginning: there were situations that the editor contains proper data and all attributes - but is not editable.
 

• Fixed all code styles issues (also comments)

  Ready for review.

[Dumluregn]

Rebased onto latest major. Also I've added the note to observe the undo plugin to the manual test with plugins.

[Dumluregn]

@sculpt0r I hate to say it but... we still have work to do here...

I tried to reproduce the errors mentioned by @LukaszGudel and I didn't, but found more two issues during the monkey clicking.

Scenario 1)

1. Embed an image (using dialog or autoembed).
2. Toggle source mode twice.

The whole webpage freezes. And as this happens even without detaching the editor, we have to fix it.

Scenario 2)

1. Insert image to editor.
2. Detach and reattach editor.

This creates one dummy undo step which doesn't change the content. I would say it isn't something critical and only happens after toggling the editor, so if the fix would take more than 1/2 MD then let's extract this as a separate issue.

PS I've pushed some changes so remember to pull the latest branch.

[sculpt0r]

Scenario 1)

Looks like fullPage config set to true causing problems with recreating. I found that an additional iframe in content is a problem.

This for loop

ckeditor4/core/htmlparser/fragment.js

Line 107 in 05e2b11

for ( var i = 0; i < pendingInline.length; i++ ) {

ends as infinite. It stuck on else

ckeditor4/core/htmlparser/fragment.js

Line 131 in 05e2b11

} else {

 
The hardest part: why adding onload to iframe causes crash here 🗡️

ckeditor4/core/htmlparser/fragment.js

Line 93 in 05e2b11

var root = parent instanceof CKEDITOR.htmlParser.element ? parent : typeof parent == 'string' ? new CKEDITOR.htmlParser.element( parent ) : new CKEDITOR.htmlParser.fragment();

In a normal situation root.name is body. With this fix, root.name is textarea
Funny thing, there is no error if I try to use a debugger - which leads me to time-related issues...

Additional iframe inside content causes extra load event for the main iframe. So if there is additional iframe (like one from embedded image) In the beginning - currently - we are recreating editable for no reason

[github-actions]

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

[sculpt0r]

Closing due to:

• long unnecessary commits history
• change resolution to MutationObserver

This solution works in IE < 11, but was error-prone (due to relying on load events mixed with lots of flags). This feature is mainly required by integrations, so support IE11 is enough for that. The PR with a new solution: #4601