Ckeditor widget broken atom drop on firefox

Found a tricky but working workaround on firefox : insert atom at the end of document, if the liner llistener did not catch it...
Does not change anything on Safari (and Chrome, I presume)
Warning : Not tested, at all, on IE...

NB: I did not remove the console logs, which show who caught what...

Still can't reproduce the bug, even on Firefox 34 :-(

Are we talking about a drop in an empty textarea, or a drop in a taxtarea that already contains text paragraphs ?

Thanks nagy.balint for the report, it gives me several hints...

First, I'd always thought that false drop swere drops beyond document limits
Now I see that false drops occur when over a non editable content, what an atom typically is.
The errors occurring on drop "between two adjacent atoms" are probably occuring over one of these two atoms (it depends on css theming (magins, border, ...) but for my atoms, there is a single 1 pixel high line "free" between two consecutive atoms !)
As my patch would try to insert the atom at the end of yhe document, it is already wrong...

Second, during my tests, I was satisfied when I got ONE successfull drop...
I never ran a second trial immediately after (that's why I did not experience JS errors...)
The problem is : after a false drop, the "onLibraryAtomDrag" process is more or less broken, but it is not cleaned up, and a lot of elements (locator, relation, ... are still around, and there is a destructive competition with the new listeners of the second drag...
More over, the dragend event ('On dragend (without drop), stop the liner.") is never fired !

This last issue has to be fixed before we go farther in false drop workaround..

On Chrome, I can reproduce the fact that a drop fails when the mouse pointer is over an atom when the drop occurs.

The editable.once('drop', function (evt) {... insert dropped atom ...}) listener added in onLibraryAtomDrag is not triggered in that case. Dunno which other event to use, though :-/

Congratulations, nagy, you're right on all points !

I've integrated your suggestions, now works ok on firefox and safari.

Can you test on IE ?

Thanks

Testing this, but :

+++ b/modules/library/dnd/plugins/dndck4/plugin.js
@@ -104,6 +104,20 @@ CKEDITOR.plugins.add('dndck4', {
+      // Prevent false drops (out of range)
+      editor.document.on('drop', function (evt) {
+        if (Drupal.dnd.currentAtom) {
+          var atom = Drupal.dnd.sas2array(evt.data.$.dataTransfer.getData('Text'));

The code just above uses Drupal.dnd.sas2array(Drupal.dnd.currentAtom)
rather than
Drupal.dnd.sas2array(evt.data.$.dataTransfer.getData('Text'));
Let's stay consistent ?

Also : I might be confused, but couldn't this be moved into the onLibraryAtomDrag() function ?
(in which case, the previous remark gets fixed as well, since the Drupal.dnd.sas2array() gets passed as the atomInfo param)

Dunno about the false drops, since I haven't been able to reproduce them so far (also, no-one aswered my question in #4 :-p)

But on Chrome, even with the patch, I still get the "nothing happens when an atom is dropped from the library onto an existing ebedded atom" behavior mentioned in #6

#yched,
#12.1 : fixed
#12.2 : no, just because the drop event of onLibraryAtomDrag is NOT fired
#13 drop on an atom seems to be prohibited just by the [content-editable=false] property
("This is read only area, no drop allowed")
same behaviour exists with image2 widget

The goal of this issue is not to make drops always working, it's just to prevent Firefox drop the html part of the drag, if no valid listener is present to catch it;

Webkit browsers already prevent this kind of drop, so there is nothing to, in the scope of this issue

Finally for #4
"Are we talking about a drop in an empty textarea, or a drop in a taxtarea that already contains text paragraphs ?"
We are talking about drop in a textarea which is empty, or contains limited text, so that the "end of document" position is visible on screen, and leaves an empty area below, where "false drops" occur with firefox

drop on an atom seems to be prohibited just by the [content-editable=false] property
("This is read only area, no drop allowed")
same behaviour exists with image2 widget

OK - native widget drag-n-drop (dragging a widget that is already present in the editor to a different location, using the crosshair icon that appears on mouseover) does work when the mouse is over another widget when you do the drop. But it internally uses mousedown / mouseup events, not drag events.
But dnd-library.js initiates a drag event, so we probably don't have a choice for our case here.

- couldn't this be moved into the onLibraryAtomDrag() function ?
- no, just because the drop event of onLibraryAtomDrag is NOT fired

I'm not suggesting that the *content* your editor.document.on('drop') listener should move into the existing editable.once('drop') listener in onLibraryAtomDrag(),
I'm suggesting that your whole editor.document.on('drop') could move into onLibraryAtomDrag(), as a new listener *in addition* to the existing one.

I think doing so would be cleaner by keeping similar code together, and would probably let us refactor so that we don't duplicate the code that does the actual insert (the Drupal.dndck4.getDefaultInsertData() / Drupal.dndck4.insertNewWidget() part)

We are talking about drop in a textarea which is empty, or contains limited text, so that the "end of document" position is visible on screen, and leaves an empty area below, where "false drops" occur with firefox

Oh, OK, that may be why I can't reproduce it. My setup uses CKEditor in div mode, not in iframe mode, so I don't have this "end of document" issue, since my CK doesn't contain a "document".

#15 : I'm thinking of something lilke that - does this work for you guys ?

yes it works (for me)
and it's of course a better solution ;-)
thanks

oup's, forgot to mention : I had to fix a typo (missing parenthesis)

Cool ! Sorry about the typo, I wrote this in a dumb text editor and did not actually test it, since I can't reproduce the problem anyway ;-)

+++ b/modules/library/dnd/plugins/dndck4/plugin.js
@@ -379,6 +379,7 @@ Drupal.dndck4 = {
     listeners.push(editable.once('drop', function (evt) {
       ...
     });
+    listeners.push(editor.document.once('drop', function (evt) {

Then I guess it would be good to add some comments to clarify:
- that this second listener is needed to catch some cases (which ones ?) that the first one doesn't,
- that when the first one runs, it cancels the second one with stopPropagation();
Possibly it could make it easier to explain if the listener to editor.document was attached first ?
Something like : "Add a default drop listener that catches all the drops." / "Add a more specific listener that accounts for the liner, but does not detect XXXX. In the cases where it runs, cancel the default listener by stopping the event propagation." ?

+++ b/modules/library/dnd/plugins/dndck4/plugin.js
@@ -390,17 +391,27 @@ Drupal.dndck4 = {
     // On dragend (without drop), stop the liner.
     listeners.push(editor.document.on('dragend', function (evt) {
       stopLiner();
     }));
+    $(document).bind('dragend', function (evt) {
+      stopLiner();
+    });

Same here - although they really look damn close...
Really really needed ? ;-)

There is a remaining problem with the $(document).bind('dragend' in the onLibraryAtomDrag function approach, and its that the onLibraryAtomDrag will be called several times

Yep, that is why the existing code in onLibraryAtomDrag() does
listeners.push(foo.on('event'), callback);
The listeners in that listeners array are unbound in stopLiner().

But I still don't get why that new dragend listener is better than the existing one :-)

[edit: scratch the interdiff here. Patch is in #27, and interdiff in #28 :-/ ]

@nagy.balint : OK, thanks, got it now.

Then, the 'dragend' event should be attached by :
listeners.push(CKEDITOR.document.on('dragend'), function() {stopLiner();});

- CKEDITOR.document is the CK object for the whole containing document, which is what we want.
- Attaching the event through CK objects and API rather than jQuery lets us stay consistent with the other events added here and how they are cleaned up (added to the listeners array, unbound in stopLiner()),

Er, patch got lost somehow

Gee, and the interdiff in #26 was crap, here it is.

#notmydaytoday

Oh god, definitely not my day.
Fix silly typo. I shouldn't be posting patches on a sunday :-/

Interdiff is with #19, for clarity.

Also, found out that the "catch all" drop listener added on editor.document is not good if the CK is in divarea mode, because in that case, editor.document is the whole page, and so it reacts to drops on all the page (for example on the "node title" textfield), which then leads to JS errors.

Added a check to only add it if not in divarea mode.

We should definitely add some comments here - not today :-)

I'm still trying to see if we can somehow unify the two drop listeners. Will report later.

Meanwhile, found out that the 'dragstart' listener gets reattached each time we go in and out of "source mode" / "wysiwyg mode" using the "Source" button, which then makes the drop listeners fire several times on atom drop. Updated patch fixes that.

OK, I think I brought this down to one single listener. The trick is to attach it to either the iframe document or the editable div, depending on whether the CK is in divarea mode or in iframe mode.

I think I tested all the cases (divarea / iframe, editor with no content / content smaller then the editor area / lots of contents) on both chrome and firefox.
Could not check on IE / Safari though.

Also, renamed the stopLiner() internal function, since it does more than just cleaning up the liner thing.

Also, noticed that on Chrome, the 'drop' listener on the iframe document does not fire if the current editor content is smaller than the iframe height, and the drop is made in the iframe but outside the body content.

This can also be observed with the "old / non-widget" scald plugin : if the CK contains only one small paragraph of text, then atom drops below that paragraph are not caught.

Updated patch adds a workaround : add a min-height on the iframe body during the drop, to make sure it extends to the whole iframe area. I kind of suck at manipulating DOM dimensions, so maybe the code is not as smart as it could be :-/

Hi all,

Found a last little bug for 2014 :

If a textarea is in the form 'Text with summary', everything is OK in the main (value) part, but drop (and insert) fail in the summary part :
The getDefaultInsertData() function fails on context : editor.element.$.attributes['data-dnd-context'].value, which is not defined for the summary;

solved by this tricky addition :

    editor.on('instanceReady', function (evt) {
...
      // Get a valid default context, in case of summary in 'text and summary' textarea.
        if (!editor.element.$.attributes['data-dnd-context']
         && editor.element.$.id.substr(-7) == 'summary') {
          var field = editor.element.$.id.substr(0, editor.element.$.id.length-7);
          for (var instance in CKEDITOR.instances) {
            if (CKEDITOR.instances[instance].element.$.attributes['data-dnd-context']
             && CKEDITOR.instances[instance].element.$.id == field+'value') {
              var context = CKEDITOR.instances[instance].element.$.attributes['data-dnd-context'].value;
              editor.element.$.attributes['data-dnd-context'] = {value: context};
              break;
            }
          }
        }
    });

[I'm sorry I can't build a real patch at the moment]
Note that this could be better handled in dnd.module : dnd_process_textarea(), but summary element is processed before value element, and I could not find the info in the form_state... (and the comment ahead of this function does not help...)

Happy new year to the scald team !

No, #36 was not the last :

Clicking Switch to plain text editor and then Switch to rich text editor breaks the dragstart handler (it's still linked to the old instance(s));
Preventively unbinding the event solves the problem;

pseudo-interdiff :

     // Listen to atom drags from the Library.
     editor.on('instanceReady', function (evt) {
+      // remove any previous (obsolete) binding
+      $(document).unbind('dragstart');
       $(document).bind('dragstart', function (evt) {
         if (Drupal.dnd.currentAtom) {
           var dragInfo = Drupal.dnd.sas2array(Drupal.dnd.currentAtom);
           Drupal.dndck4.onLibraryAtomDrag(editor, Drupal.dnd.Atoms[dragInfo.sid]);
         }
       });

As usual, this is certainly not the best solution, but how to dynamically update the editor argument ?
(The dnd library widget (where the dragstart occurs) is persistent while the textareas CKEditor instances are destroyed/rebuilt...)

@gifad: Thanks, will try to look into that.

However, it might be good to check if thoses issues exist with the old plugin as well. If yes, we might want to defer to a sperate issue, as the best place for the fix could possibly be outside the new plugin JS file.

Also - does that mean that, other from the remarks in #36 / #37, the patch in #35 works for you ?

Hi @yched,
Yes the patch in #35, (as all previous ones) work fine to me, ... until I find an unexpected behaviour in 'border' cases ...
Forgot to mention : the two previous issues are on drag and drop only : 'Insert' button always work flawlessly - so I suppose the old plugin is ok... [but I have limited access to dev environments at the moment]

Edit:
Sorry, yet another glitch with #35 :

Open an existing, potentially large, document;
Scroll anywhere in the document (or not), using only scroll bar or mousewheel (without clicking (focusing) in the document;
Initiate a drag; the liner behaves correctly, following cursor position;
Now drop the atom;
The atom will be inserted at the very top of the document, the document will eventually scroll up to make the atom visible;
No JS error, the dropped widget seems perfectly functional, although misplaced;
Note : this drop cannot be undone..

Repeating the same operation, but with a click inside the document (just a right click to "delete the atom from the textarea", for instance), everything is correct
As far as I can trust firebug, a console.log(range); from range = finder.getRange(sorted[0]);show exactly the same object in both cases

the two previous issues [#36 / #37] are on drag and drop only : 'Insert' button always work flawlessly - so I suppose the old plugin is ok

Well, the old plugin does drag-n-drop too, that's why I'm asking ;-)

@yched, #41 : "" the old plugin does drag-n-drop too""
short answer : (but see below)
yes but the operations are on the drop side, and are actually the same as the insert button (just adds moving the insertion point during drag)

=====

@yched, #35, to review last issues in the legacy dnd/plugins/ckeditor/plugin.js :

#36 : Text with summary
The legacy plugin fails at the same point, but this is masked most of the times :
- if the browser is firefox
- if the default context is sdl_editor_representation
The failure occurs before the handler have modified anything in the DOM, and, more importantly, before it executes evt.preventdefault().
So the default is executed, that is insert the text/html embedded in the drag at the insertion point (BTW, this drop handler is totally useless with firefox...)
If the atom needs to be refreshed later, this will succeed, as the representation is in the cache;
Now, if the default context is not sdl_editor_rep., we get the "undefined" label for the atom...
The fix in #36 is tightly related to the text with summary anomaly, it can be applied as well, and solves the problem;

#37 : switching plain text <> rich text editors
The problem does not exist in the legacy plugin, as the drag and drop operation is "drop driven", and the drop handler has, by definition, the same lifecycle as the instance it applies to;
The problem in the widget plugin is that the operation is bound to the drag side, which is not aware of the death and rebirth of the drop side (mostly the "editable")

@nagy.balint : be sure to apply also #33 (on instanceReady instead of contentDom)
what do you experiment, when "it breaks dragstart" ?

I'm not sure at all of my fix, as I know nothing about the lifecycle of so-called "objects" in javascript ;/

#39 insert at top, if document never focused
Same behavior with legacy plugin
Unfortunately, I have no idea of a solution : I suspect the document (or rather the editable) is not fully initialized before a first focus...
see http://docs.ckeditor.com/#!/api/CKEDITOR.editable-property-status :
"" The ready status is set after the first CKEDITOR.editor.setData is called. ""

@nagy.balint : I don't know why, but the code at #37 works randomly
(I usually embed a lot of console.log() in the code, does it modify the relative timing of events ?)

@all : I finally got rid of this dangling "editor" argument
Tested on firefox only, this works :

    editor.on('instanceReady', function (evt) {
      // remove any previous (obsolete) binding
      $(document).unbind('dragstart'),
      $(document).bind('dragstart', function (evt) {
        if (Drupal.dnd.currentAtom) {
          var dragInfo = Drupal.dnd.sas2array(Drupal.dnd.currentAtom);
          Drupal.dndck4.onLibraryAtomDrag(Drupal.dnd.Atoms[dragInfo.sid]);
        }
      });

note the removal of the "editor" argument.
and the function definition, with a reliable "editor" :

  // Heavily inspired from CKE widget plugin's onBlockWidgetDrag().
  onLibraryAtomDrag: function (atomInfo) {
    var editor = Drupal.ckeditorInstance._.editable.editor;
    var finder = editor.widgets.finder,
...
...

This way, the drag handler is explicitly linked to the (active) instance that will receive the drop

Test with a "Text with summary" document : with "Edit summary" clicked, you can drag the atom over the two parts of the textarea, you see the liner is drawn (and the drop succeed) only in the "active" (last focused) part...

At least, there was a relation between #37 and #39 ...

Enough for 2014, happy new year !

Hi nagy.balint,

The key point of #44 is that each instance monitors drag, and is ready to receive the drop in its editable;
This is just something I can understand;
With a single binding, I just can't figure out how can the single "editor" variable can delegate the events to the relevant instance (again, I'm not javascript expert)

nice, nagy.balint, very nice ;-)
#51 passes all my tests, on firefox and safari, on macOSX

#39 may be definitely cast to a ckeditor on firefox issue, and is minor.

Thanks again,

Re: the "summary" textarea not having the 'data-dnd-context' attribute

The old plugin (scald/modules/library/dnd/plugins/ckeditor/plugin.js) also has code that relies on editor.element.$.attributes['data-dnd-context'].value, so it's likely to be broken as well for the "summary" textareas ?

So :
- if it's not a bug specific to the new plugin, it would be better off adressed in a separate issue
- Plus, I'd tend to consider the issue is rather that dnd_process_textarea() doesn't place the attribute on the "summary" textarea, rather than having a not-too-beautiful workaround in the JS side to fetch it from a different textarea (and have to duplicate it in the two plugins).
The "summary" is a textarea with CK & support for scald drag-n-n-drop, so it should have the attribute, just like the main "body" textarea does.

this issue was created for a major issue with the drag and drop on firefox, which this patch solves and it seems to be well tested now

But that bug was fixed in #35, and is not related to the "summary" thing, right ?
So yeah, I agree with "let's commit the fix for *that* major firefox bug, and leave out the summary stuff, which is a totally different (and larger) bug, for a separate issue :-)

Meaning, #51 without the "// Get a valid default context, in case of summary in 'text and summary' textarea" hunk ?

@yched,

The "summary" is a textarea

not really : the $element does not even has a #field_name
The only hint I can find is the $element[#id] ( edit-body-und-0-summary ), and then navigate through the form_state (via #parents) to find the "real" textarea...
(same algorithm as my workaround, in fact, but the code would be yet more "not-too-beautiful", by a large step)

My conclusion is exactly the same as nagy.balint...

@yched
[Edit : Finally wrote really terrible code for a fixed dnd_process_textarea() (attached)
will submit an issue (and patch) when I get back to a git-enabled computer, if my js workaround is definitely rejected;
At least, it's on server side, your boss and customers won't see this code with your signature ;-)
[Edit: some one to fix the comment ahead dnd_process_textarea() ?
really not intended pun...]]

@gifad :
I was just looking into that as well. I was more inclined to fix this in mee_field_widget_form_alter() - it's the function that adds #dnd-enabled on the main (non-summary) textarea, and should IMO set it as well on the 'summary' textarea if the widget is 'text_textarea_with_summary'
(as a side note, the part in there about $element['#id'] and '#dnd-settings' looks like dead code, nothing ever uses that)

But then I noticed a lot of other things are broken regarding atoms in 'summary' :
1) The resource manager does not detect the atoms that are in there (mee's text_field_insert() / text_field_update()), so the atoms included in the summary are not tracked in the {mee_resource} table
2) When using the old plugin (mee_store_format() == 'sas'), the 'summary' value is not processed like the main textarea (the "sas code <--> rendered atom" transformations on form display and form submit - mee's text_field_presave() / mee_field_widget_form_alter()).

Which means that "atoms in summaries" does not seem to be an intended feature in scald so far. The "new widget" just happens to work a bit better with them since it is not affected by 2), but that still leaves 1), atom tracking is broken in summaries.

Thus, I still think "atoms in summaries" would be better off in a separate issue, since, again :
- it's completely unrelated to the original issue here, and we're already at 60 posts
- the current workaround is kind of ugly :-)
- the problem space about supporting 'summary' textareas is much larger

Just reattaching #51 with a couple minor style/comment fixes [edit: also, namespaced the event a bit more specifically].

I'll leave it to @jcisio to decide if he wants to keep the "Get a valid default context, in case of summary in 'text and summary' textarea" snippet.

Also : removing the event on destroy might be a good idea anyway, even if the editor is not re-enabled after that.

#61 does not work :

Safari:
TypeError: null is not an object (evaluating 'editable.getFirst')

Firefox:
TypeError: editable is null
var editableHasContent = (editable.getFirst() != null);

in onLibraryAtomDrag: function (editor, atomInfo)

after "Switch to plain text editor" / "Switch to rich text editor"

#60 was OK

@gifad : Weird, can't reproduce #62.

With patch #61, "Switch to plain text editor / Switch to rich text editor / drag an atom into the editor" works fine for me on both Chrome and Firefox - tested both iframe and divarea modes.

@yched : my bad : I kept the nagy.balint's (#51) namespace in the bind statement, and yours in the unbind;

Actually, #61 works ok
sorry..

@nagy.balint #65 :

However i discovered a new bug:
If you try to drop an atom into a textarea which does not have dnd enabled you get the following:
On Chrome:
plugin.js?t=EAPE:320 Uncaught TypeError: Cannot read property 'value' of undefined

Can't reproduce. Can you provide specific info about the "textarea which does not have dnd enabled" ? Is it a field ? which configuration ? Does it have a CKEditor enabled or is it a plain textarea ?

@nagy.balint : I have exactly the same configuration;
problem solved by :

   getDefaultInsertData: function (editor, atomInfo) {
     var sasData = Drupal.dnd.sas2array(atomInfo.sas);
     return {
       sid : atomInfo.sid,
       type: atomInfo.meta.type,
       // The default context for newly embedded atoms is a setting of the text
       // field, and is placed in the 'data-dnd-context' attribute on the
       // textarea.
-      context : editor.element.$.attributes['data-dnd-context'].value,
+      context : (editor.element.$.attributes['data-dnd-context']) ?
+        editor.element.$.attributes['data-dnd-context'].value :
+        Drupal.settings.dnd.contextDefault,
       // Modules can use hook_scald_dnd_library_item_alter() to add default
       // options in the sas code for the atom.
       options : sasData.options || '{}',

Rather than "prevent the drop", this lets it go with site default context
I think "dnd-enabled" is there to provide the library widget, rather than allow/prevent drop

EDIT : this was my first workaround for the text and summary problem, I missed to publish it...

REEDIT : Text and summary solved by this patch to mee.module :

function mee_field_widget_form_alter(&$element, &$form_state, $context) {
...
   // Activate DnD Library for this element if enabled.
   if (!empty($enables['dnd'])) {
-    // Force an #id so that we can reference to it later
-    $element['#id'] = drupal_html_id('edit-' . $context['field']['field_name']);
-    $element += array(
-      '#dnd-enabled' => TRUE,
-      '#dnd-settings' => array(
-        'drop_selector' => '#' . $element['#id'] . ' .drop',
-      ),
-    );
+    $fieldname = $element['#field_name'];
+    $language = $element['#language'];
+    $settings = $form_state['field'][$fieldname][$language]['instance']['settings'];
+    $context_default = isset($settings['context_default']) ? 
+       $settings['context_default'] : 
+       variable_get('dnd_context_default', 'sdl_editor_representation');
+  //uncomment when dnd_process_textarea removed from dnd.module
+  //$element['#attached']['library'][] = array('dnd', 'library');
+  //$element['#attributes']['data-dnd-context'][] = $context_default;
+    if (isset($element['summary']))
+      $element['summary']['#attributes']['data-dnd-context'][] = $context_default;
  }

and then remove dnd_element_info_alter() and dnd_process_textarea()

@nagy.balint : I mostly use this code mixed with my "mutable" version, but the drop part is not affected;
I just re-did a quick test of this patch alone with a clean 7x-1.3+1-dev, and drag and drop are just OK.
So I'm OK for RTBC (just missing @jcisio review ?)

Sorry for letting this slide a bit.

+++ b/modules/library/dnd/plugins/dndck4/plugin.js
@@ -292,7 +300,9 @@ Drupal.dndck4 = {
       // The default context for newly embedded atoms is a setting of the text
       // field, and is placed in the 'data-dnd-context' attribute on the
       // textarea.
-      context : editor.element.$.attributes['data-dnd-context'].value,
+      context : (editor.element.$.attributes['data-dnd-context']) ?
+        editor.element.$.attributes['data-dnd-context'].value :
+        Drupal.settings.dnd.contextDefault,

I still don't get exactly why we do this ? Is that for the case mentioned in #68 / #69 ? (text field with 'Drag'n'Drop Enabled' and 'MEE Enabled' checked off)

Why would we want to allow the drop in those cases ? The field says it wants to completely stay out of scald...
Shouldn't the plugin simply return immediately in its init: function (editor) { if the editor.element.$.attributes['data-dnd-context'] is not present ?