Unable to create D'n'D area inside CKEditor dialog => broked some add-ons

[dk]

Steps to reproduce

1. Open DOKSoft Quick Image add-on or other one with D'n'D area inside dialog.
2. Try to D'n'D files into the area.

Expected result

CKEditor must respect outside areas and do not use D'n'D handler globally.

Actual result

You unable to drop files inside D'n'D area.

Other details (browser, OS, CKEditor version, installed plugins)

Any browser. This aspect of D'n'D feature was broked in CKEditor 4.5.2. Still (4.5.4) exists in CKEditor.

[Piotrek Koszuliński]

I'm sorry, but DOKSoft Quick Image is a 3rd party addon and you need to report this issue to its author.

[dk]

This issue affects not only to DOKSoft add-ons. Currently there is not able to handle D'n'D areas in the dialogs at all.

I created test add-on ("dndtest") and packed it with CKEditor 4.5.1 and 4.5.3, as you can see all works in 4.5.1 and broken in 4.5.3. To check how plugin works see browser's console log: you will see text "Drop!" if all works fine. The archive contains already preconfigured CKEditor versions with the test plugin. You can change CKEditor version in ckeditor.html.

[dk]

Test plugin for DnD in dialog with CKE 4.5.1 & 4.5.3 preconfigured

[Piotrek Koszuliński]

This issue affects not only to DOKSoft add-ons. Currently there is not able to handle D'n'D areas in the dialogs at all.

But that's not a bug. That's intentional. Dropping on a dialog would reload your page (load the file that you dragged). We don't know how the DOKSoft's plugin work, so they need to report a concrete problem that they have. We will not be able to fix their plugin.

[dk]

DOKSoft is me. But this has no matter here, the problem is more global then our add-ons.

You are right when you telling that by default dropping to the dialog will cause reloading the page. But I'm telling not about dropping in just a dialog. I mean a case when I add <div> into dialog and with JS DnD API want use this <div> as drop area.

1. Add <div> inside dialog's HTML
2. Define its .ondrop(event) listener
3. By JS/HTML spec this <div> must accept dropped files on it using defined in function processing, not reloading a page.

But if we use CKEditor 4.5.2 or newer we unable to use this part of JS feature.

So any developer unable to create DnD area with custon <div> element inside CKEditor dialog. Even Flash component included into a dialog unable to accept dropped on it files.

Just see the code of my test add-on (from attached to this ticket file):

CKEDITOR.plugins.add( 'dndtest', {                                    
    icons: 'dndtest',                                                 
    init: function( editor ) {                                        
                                                                      
                                                                      
editor.addCommand( 'dndtest', new CKEDITOR.dialogCommand( 'dndtest' ) );
editor.ui.addButton( 'dndtest', {                                     
    label: 'DND Test',                                                
    command: 'dndtest'                                                
});

CKEDITOR.dialog.add( 'dndtest', function( editor ) {
    return {
        title: 'DND test',
        minWidth: 400,
        minHeight: 200,

        contents: [
            {
                id: 'tab-basic',
                label: 'Basic Settings',
                elements: [
                    {
                        type: 'html',
                        html: '<div id="dnd" style="width:300px;height:200px;border:1px solid gray">D"n"D to me</div>'
                    }
                ]
            }
        ],

        onShow: function() {.
            var el = document.getElementById("dnd");
            el.ondrop = function(event) {
                // THIS CODE WILL NOT BE EXECUTED IN CKEDITOR 4.5.2 AND NEWER
                event.preventDefault();
                console.log("Drop!");
            };
         }
    };
});

    }
});

[Piotrek Koszuliński]

We worked recently on blocking dropping on CKE's UI. It needs to be checked how to allow dropping in certain, controllable, situations.

[Tomasz Jakut]

It's probably not the finest solution, but I added allowDropOn property to dialog definition. It contains list of ids of elements that should be marked as droppable areas. So if we want to be able to drop something on #dnd, we can do it that way:

CKEDITOR.dialog.add( 'dndtest', function( editor ) {
    return {
        title: 'DND test',
        minWidth: 400,
        minHeight: 200,

        allowDropOn: [ 'dnd' ],

        contents: [
            {
                id: 'tab-basic',
                label: 'Basic Settings',
                elements: [
                    {
                        type: 'html',
                        html: '<div id="dnd" style="width:300px;height:200px;border:1px solid gray">D"n"D to me</div>'
                    }
                ]
            }
        ],

        onShow: function() {.
            var el = document.getElementById("dnd");
            el.ondrop = function(event) {
                // NOW IT SHOULD WORK
                event.preventDefault();
                console.log("Drop!");
            };
         }
    };
});

Internally it uses CKEDITOR.plugins.clipboard.preventDefaultDropOnElement with additional parameter (so that method was also modified).

Current version is on ​branch:t/13736.

[Marek Lewandowski]

Moving tickets to the next milestone.

[Marek Lewandowski]

Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.

[Marek Lewandowski]

We won't fit it in 4.6.2 just yet, let's move it to our todo list to make sure we won't forget about it.

[kkrzton]

Rebased on major and updated version tags in tests.

[kkrzton]

I wonder why dragover event was blocked ​here? Is it not possible to drop when it is allowed? The blocked dragover results in drop cursor not visible over droppable area in the browser.

IE8:

Unit test fails with

tests/plugins/clipboard/drop test preventDefaultDropOnElement with exclusions

Unexpected error: Object doesn't support this property or method
Expected: undefined (undefined)
Actual:   undefined (undefined)

In manual test, dropping reloads page (opens the image). I am not sure if D'n'D is supported in IE8? Our ​docs says that There is no support for uploading pasted and dropped files so normal drop should work.

Edge:

The drop cursor is visible for the whole dialog. Not sure if it is an issue and if it can be fixed easily.

---

In all browsers but Edge it is possible to drop image on dialog overlay (also the drop cursor is active there). In Edge it is not possible to drop image on dialog overlay and there is disabled drop cursor visible. This is not connected with this issue, but worth mentioning anyway.

---

The solution and code looks pretty good. Only IE8 needs some attention (Edge works fine, the cursor behaves a little different, but it is not an issue IMHO.). And I would also take a look at blocking dragover, maybe there is a way to achieve the same results without blocking it (so drop cursor is active/visible)?

[Tomasz Jakut]

It seems that fix was not working in IE8 due to the fact that it doesn't support Array.prototype.indexOf. I've replaced this method with our CKEDITOR.tools.array.indexOf. I've also slightly modified test to be able to run smoothly in IE8. Pushed changes to t:/13736.

I wonder why dragover event was blocked ​here? Is it not possible to drop when it is allowed?

Unfortunately not. Even ​MDN mentions it:

When the user releases the mouse, the drag and drop operation ends. If the mouse was released over an element that is a valid drop target, that is, one that cancelled the last dragenter or dragover event, and then the drop will be successful, and a drop event will fire at the target.

[Tade0]

I see that the fix works, but to me it doesn't seem clean enough.

Instead of handling events differently depending on a whitelist of ids, why not, given the fact, that by default events propagate in capturing mode, stop event propagation on elements that are meant to be drop areas?

I updated the manual test so that it explains my point.