[#43577] Fix drag and drop by counting drag over and leave (#11896)

* [#43577] Fix drag and drop by counting drag over and leave

- https://community.openproject.org/work_packages/43577
- replace action button text for attaching files
- refactored drag and drop behaviour
  - on any global dragenter increase a counter
  - on any global dragleave decrease counter
  - if counter is 0 -> no global dragging is happening (mouse was moved out of window)
  - this way, we are independent from the order in which the DragEvents are fired by the browser (bigger differences between Firefox and Chrome)

Co-authored-by: Benjamin Bädorf <b.baedorf@openproject.com>

config/locales/js-en.yml

@@ -520,7 +520,7 @@ en:
     label_drop_files: "Drop files here to attach files."
     label_drop_or_click_files: "Drop files here or click to attach files."
     label_drop_folders_hint: You cannot upload folders as an attachment. Please select single files.
-    label_add_attachments: "Add attachments"
+    label_add_attachments: "Attach files"
     label_formattable_attachment_hint: "Attach and link files by dropping on this field, or pasting from the clipboard."
     label_remove_file: "Delete %{fileName}"
     label_remove_watcher: "Remove watcher %{name}"

frontend/src/app/shared/components/attachments/attachments.component.html

@@ -1,7 +1,7 @@
 <op-attachment-list
   [ngClass]="{
     'op-file-section--list': true,
-    'op-file-section--list_dragging': dragging
+    'op-file-section--list_dragging': dragging > 0
   }"
   *ngIf="(attachments$ | async).length"
   [attachments]="attachments$ | async"
@@ -26,7 +26,7 @@
   [ngClass]="{
     'hide-when-print': true,
     'op-file-section--drop-box': true,
-    'op-file-section--drop-box_dragging': dragging,
+    'op-file-section--drop-box_dragging': dragging > 0,
     'op-file-section--drop-box_dragging-over': draggingOverDropZone,
     'op-file-section--drop-box_float': (attachments$ | async).length
   }"

frontend/src/app/shared/components/attachments/attachments.component.ts

@@ -38,20 +38,21 @@ import {
   ViewChild,
   ViewEncapsulation,
 } from '@angular/core';
+import { Observable } from 'rxjs';
+import { filter, map, tap } from 'rxjs/operators';
+
 import { HalResource } from 'core-app/features/hal/resources/hal-resource';
 import { HalResourceService } from 'core-app/features/hal/services/hal-resource.service';
 import { I18nService } from 'core-app/core/i18n/i18n.service';
 import { States } from 'core-app/core/states/states.service';
-import { filter, map, tap } from 'rxjs/operators';
 import { UntilDestroyedMixin } from 'core-app/shared/helpers/angular/until-destroyed.mixin';
 import { populateInputsFromDataset } from 'core-app/shared/components/dataset-inputs';
 import { UploadFile } from 'core-app/core/file-upload/op-file-upload.service';
 import { AttachmentsResourceService } from 'core-app/core/state/attachments/attachments.service';
 import { ToastService } from 'core-app/shared/components/toaster/toast.service';
 import { TimezoneService } from 'core-app/core/datetime/timezone.service';
-import isNewResource from 'core-app/features/hal/helpers/is-new-resource';
 import { IAttachment } from 'core-app/core/state/attachments/attachment.model';
-import { Observable } from 'rxjs';
+import isNewResource from 'core-app/features/hal/helpers/is-new-resource';
 
 function containsFiles(dataTransfer:DataTransfer):boolean {
   return dataTransfer.types.indexOf('Files') >= 0;
@@ -82,7 +83,7 @@ export class OpAttachmentsComponent extends UntilDestroyedMixin implements OnIni
 
   public draggingOverDropZone = false;
 
-  public dragging = false;
+  public dragging = 0;
 
   @ViewChild('hiddenFileInput') public filePicker:ElementRef<HTMLInputElement>;
 
@@ -95,14 +96,29 @@ export class OpAttachmentsComponent extends UntilDestroyedMixin implements OnIni
   };
 
   private get attachmentsSelfLink():string {
-    const attachments = this.resource.attachments as unknown&{ href:string };
+    const attachments = this.resource.attachments as unknown & { href:string };
     return attachments.href;
   }
 
-  private get collectionKey():string {
+  public get collectionKey():string {
     return isNewResource(this.resource) ? 'new' : this.attachmentsSelfLink;
   }
 
+  private onGlobalDragLeave:(_event:DragEvent) => void = (_event) => {
+    this.dragging = Math.max(this.dragging - 1, 0);
+    this.cdRef.detectChanges();
+  };
+
+  private onGlobalDragEnd:(_event:DragEvent) => void = (_event) => {
+    this.dragging = 0;
+    this.cdRef.detectChanges();
+  };
+
+  private onGlobalDragEnter:(_event:DragEvent) => void = (_event) => {
+    this.dragging += 1;
+    this.cdRef.detectChanges();
+  };
+
   constructor(
     public elementRef:ElementRef,
     protected readonly I18n:I18nService,
@@ -159,15 +175,17 @@ export class OpAttachmentsComponent extends UntilDestroyedMixin implements OnIni
         }),
       );
 
