Opened 4 years ago

Last modified 2 years ago

#13736 review_failed Bug

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

Reported by: dk Owned by: Tomasz Jakut
Priority: Nice to have (we want to work on it) Milestone: CKEditor 4.7.1
Component: General Version: 4.5.2
Keywords: Cc:

Description

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.

Attachments (1)

ckeditor-dnd-demo.zip (2.4 MB) - added by dk 4 years ago.
Test plugin for DnD in dialog with CKE 4.5.1 & 4.5.3 preconfigured

Change History (25)

comment:1 Changed 4 years ago by Piotrek Koszuliński

Resolution: invalid
Status: newclosed

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

comment:2 Changed 4 years ago by 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.

Changed 4 years ago by dk

Attachment: ckeditor-dnd-demo.zip added

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

comment:3 Changed 4 years ago by 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.

comment:4 Changed 4 years ago by 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!");
            };
         }
    };
});

    }
});
Last edited 4 years ago by dk (previous) (diff)

comment:5 Changed 4 years ago by Piotrek Koszuliński

Keywords: D'n'D dialog removed
Resolution: invalid
Status: closedreopened
Version: 4.5.4 (GitHub - master)4.5.2

comment:6 Changed 4 years ago by Piotrek Koszuliński

Status: reopenedconfirmed

comment:7 Changed 4 years ago by 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.

comment:8 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5

comment:9 Changed 4 years ago by Tomasz Jakut

Owner: set to Tomasz Jakut
Status: confirmedassigned

comment:10 Changed 4 years ago by Tomasz Jakut

Status: assignedreview

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.

Last edited 4 years ago by Tomasz Jakut (previous) (diff)

comment:11 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5CKEditor 4.5.6

comment:12 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.6CKEditor 4.5.7

comment:13 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:14 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:15 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:16 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:17 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.11CKEditor 4.6.1

comment:18 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

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

comment:19 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.2
Priority: NormalNice to have (we want to work on it)

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.

comment:20 Changed 2 years ago by kkrzton

Milestone: CKEditor 4.7.0

Rebased on major and updated version tags in tests.

comment:21 Changed 2 years ago by kkrzton

Status: reviewreview_failed

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)?

comment:22 Changed 2 years ago by Tomasz Jakut

Status: review_failedreview

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.

comment:23 Changed 2 years ago by Tade0

Status: reviewreview_failed

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 bubbling mode, stop event propagation on elements that are meant to be drop areas?

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

EDIT: confused capturing with bubbling at first.

Last edited 2 years ago by Tade0 (previous) (diff)

comment:24 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.7.0CKEditor 4.7.1
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy