Opened 4 years ago

Closed 3 years ago

#13184 closed Bug (fixed)

[DnD] Drop on certain UI elements reloads the page

Reported by: Olek Nowodziński Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.2
Component: UI : Dialogs Version: 4.5.0 Beta
Keywords: Cc:

Description (last modified by Olek Nowodziński)

Dialogs

  1. Use http://tests.ckeditor.dev:10450/tests/plugins/uploadwidget/manual/droponnotallowedparts
  2. Open a dialog.
  3. Drop an image: on a dialog, dialog cover etc.

Expected: No action.

Actual: Page reload. The image is displayed by the browser.


Notifications

  1. Use http://tests.ckeditor.dev:10450/tests/plugins/uploadwidget/manual/droponnotallowedparts
  2. Drop some image.
  3. While the image is uploading, drop another on progress bar.

Expected: No action.

Actual: Page reload. The image is displayed by the browser.


Scrollbars

  1. Use http://tests.ckeditor.dev:10450/tests/plugins/uploadwidget/manual/droponnotallowedparts
  2. Create a long content, so the vertical scrollbar is visible.
  3. Drop an image on the scrollbar.

Expected: No action.

Actual: Page reload. The image is displayed by the browser.

Change History (13)

comment:1 Changed 4 years ago by Jakub Ś

Status: newconfirmed

Reproducible in every browser.

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

Milestone: CKEditor 4.5.0

comment:3 Changed 3 years ago by Olek Nowodziński

Description: modified (diff)
Milestone: CKEditor 4.5.1
Summary: [DnD] It is possible to drop on the dialog, which causes page reload[DnD] Drop on certain UI elements reloads the page

comment:4 Changed 3 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:5 Changed 3 years ago by Tade0

Status: assignedreview

I have one obvious solution to this pushed to branch:t/13184, although it doesn't work properly on IE, because for some reason dropping an image on the dialog title still results in a page reload.

Interestingly, attaching a listener that uses preventDefault() to the title did not solve this issue. Any suggestions why might this be happening?

comment:6 Changed 3 years ago by Tade0

Apparently attaching the listeners to the body parent solves this problem.

Changes pushed to branch:t/13184a.

comment:7 Changed 3 years ago by Tade0

Inspired by this commit: https://github.com/ckeditor/ckeditor-dev/commit/5394b5b I modified my solution to be consistent with what was done there.

Changes pushed to branch:t/13184b

comment:8 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed

Still able to reproduce "Notifications" case. Checked in latest Chrome.

comment:9 Changed 3 years ago by Tade0

Status: review_failedreview

Applied the same changes as previously, but this time to the notification DOM element.

Rebased to master, added tests. Changes pushed to branch:t/13184b.

comment:10 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed

Together with p.jasiun, we decided that it's time to clean up the code. Since we're unable predict the number of future use cases and the code is already redundant:

  1. Please implement CKEDITOR.plugins.clipboard.preventDefaultDropOnElement( element ) that would correspond with
    function preventDefaultSetDropEffectToNone( evt ) {
    	evt.data.preventDefault();
    	evt.data.$.dataTransfer.dropEffect = 'none';
    }
    
    element.on( 'dragover', preventDefaultSetDropEffectToNone );
    
  2. Use CKEDITOR.plugins.clipboard.preventDefaultDropOnElement to replace existing implementations (clipboard, notification, dialog).
  3. Write documentation for the method and automated tests (if possible). Some ideas in https://github.com/ckeditor/ckeditor-dev/commit/cb5fd3a#diff-1a20173088d59723c5efce3532752b2fR963 (didn't check that). Don't worry If not applicable because we still have manual tests for the feature.

BTW: I think I'm still able to drop on editor's scrollbar and reload the page. It could be OS–specific though.

Thanks!

comment:11 Changed 3 years ago by Tade0

Status: review_failedreview

On what OS does this happen?

I added a generic unit test that just checks whether on dragover preventDefault was called and dropEffet was set to 'none'.

Changes pushed to branch:t/13184c.

comment:12 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_passed

Pushed some fixes and refactored tests.

comment:13 Changed 3 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged git:20cc11b into master.

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