-    document.body.addEventListener('dragover', this.onGlobalDragOver.bind(this));
-    document.body.addEventListener('dragleave', this.onGlobalDragEnd.bind(this));
-    document.body.addEventListener('drop', this.onGlobalDragEnd.bind(this));
+    document.body.addEventListener('dragenter', this.onGlobalDragEnter);
+    document.body.addEventListener('dragleave', this.onGlobalDragLeave);
+    document.body.addEventListener('dragend', this.onGlobalDragEnd);
+    document.body.addEventListener('drop', this.onGlobalDragEnd);
   }
 
   ngOnDestroy():void {
-    document.body.removeEventListener('dragover', this.onGlobalDragOver.bind(this));
-    document.body.removeEventListener('dragleave', this.onGlobalDragEnd.bind(this));
-    document.body.removeEventListener('drop', this.onGlobalDragEnd.bind(this));
+    document.body.removeEventListener('dragenter', this.onGlobalDragEnter);
+    document.body.removeEventListener('dragleave', this.onGlobalDragLeave);
+    document.body.removeEventListener('dragend', this.onGlobalDragEnd);
+    document.body.removeEventListener('drop', this.onGlobalDragEnd);
   }
 
   public triggerFileInput():void {
@@ -198,7 +216,7 @@ export class OpAttachmentsComponent extends UntilDestroyedMixin implements OnIni
 
     this.uploadFiles(files);
     this.draggingOverDropZone = false;
-    this.dragging = false;
+    this.dragging = 0;
   }
 
   public onDragOver(event:DragEvent):void {
@@ -213,18 +231,6 @@ export class OpAttachmentsComponent extends UntilDestroyedMixin implements OnIni
     this.draggingOverDropZone = false;
   }
 
-  public onGlobalDragEnd():void {
-    this.dragging = false;
-
-    this.cdRef.detectChanges();
-  }
-
-  public onGlobalDragOver():void {
-    this.dragging = true;
-
-    this.cdRef.detectChanges();
-  }
-
   protected uploadFiles(files:UploadFile[]):void {
     let uploadFiles = files || [];
     const countBefore = files.length;

frontend/src/app/shared/components/storages/storage/storage.component.html

@@ -30,7 +30,7 @@
       class="spot-list spot-list_compact op-file-list"
       [ngClass]="{
         'op-file-section': true,
-        'op-file-section--list_dragging': dragging
+        'op-file-section--list_dragging': dragging > 0
       }"
       data-qa-selector="file-list"
     >
@@ -59,7 +59,7 @@
       [ngClass]="{
         'hide-when-print': true,
         'op-file-section--drop-box': true,
-        'op-file-section--drop-box_dragging': dragging,
+        'op-file-section--drop-box_dragging': dragging > 0,
         'op-file-section--drop-box_dragging-over': draggingOverDropZone,
         'op-file-section--drop-box_float': (fileLinks$ | async).length
       }"

frontend/src/app/shared/components/storages/storage/storage.component.ts

@@ -97,7 +97,7 @@ export class StorageComponent extends UntilDestroyedMixin implements OnInit, OnD
 
   draggingOverDropZone = false;
 
-  dragging = false;
+  dragging = 0;
 
   private isLoggedIn = false;
 
@@ -130,6 +130,21 @@ export class StorageComponent extends UntilDestroyedMixin implements OnInit, OnD
     return this.storage._links.open.href;
   }
 
+  private onGlobalDragLeave:(_event:DragEvent) => void = (_event) => {
+    this.dragging = Math.max(this.dragging - 1, 0);
+    this.cdRef.detectChanges();
+  };
+
+  private onGlobalDragEnd:(_event:DragEvent) => void = (_event) => {
+    this.dragging = 0;
+    this.cdRef.detectChanges();
+  };
+
+  private onGlobalDragEnter:(_event:DragEvent) => void = (_event) => {
+    this.dragging += 1;
+    this.cdRef.detectChanges();
+  };
+
   constructor(
     private readonly i18n:I18nService,
     private readonly cdRef:ChangeDetectorRef,
@@ -167,17 +182,19 @@ export class StorageComponent extends UntilDestroyedMixin implements OnInit, OnD
 
     this.allowEditing$ = this
       .currentUserService
-      .hasCapabilities$('file_links/manage', (this.resource.project as unknown&{ id:string }).id);
+      .hasCapabilities$('file_links/manage', (this.resource.project as unknown & { id:string }).id);
 
-    document.body.addEventListener('dragover', this.onGlobalDragOver.bind(this));
-    document.body.addEventListener('dragleave', this.afterGlobalDragEnd.bind(this));
-    document.body.addEventListener('drop', this.afterGlobalDragEnd.bind(this));
+    document.body.addEventListener('dragenter', this.onGlobalDragEnter);
+    document.body.addEventListener('dragleave', this.onGlobalDragLeave);
+    document.body.addEventListener('dragend', this.onGlobalDragEnd);
+    document.body.addEventListener('drop', this.onGlobalDragEnd);
   }
 
   ngOnDestroy():void {
-    document.body.removeEventListener('dragover', this.onGlobalDragOver.bind(this));
-    document.body.removeEventListener('dragleave', this.afterGlobalDragEnd.bind(this));
-    document.body.removeEventListener('drop', this.afterGlobalDragEnd.bind(this));
+    document.body.removeEventListener('dragenter', this.onGlobalDragEnter);
+    document.body.removeEventListener('dragleave', this.onGlobalDragLeave);
+    document.body.removeEventListener('dragend', this.onGlobalDragEnd);
+    document.body.removeEventListener('drop', this.onGlobalDragEnd);
   }
 
   public removeFileLink(fileLink:IFileLink):void {
@@ -194,7 +211,7 @@ export class StorageComponent extends UntilDestroyedMixin implements OnInit, OnD
           storageName: this.storage.name,
           storageLocation: this.storageFilesLocation,
           storageLink: this.storage._links.self,
-          addFileLinksHref: (this.resource.$links as unknown&{ addFileLink:IHalResourceLink }).addFileLink.href,
+          addFileLinksHref: (this.resource.$links as unknown & { addFileLink:IHalResourceLink }).addFileLink.href,
           collectionKey: this.collectionKey,
           fileLinks,
         };
@@ -280,7 +297,7 @@ export class StorageComponent extends UntilDestroyedMixin implements OnInit, OnD
   }
 
   private get fileLinkSelfLink():string {
-    const fileLinks = this.resource.fileLinks as unknown&{ href:string };
+    const fileLinks = this.resource.fileLinks as unknown & { href:string };
     return `${fileLinks.href}?filters=[{"storage":{"operator":"=","values":["${this.storage.id}"]}}]`;
   }
 
@@ -302,7 +319,7 @@ export class StorageComponent extends UntilDestroyedMixin implements OnInit, OnD
     if (event.dataTransfer === null) return;
 
     this.draggingOverDropZone = false;
-    this.dragging = false;
+    this.dragging = 0;
   }
 
   public onDragOver(event:DragEvent):void {
@@ -316,18 +333,6 @@ export class StorageComponent extends UntilDestroyedMixin implements OnInit, OnD
   public onDragLeave(_event:DragEvent):void {
     this.draggingOverDropZone = false;
   }
-
-  public afterGlobalDragEnd():void {
-    this.dragging = false;
-
-    this.cdRef.detectChanges();
-  }
-
-  public onGlobalDragOver():void {
-    this.dragging = true;
-
-    this.cdRef.detectChanges();
-  }
 }
 
 function containsFiles(dataTransfer:DataTransfer):boolean {

modules/budgets/spec/features/budgets/attachment_upload_spec.rb

@@ -31,15 +31,12 @@ require 'features/page_objects/notification'
 
 describe 'Upload attachment to budget', js: true do
   let(:user) do
-    create :user,
-           member_in_project: project,
-           member_with_permissions: %i[view_budgets
-                                       edit_budgets]
+    create(:user, member_in_project: project, member_with_permissions: %i[view_budgets edit_budgets])
   end
   let(:project) { create(:project) }
-  let(:attachments) { ::Components::Attachments.new }
-  let(:image_fixture) { ::UploadedFile.load_from('spec/fixtures/files/image.png') }
-  let(:editor) { ::Components::WysiwygEditor.new }
+  let(:attachments) { Components::Attachments.new }
+  let(:image_fixture) { UploadedFile.load_from('spec/fixtures/files/image.png') }
+  let(:editor) { Components::WysiwygEditor.new }
 
   before do
     login_as(user)
@@ -105,7 +102,7 @@ describe 'Upload attachment to budget', js: true do
     end
 
     script = <<~JS
-      const event = new DragEvent('dragover');
+      const event = new DragEvent('dragenter');
       document.body.dispatchEvent(event);
     JS
     page.execute_script(script)

modules/documents/spec/features/attachment_upload_spec.rb

@@ -35,24 +35,24 @@ describe 'Upload attachment to documents',
            journal_aggregation_time_minutes: 0
          } do
   let!(:user) do
-    create :user,
+    create(:user,
            member_in_project: project,
            member_with_permissions: %i[view_documents
-                                       manage_documents]
+                                       manage_documents])
   end
   let!(:other_user) do
-    create :user,
+    create(:user,
            member_in_project: project,
            member_with_permissions: %i[view_documents],
-           notification_settings: [build(:notification_setting, all: true)]
+           notification_settings: [build(:notification_setting, all: true)])
   end
   let!(:category) do
     create(:document_category)
   end
   let(:project) { create(:project) }
-  let(:attachments) { ::Components::Attachments.new }
-  let(:image_fixture) { ::UploadedFile.load_from('spec/fixtures/files/image.png') }
-  let(:editor) { ::Components::WysiwygEditor.new }
+  let(:attachments) { Components::Attachments.new }
+  let(:image_fixture) { UploadedFile.load_from('spec/fixtures/files/image.png') }
+  let(:editor) { Components::WysiwygEditor.new }
 
   before do
     login_as(user)
@@ -86,7 +86,7 @@ describe 'Upload attachment to documents',
       expect(page).to have_selector('#content img', count: 1)
       expect(page).to have_content('Image uploaded on creation')
 
-      document = ::Document.last
+      document = Document.last
       expect(document.title).to eq 'New documentation'
 
       # Expect it to be present on the show page
@@ -113,7 +113,7 @@ describe 'Upload attachment to documents',
       scroll_to_element(page.find('[data-qa-selector="op-attachments"]'))
 
       script = <<~JS
-        const event = new DragEvent('dragover');
+        const event = new DragEvent('dragenter');
         document.body.dispatchEvent(event);
       JS
       page.execute_script(script)
@@ -155,7 +155,7 @@ describe 'Upload attachment to documents',
     it_behaves_like 'can upload an image'
   end
 
-  context 'internal upload', with_direct_uploads: false do
+  context 'for internal uploads', with_direct_uploads: false do
     it_behaves_like 'can upload an image'
   end
 end

spec/features/forums/attachment_upload_spec.rb

@@ -32,16 +32,16 @@ require 'features/page_objects/notification'
 describe 'Upload attachment to forum message', js: true do
   let(:forum) { create(:forum) }
   let(:user) do
-    create :user,
+    create(:user,
            member_in_project: project,
            member_with_permissions: %i[view_messages
                                        add_messages
-                                       edit_messages]
+                                       edit_messages])
   end
   let(:project) { forum.project }
-  let(:attachments) { ::Components::Attachments.new }
+  let(:attachments) { Components::Attachments.new }
   let(:image_fixture) { UploadedFile.load_from('spec/fixtures/files/image.png') }
-  let(:editor) { ::Components::WysiwygEditor.new }
+  let(:editor) { Components::WysiwygEditor.new }
   let(:index_page) { Pages::Messages::Index.new(forum.project) }
 
   before do
@@ -116,7 +116,7 @@ describe 'Upload attachment to forum message', js: true do
     end
 
     script = <<~JS
-      const event = new DragEvent('dragover');
+      const event = new DragEvent('dragenter');
       document.body.dispatchEvent(event);
     JS
     page.execute_script(script)

spec/features/wiki/attachment_upload_spec.rb

@@ -31,14 +31,14 @@ require 'features/page_objects/notification'
 
 describe 'Upload attachment to wiki page', js: true do
   let(:user) do
-    create :user,
+    create(:user,
            member_in_project: project,
-           member_with_permissions: %i[view_wiki_pages edit_wiki_pages]
+           member_with_permissions: %i[view_wiki_pages edit_wiki_pages])
   end
   let(:project) { create(:project) }
-  let(:attachments) { ::Components::Attachments.new }
+  let(:attachments) { Components::Attachments.new }
   let(:image_fixture) { UploadedFile.load_from('spec/fixtures/files/image.png') }
-  let(:editor) { ::Components::WysiwygEditor.new }
+  let(:editor) { Components::WysiwygEditor.new }
   let(:wiki_page_content) { project.wiki.pages.first.content.text }
 
   before do
@@ -79,7 +79,7 @@ describe 'Upload attachment to wiki page', js: true do
     editor.in_editor do |container, _|
       # Expect URL is mapped to the correct URL
       expect(container).to have_selector('img[src^="/api/v3/attachments/"]')
-      expect(container).to have_no_selector('img[src="image.png"]')
+      expect(container).not_to have_selector('img[src="image.png"]')
 
       container.find('img[src^="/api/v3/attachments/"]', match: :first).click
     end
@@ -129,7 +129,7 @@ describe 'Upload attachment to wiki page', js: true do
     expect(page).to have_selector('.ck-editor__editable', wait: 5)
 
     script = <<~JS
-      const event = new DragEvent('dragover');
+      const event = new DragEvent('dragenter');
       document.body.dispatchEvent(event);
     JS
     page.execute_script(script)