Set cardinality of the node body field to e.g. 8.
Go to node/add and try to rearange the body field items.
Before releasing a field item, hold it for 1-2 seconds.
Watch how JS expcetions are being thrown:

Error: uncaught exception: The editor instance "edit-body-1-value" is already attached to the provided element.

Comments

hchonov created an issue. See original summary.

hchonov’s picture

Issue summary: View changes
hchonov’s picture

Status: Active » Needs review
Issue tags: +Quick fix, +rc target triage
FileSize
981 bytes
Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I'll review this ASAP.

Wim Leers’s picture

Title: JS Exceptions are thrown each time a multiple field with ckeditor instances is being rearanged » JS exceptions are thrown each time a multiple-valued text field with a CKEditor instances is being rearanged
Issue tags: +JavaScript

Reproduced.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
941 bytes
468.62 KB

This appears to be happening because CKEDITOR.destroy is non-blocking.

I managed to reproduce it with just four values for the body field too. If you then drag and drop with the following patch applied (which just adds debug output), you can clearly see that an instance that was just destroyed is in fact not yet fully destroyed (it's still in CKEDITOR.instances) when it is re-created. See attached screenshot.

Pinged the CKEditor team for feedback.

hchonov’s picture

I think there is nothing wrong with the CKEDITOR.destroy being non-blocking. Why do we want to destroy and create each CKeditor instance on each table reorder event?

The worst is like you might see in your screenshot it happens more than once just after moving a single element one level up.

Should we update the editor.js detach behavior to listen to the trigger event "move" fired by tableDrag as well and on this event not detach the editors, because actually the DOM elements for the editors did not change?

I've done that in the patch, I am attaching now, but the problem then is that the CKeditor, which was dragged is losing the content and clicking in the editor area is not possible anymore. This happens because of the iframes losing the content, it is just a browser behavior. But if we use the divarea ckeditor plugin instead then we do not have iframes and everything looks fine and the divarea is ligher than the iframe.

Attaching a patch showing what I mean.

Wim Leers’s picture

#7: I thought exactly the same, and wrote exactly the same code. But try it, it doesn't work; it seems CKEditor gets confused internally.

At least, I'm not comfortable doing #7 without confirmation from the CKEditor team.

hchonov’s picture

It does not work because of the iframes, using the divarea plugin everything works fine. Iframes lose thier content somehow.

Wim Leers’s picture

Also, regarding divarea (http://ckeditor.com/addon/divarea):

Much similar to inline editing, it benefits from allowing the editor content to inherit from host page styles.

… we precisely don't want that. We want it to be isolated.

hchonov’s picture

I know we are waiting for a reply from the CKeditor team, but I decided to build a new patch just as a proof of concept.

1. divarea plugin for ckeditor is activated by default.
2. on tableDrag event "move" we do not destroy and recreate the CKeditor instances again and again.

=> having multiple field items with CKeditor instances and rearanging them now is much faster and nicer for the user and there are no exceptions as well.

Reinmar’s picture

My favourite problem :D Choose between styles separation and dealing with disappearing iframes.

The problem with iframes comes from the native behaviour that detaching (i.e. moving or removing) an iframe from DOM unloads the inner document.

The proper way to handle this in a draggable component:

  • Initialise editor.
  • Once editor#instanceReady enable dnd.
  • Dragging should not cause detaching editor element from the DOM, so the content does not get unloaded unless user changed the order of the things.
  • Now, on drop block "draggability" and:
    • (The best solution) before the editor element is moved in the DOM call editor.destroy(). It's synchronous, so you don't need to wait for anything. It should not be, however, called before the editor is fully initialise, hence enabling dnd after editor initialisation finished.
    • (Alternative, may log errors, but should work fine) once you moved the editor element in the DOM, destroy the editor. It may bleed but you can intercept those logs by using http://docs.ckeditor.com/#!/api/CKEDITOR-event-log
  • Initialise a new editor and unblock dnd.
Reinmar’s picture

And yes... this is super irritating ;/

In a perfect world there should be a way to sandbox a document fragment inside another document (shadow DOM will enable that but I'm not yet sure if it won't cause other kinds of issues – namely how to run some scripts inside).

In less perfect world CKEditor should take care of reinitialising the iframe itself. That would save all the work needed to reinitialise also the editor infrastructure and UI, saving precious milliseconds. It's a possible improvement although highly non-backward compatible.

Wim Leers’s picture

IOW: the problem is not that that destroying is asynchronous, it's that initializing is. Which makes sense. So at some point during dragging/dropping, as we are continuously moving the CKEditor instance, and constantly are destroying/initializing it, at some point it won't have been initialized yet before we destroy it, and hence BOOM.

How do you recommend detecting that CKEditor is fully initialized?

I am tempted to say "Drupal's table drag logic should let us know whether a certain move is *final* or not, and we'll only re-initialize CKEditor for the *final* move", but that may have a problem too: if you're dragging but pausing for several seconds in a certain place, you'd expect to see a CKEditor instance there.

Once editor#instanceReady enable dnd.

This doesn't work, because we instantiate CKEditor for every row it is dragged along. i.e. while CKEditor is instantiated, we are still in the process of dragging and dropping!

Reinmar’s picture

This doesn't work, because we instantiate CKEditor for every row it is dragged along. i.e. while CKEditor is instantiated, we are still in the process of dragging and dropping!

Hm... if you're changing DOM every time user moved the cursor between next pair of rows, then I would recommend destroying editor immediately on drag start and start showing some placeholder. Alternatively, you can allow the editor to get unloaded on first DOM change and fix it on drag end by destroying and initialising the editor. It will bleed but it will work (with the latest CKE). In other words, don't reinitialise the editor while dnding, because it's not synchronous and it will be very hard for you to sync destroys with initialisations.

BTW. So far no one pointed me to a dnd helper which would remove the dragged item from the DOM before drag end. Is it your custom helper?

Wim Leers’s picture

Yes, this is core/misc/tabledrag.js. Drupal.tableDrag.prototype.row.prototype.swap is what indirectly causes CKEditor to be detached and re-attached.

Wim Leers’s picture

As of CKEditor 4.5.5, CKEditor even detects this problem: https://dev.ckeditor.com/ticket/13790. But no solution is offered.

nod_’s picture

Sooo, I "fixed" it by try/catching. Since the warning don't actually break anything we could go with that.

Side not, not sure why we return stuff from the attach/detach function, it's not used anywhere.

hchonov’s picture

When you compare DnD of CKEditors with IFrames with #11, where the CKEditor is using DivAreas instead of IFrames you might see how much diffrent the UX is. I just do not think it is right to destroy the CKEditor and recreate it again... This would happen then for each DnD, so if I want to swap item 1 with item 5 in the list we'll have to recreate the CKEdtior 4 times.

Could we not make it happen, that the CKeditor refills the content in the IFrame again after DnD and then do not recreate the instance?

no_angel’s picture

Status: Needs review » Needs work

Changing status to needs work.

Wim Leers’s picture

Issue tags: -rc target triage

Let's add documentation to #18, then this will be an acceptable solution IMO.

Wim Leers’s picture

Hrm, actually per the explanation in #14, it's not the creating that ever fails, only destroying does. So the first hunk of #18 is not necessary AFAICT?

thpoul’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
896 bytes

Added documentation per #21 and made the change suggested at #22

Wim Leers’s picture

Issue tags: +Needs manual testing

Time for one more round of manual testing. Hoping @hchonov can do that.

hchonov’s picture

Status: Needs review » Needs work

Honestly I was expecting this to work, but it doesn't.

[CKEDITOR] Error code: editor-incorrect-destroy.
[CKEDITOR] For more information about this error go to http://docs.ckeditor.com/#!/guide/dev_errors-section-editor-incorrect-destroy

There are no exceptions anymore, but this is because of the new handling in CKeditor itself. But now we get this messages in the console instead, which means we are still not doing it the right way.

Wim Leers’s picture

That link says The editor is being destroyed before it is fully initialized., and we full well know/realize that.

I don't see how we can do better.

hchonov’s picture

So the patch is not needed anymore as there are no exceptions raised by CKEditor anymore but only the console warnings. Sorry for that, I just realized it and didn't check when I wrote the previous comment.

So just to summarize:

  1. Using CKEditor with IFrame: when changing the DOM e.g. tabledrag reorder the CKEdtior should be destroyed and reinitialized otherwise the instance is broken.
  2. Using CKEditor with divarea: when changing the DOM e.g. tabledrag reorder the CKEditor should not be destroyed and reinitialized, but D8 8.0.x atm does it, as it does not differentiate between divarea and iframe CKEditor.

Could we provide a D8 core solution for the case when the developer decided to swtich from IFrame to divarea CKEditor so that in this case the CKEditor is not unnecessary destroyed and reinitialized? I am talking about something similar to the change in editor.js at the bottom of the patch in #11.

Wim Leers’s picture

Priority: Normal » Minor
Issue tags: -Quick fix

But the editor.module API is not designed for the divarea. Look at the http://ckeditor.com/addon/divarea description:

This plugin uses a <div> element (instead of the traditional <iframe> element) as the editable area in the themedui creator. Much similar to inline editing, it benefits from allowing the editor content to inherit from host page styles.

That's something that belongs in contrib, not in core IMO. Because you cannot even use <divarea> on e.g. the /node/add form.


@Reinmar told me in chat:

reinmar [3:38 PM]  So you’re not able to block drag and drop while CKEditor is instantiating, right?
wimleers [3:38 PM]  Even if we were able to do that, it'd be utterly unacceptable, because it'd mean you can only move a row one row further, then wait for CKE to initialize, then try again, etc
reinmar [3:39 PM]  ah, ok. I wasn’t sure how the dnd works in your case. Because I know that some implementations don’t change the DOM before the mouse is released. They only operate on CSS
wimleers [3:40 PM]  this one is definitely JS based, and moves things around in the DOM whenever the cursor position dictates that the row has just entered a different "slot"
wimleers [3:40] that's also why we need to destroy/init CKEditor in the first place
reinmar [3:41 PM]  Then yep – you cannot do much unfortunately ;/ You could leave the try-catch on destroy() just to be certain that nothing else fails.  And if you want to get rid of the console logs then you can change CKEDITOR.verbosity
reinmar [3:41]  http://docs.ckeditor.com/#!/api/CKEDITOR-property-verbosity

So we should open a follow-up issue to set CKEDITOR.verbosity to match Drupal's error_level setting in the system.logging configuration. Then this logged error message will only appear while developing, not while in production.

Thoughts?

nod_’s picture

error_level is a can of worm: #1419648: javascript error reporting interface (shouldn't be closed but whatever).

Would rather a setting to configure somewhere in the ckeditor module. Having a global value is going to be a pain to get in.

Wim Leers’s picture

Can you explain why it's a can of worms? It's not clear to me.

Wim Leers’s picture

Assigned: Unassigned » nod_

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sukanya.ramakrishnan’s picture

Hmm, I am reading that ckeditor instances shud never be destroyed . Removing is better. Not sure if this is related.
http://stackoverflow.com/questions/1794219/ckeditor-instance-already-exists

Thanks,
Sukanya

jhedstrom’s picture

pk188’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
927 bytes

Re rolled.

Status: Needs review » Needs work

The last submitted patch, 37: 2611872-37.patch, failed testing. View results

pk188’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review

The last submitted patch, 6: 2611872-debug-do-not-test.patch, failed testing. View results

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing +Needs JS testing

@pk188: thanks for the reroll!

However, for this patch to land today, we'd need JS tests. That didn't exist yet when we were first working on this.