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?

• Used MutationObserver to detect whenever the editor is attached to DOM again.
• Based on that information, recreate the entire editable section. In the case of IE, I have to wait until load event. So only for IE, there is an extra flag used to mark, that editable should be recreated.
• Added extra flag which preserves iframe element during editable detach process. Used only internally in recreating.
• Added config option (detachableParent) which allows pass native DOM object which should be observed for detach / attach changes
• Added new editor.status (recreating) which prevents null references in recreating the process
• Added manual & unit tests

We won't be able to support IE below 11, because of MutationObserver usage. However, it's mostly for integrations, which doesn't support IE below 11.

Three units test are passing regardless of this fix:

• 'test reattached editor makes editor data empty with observed invalid DOM object'
• 'test reattached editor parent makes editor data empty with observed invalid DOM object'
• 'test reattached editor parent makes editor data empty with observed detached parent element'

They confirm if iframe reloading still makes the editor empty. Also, they show that an invalid configuration still leads to an empty editor.

Fix two tests that create query strings for querySelector() with restricted characters (IE):

• https://github.com/ckeditor/ckeditor4/pull/4601/files#diff-a15eeadc2752af4c0b9dcfc057fb5fe450dfb6cf9a672e4fc9baacaac9036903
• https://github.com/ckeditor/ckeditor4/pull/4601/files#diff-64582745fd35627ba47ea103cea4f90242ccede20a04a22c9c7311393b50dba7

Which issues does your PR resolve?

Closes #4462

[sculpt0r]

@ckeditor/qa-team Hello there 👋 
Could you look at this feature again? The previous solution (#4481) was discarded and we came up with something better.

Note that it won't work for IE below 11.

There is a manual test with some additional plugins (tests/plugins/wysiwygarea/manual/detachedwithplugins) which may be helpful during some monkey clicking.

[LukaszGudel]

I've tested this feature using provided manual test and everything seems to work correctly 👍 I did not find anything broken during my testing.

[Dumluregn]

As for the solution, it looks much clearer now with mutationObserver (no custom attributes on iframe native element, yay!). It looks to work just like before 👍🏻

Besides inline comments, I'd like to ask for explicit tests for the cases that were failing in the previous version of the solution, so the embed test and an image with undo test (and BTW I see the second one is still failing - could you relate to it since you didn't in the original PR?).

[sculpt0r]

@Dumluregn - ready for review.

• added missing tests from the previous PR
• correct inline comments

I try to fix this additional step in history - but without success... TBH I was able to fix it, but at the same time undo was broken if I toggle editor before insert image - so still wrong behaviour - even worse. As you write - could we extract it as another issue? But in this case, we will have one failing manual test.

[Dumluregn]

Just some small refactors left.

Besides that - on IE11 I've encountered an error after each toggle:

Looks like Bender issue TBH 🤔 could you verify if it's reproducible outside of testing environment? I didn't experience any influence on editor (all tests pass), but in manual descriptions we write that any error causes the test to fail, so we should take some action.

[sculpt0r]

Rebase on newest major

[sculpt0r]

Besides that - on IE11 I've encountered an error after each toggle:

Definitely, it's related to our solution. Because in unit tests which expect empty editor (so didn't call restore logic) this issue doesn't appear for me in IE11. All other units from detached throw this error... 🤔

Interesting... I used the debugger and go line, by line... This error appears after this line:

ckeditor4/plugins/wysiwygarea/plugin.js

Line 478 in bc88d4e

doc.write( data );

Then it leads to sth like this:

And this is the source of our error :] This block code comes from

ckeditor4/plugins/wysiwygarea/plugin.js

Lines 435 to 444 in bc88d4e

	var bootstrapCode =
	'<script id="cke_actscrpt" type="text/javascript"' + ( CKEDITOR.env.ie ? ' defer="defer" ' : '' ) + '>' +
	'var wasLoaded=0;' + // It must be always set to 0 as it remains as a window property.
	'function onload(){' +
	'if(!wasLoaded)' + // FF3.6 calls onload twice when editor.setData. Stop that.
	'window.parent.CKEDITOR.tools.callFunction(' + this._.frameLoadedHandler + ',window);' +
	'wasLoaded=1;' +
	'}' +
	( CKEDITOR.env.ie ? 'onload();' : 'document.addEventListener("DOMContentLoaded", onload, false );' ) +
	'</script>';

The easiest way is to just suppress the error by adding window.parent.CKEDITOR &&. Kind of a workaround for now, but since this PR is long enough - maybe we can use it? As you said:

I didn't experience any influence on editor (all tests pass),

So it could be a solution for now. It doesn't break anything (at least for now 🙈 ) and this is happening only for IE11 is iframe reloading case - so eventually damage seems to be low?

[sculpt0r]

@Dumluregn - ready for review.

• rebased on the newest major
• correct all inline comments
• Suppress IE11 error - please take a look at Support editor functions after reattach to DOM #4601 (comment)

[Dumluregn]

I've encountered an issue with undo history which occurs on all browsers and is rather simple to reproduce, so we have to take care of it.

1. Open any manual test.
2. Toggle source mode.
3. Toggle editor (detach+reattach).
4. Use bold button.

Editor doesn't record any undo step. Even if it looks like it does (after you type anything), you can't access it.

Other than that - mostly small ammendments.

[sculpt0r]

@Comandeer - ready for another review.

• Added unit test for getSelection()
• Fix failing tests. They failed because the previous test has still an active observer (the default observed element is the entire document). So now - the editor is destroyed at the end of the test.

[Comandeer]

LGTM! 🎉

I've just updated API docs for CKEDITOR.editor#getSelection() (it seems that we have them wrong for years 🙈) and simplify it a little bit (replaced editable.editor.status with this.status).

I've also added additional changelog entry for the change in CKEDITOR.editor#getSelection() under API Changes.