Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#12619 closed Bug (fixed)

[Chrome] Drop image to the empty document does not work properly.

Reported by: Piotr Jasiun Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: General Version:
Keywords: Cc:

Description

Drop image to the empty document does not work properly. Needs fix.

Change History (16)

comment:1 Changed 5 years ago by Piotr Jasiun

Status: newconfirmed

comment:2 Changed 5 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:3 Changed 5 years ago by Artur Delura

To allow drop on empty document we are forced to listen to dragover event on CKEDITOR.document and prevent default browser behaviour when fired. Since issue seems quite easy to fix, there is an issue that we are interferencing DOM element which is not an CKEDTIOR property.

I think this shouldn't be a default behaviour. We should add an option config, which will let user decide whether he or she want us to prevent default browser behaviour.

Last edited 5 years ago by Artur Delura (previous) (diff)

comment:4 Changed 5 years ago by Piotr Jasiun

Lets try with iframe document, or iframe itself, or contents element. There should be some way. Configuration is not an option in this case. I have even no idea how such option should be called so user will understand what does it do.

Last edited 5 years ago by Piotr Jasiun (previous) (diff)

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

Definitely agree with PJ. The inner document is ours and we can do whatever we want with it. The editor container is ours too. But the outer space no.

But I don't understand why did you actually want to touch the outer document. The issue is that dropping into empty inner document does not work. So the only thing I imagine we need to do is to start listening on the inner document instead of on the body.

comment:6 Changed 5 years ago by Artur Delura

I agree with you guys. I somehow overlooked fact that document cover whole area which we need to D&D.

comment:7 Changed 5 years ago by Artur Delura

Okay, so to prevent reloading page on empty document we have to listen to dragover event in our document and prevent default browser behaviour. This will also prevent browser from displaying cursor which let user know where exactly image will be dropped.

To restore this behaviour, we have to set caret position artfically in our document. To do this we can use methods like Document.caretPositionFromPoint() or Document.caretRangeFromPoint, but as we know it's very unstable.

Question is whether we want to go this way.

comment:8 Changed 5 years ago by Piotr Jasiun

We have getRangeAtDropPossition method: https://github.com/ckeditor/ckeditor-dev/blob/major/plugins/clipboard/plugin.js#L1632-L1772.

But the problem is about dropping below the document and only on Chrome (I hope), so you could use caretRangeFromPoint https://github.com/ckeditor/ckeditor-dev/blob/major/plugins/clipboard/plugin.js#L1651-L1653 with the real x posiotion and y on the bottom of body.

comment:9 Changed 5 years ago by Artur Delura

Okay, I worked out simplest solution:

If dragover target is HTML element then we prevent default browser behaviour.

if ( evt.data.$.target.tagName === 'HTML' ) {
    evt.data.preventDefault();
}

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

Even simpler - evt.data.getTarget().is( 'html' ).

But knowing IEs check first if evt.data.getTarget() returns something and that it's an element. IEs tended to return whatever they like (including null).

comment:11 Changed 5 years ago by Artur Delura

Status: assignedreview

Changes in branch:t/12619b. I'm not sure about manual tests which I've written.

comment:12 Changed 5 years ago by Piotr Jasiun

Status: reviewreview_failed

Use server mock instead of real CKFinder for manual tests like these tests do. Also you could move manual test to the uploadwidget plugin since this is about uploading.

comment:13 Changed 5 years ago by Artur Delura

Status: review_failedassigned

comment:14 Changed 5 years ago by Artur Delura

Changes in the same branch.

comment:15 Changed 5 years ago by Artur Delura

Status: assignedreview

comment:16 Changed 5 years ago by Piotr Jasiun

Resolution: fixed
Status: reviewclosed

That cool that you managed to fix this in such a simple way. Closed with git:68bc88a.

Version 0, edited 5 years ago by Piotr Jasiun (next)
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy