Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix getting drop target when dragging from file manager #28692

Merged
merged 3 commits into from
Sep 1, 2017

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Aug 15, 2017

Description

Possibly related to jquery.fileupload upgrade, now
e.originalEvent.target might be null.

Related Issue

Fixes #28689.

Motivation and Context

See ticket

How Has This Been Tested?

Manual testing like in the ticket.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@PVince81 PVince81 added this to the development milestone Aug 15, 2017
@PVince81 PVince81 self-assigned this Aug 15, 2017
@PVince81 PVince81 requested a review from VicDeo August 15, 2017 10:48
@PVince81 PVince81 added the p2-high Escalation, on top of current planning, release blocker label Aug 25, 2017
@@ -2708,7 +2708,7 @@
return false;
}

var dropTarget = $(e.originalEvent.target);
var dropTarget = $(e.originalEvent.target || e.originalEvent.delegatedEvent.target);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the e.original.target won't be needed any longer.

The change in the library that affects this is

if (that._trigger('drop', e, data) !== false) {
(before the library update) and https://github.com/owncloud/core/blob/master/apps/files/js/jquery.fileupload.js after the library update.
The actual event isn't set as the originalEvent but as the delegateEvent.

Unless we want compatibility between the library versions, which I don't think it's needed, we can remove the old code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking deeper. To be honest I was just too lazy to investigate this detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvillafanez adjusted

@PVince81 PVince81 force-pushed the fix-filelist-drop-target branch from 77a0a40 to 3e9b68a Compare August 31, 2017 08:36
@SergioBertolinSG
Copy link
Contributor

SergioBertolinSG commented Aug 31, 2017

@SergioBertolinSG
Copy link
Contributor

SergioBertolinSG commented Aug 31, 2017

  • BUG: When uploading a folder with several folders and files inside them into a subdirectory of the current view, the files are not uploaded.

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 31, 2017

stable10: owncloud/notifications#117

@jvillafanez
Copy link
Member

Wrong link? Backport doesn't seem related.

@PVince81
Copy link
Contributor Author

oh lala, major browser tab fail. I'll fix

@jvillafanez
Copy link
Member

When uploading a folder with several folders and files inside them, the files are not uploaded.

Folder upload seems to be be supported only in chrome. It's very likely a browser limitation.

@SergioBertolinSG
Copy link
Contributor

Using chrome I mean.

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 31, 2017

  • BUG: progress bar does not disappear when uploading a folder (chrome) into the current dir

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 31, 2017

I had a look at this and it's more complex, to be fixed separately. This is not related to the jquery.fileupload library update not recent changes. It is likely present since we moved to Webdav for file operations.

@PVince81
Copy link
Contributor Author

@SergioBertolinSG please retest

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 31, 2017

  • fix js tests

Vincent Petry added 2 commits August 31, 2017 15:44
When uploading multiple files into a target folder, sometimes the
progress bar does not disappear.
@PVince81 PVince81 force-pushed the fix-filelist-drop-target branch from bab8bc8 to 8a69278 Compare August 31, 2017 13:44
@PVince81
Copy link
Contributor Author

tests fixed

@SergioBertolinSG
Copy link
Contributor

The estimation of the upload bar is very general (few seconds, minutes, etc) has it changed in this PR?

@SergioBertolinSG
Copy link
Contributor

I guess it didn't so it works fine.

Copy link
Contributor

@SergioBertolinSG SergioBertolinSG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine.

@PVince81
Copy link
Contributor Author

PVince81 commented Sep 1, 2017

@SergioBertolinSG the estimate was never good...

Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good

@PVince81
Copy link
Contributor Author

PVince81 commented Sep 1, 2017

stable10: #28882

@PVince81 PVince81 merged commit eb2ba34 into master Sep 1, 2017
@PVince81 PVince81 deleted the fix-filelist-drop-target branch September 1, 2017 13:22
@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4 - To release p2-high Escalation, on top of current planning, release blocker status/STALE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drag and drop into subfolder uploads to current folder
4 participants