Getting
Uncaught TypeError: Cannot read property 'wrapper' of undefined
b.on.CKEDITOR.tools.extend.finder.c.finder.lookups.default @ ckeditor.js?nseafp:1094
k.greedySearch @ ckeditor.js?nseafp:1074
Drupal.dndck4.onLibraryAtomDrag @ plugin.js?t=F74C:525
(anonymous function) @ plugin.js?t=F74C:110
c.event.handle @ jquery.js?v=1.4.4:64
c.event.add.h.handle.o @ jquery.js?v=1.4.4:57

CommentFileSizeAuthor
#16 widget-cleanup-2545360-16.patch1.09 KBnagy.balint
#15 scald-dndck4-cke452-2545360-15.patch5.05 KBgifad
#11 scald-dndck4-cke452-2545360-11.patch5.05 KBgifad
#9 scald-dndck4-cke452-v2-2545360-9.patch4.38 KBnagy.balint
#8 scald-dndck4-cke452-2545360-8.patch2.91 KBgifad
#6 scald-dndck4-cke452-2545360-5.patch1.68 KBgifad
#5 ckeditor-4-5-2-workaround-2545360-5.patch626 bytesnagy.balint
#4 scald-dndck4-cke452-2545360-4.patch1.93 KBgifad
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Seems like the trouble is caused by this fix:
http://dev.ckeditor.com/ticket/13397

As it fails on
if ( widgetsRepo._.draggedWidget.wrapper.contains( el ) ) {
in ckeditor/plugins/widget/plugin.js on line 2421

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Seems like the fix would be to add 

		// Mark dragged widget for repository#finder.
		this.repository._.draggedWidget = this;

just before our greedySearch call.
However we do not use this.repository, so first would need to get the repository.
The docs claim that

An instance of the repository is available under the CKEDITOR.editor.widgets property.

Yet i cannot find it there, and neither in the editor variable existing in the scope.

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Seems like
editor.widgets._.draggedWidget
Will work instead of this.repository._.draggedWidget

However the = this part not sure yet how to replace.

It needs something that it can do contains on
"Uncaught TypeError: Cannot read property 'contains' of undefined"

gifad’s picture

gifad CreditAttribution: gifad commented
Status: Active » Needs review
FileSize
scald-dndck4-cke452-2545360-4.patch1.93 KB

The code in dndck4 drag and drop is derived from ckeditor widget drag and drop;
The problem is what is dragged is not (yet) a widget, so the expected "this" does not yet exist !

The attached patch is a quick and dirty attempt to create the widget at dragstart time...

Two unpleasant side effects:

  1. If the page has multiple ckeditor instances (text with summary...), as many widgets are created (one by each instance)
  2. I did not take care of the "Snapshot" stuff, as I don't know how it works...

at least, it fixes the "critical" aspect of the issue ;)

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented
FileSize
ckeditor-4-5-2-workaround-2545360-5.patch626 bytes

I got to the same conclusion.

Here is a lightweight dirty workaround for the same thing.

Seems to be working in firefox, chrome and ie 9, although more testing is always welcome.

gifad’s picture

gifad CreditAttribution: gifad commented
FileSize
scald-dndck4-cke452-2545360-5.patch1.68 KB

A cleaner implementation of #4 (#4 was working only on last ckeditor instance...)

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Thanks gifad!

However I think It needs to be refactored cause now there are duplicate code when creating the widget in both the insertNewWidget function and separately when creating the dummy widget. Also snapshot should be included the same way as we have in the insertNewWidget function.
So the code would change quite a lot at the end. So maybe its better to have a more lightweight solution for now, and then later if we want to refactor the whole system, we can still do so.

@jcisio which one do you prefer? (comment #5 vs comment #6)

gifad’s picture

gifad CreditAttribution: gifad commented
FileSize
scald-dndck4-cke452-2545360-8.patch2.91 KB

Refactored the code to avoid duplicate code...
About snapshots, I can't get the "Undo" command working in the current implementation, so I did not touch it...

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented
FileSize
scald-dndck4-cke452-v2-2545360-9.patch4.38 KB

Thanks!

I however started a new patch because I think we can create the widget before calling the onLibraryAtomDrag and we can even pass the widget as 'this' to the function just like the code it was derived from. So this would make the code a lot more similar to the original, and maybe reduce some future problems.

Needs a bit more testing, and maybe it could be simplified even more, for now only the editor could be removed from onLibraryAtomDrag as widget.editor works fine.

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Patch #9 passes my tests under chrome, firefox, and ie9, under ckeditor 4.5.2, 4.5.1, and 4.4.6

gifad’s picture

gifad CreditAttribution: gifad commented
FileSize
scald-dndck4-cke452-2545360-11.patch5.05 KB

#9 tested, works fine;
Added a clean-up of the unused widget(s), in case of multiple ckeditor instances (or just text with summary)

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Seems like you have not tested your code, as in javascript you cannot set a default value to a function parameter, that produces an error.

Also It seems that destroying those values are not really necessary. Because the widget variable will be destroyed after the scope of the dragstart ends. And anyways there is a different widget variable per editor.

Also the draggedWidget will have a value on dragstart right away (per editor again) so setting it to null seems unnecessary, of course maybe it frees up a tiny amount of memory space.

So I think i will commit #9

  • nagy.balint committed 4073e7e on 
    Issue #2545360 by gifad, nagy.balint: CKEditor 4.5.2 seems to not work
nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented
Status: Needs review » Fixed
gifad’s picture

gifad CreditAttribution: gifad commented
FileSize
scald-dndck4-cke452-2545360-15.patch5.05 KB

Sorry, #11 was tested only with firefox, which did not complain...
About extra widgets, they are not automatically purged, as they have been inited;
You can follow them in the DOM : CKEDITOR.instances["edit-body-und-0-summary"].widgets.instances[0]
Attached corrected version, in case any one is interested to experiment...

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented
FileSize
widget-cleanup-2545360-16.patch1.09 KB

So something like this.

However I get under IE 9 SCRIPT5007: Unable to get value of the property 'find': object is null or undefined
ckeditor.js: line 1129 character 190

gifad’s picture

gifad CreditAttribution: gifad commented

After http://docs.ckeditor.com/#!/api/CKEDITOR.plugins.widget-method-destroy
one could use widget.repository.destroy(widget, true);
I have no IE at hand to check further...

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Unfortunately that seems to make no difference.

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Moved this to a followup, so we can close this issue.
#2546712: followup: ckeditor 4.5.2 compatibility - cleanup widget variable

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

nagy.balint’s picture

nagy.balint CreditAttribution: nagy.balint commented

Good news, 4.5.3 seems to work fine as well.