fix: only call drag start and end outputs when the element is actually dragged

BREAKING CHANGE: drag start and end events are now only called when the element is actually dragged,
use regular mousedown and mouseup events to get the old behaviour

Fixes #21
Fixes #20

Author: Matt Lewis

src/draggable.directive.ts

@@ -116,18 +116,8 @@ export class DraggableDirective implements OnInit, OnChanges, OnDestroy {
     const pointerDrag: Observable<any> = this.pointerDown
       .filter(() => this.canDrag())
       .flatMap((pointerDownEvent: PointerEvent) => {
-        pointerDownEvent.event.preventDefault();
-
-        this.zone.run(() => {
-          this.dragStart.next({ x: 0, y: 0 });
-        });
-
-        this.setCursor(this.dragCursor);
-
         const currentDrag: Subject<any> = new Subject();
 
-        this.draggableHelper.currentDrag.next(currentDrag);
-
         const pointerMove: Observable<Coordinates> = this.pointerMove
           .map((pointerMoveEvent: PointerEvent) => {
             pointerMoveEvent.event.preventDefault();
@@ -169,7 +159,20 @@ export class DraggableDirective implements OnInit, OnChanges, OnDestroy {
           .filter(
             ({ x, y }) => !this.validateDrag || this.validateDrag({ x, y })
           )
-          .takeUntil(Observable.merge(this.pointerUp, this.pointerDown));
+          .takeUntil(Observable.merge(this.pointerUp, this.pointerDown))
+          .share();
+
+        pointerMove.take(1).subscribe(() => {
+          pointerDownEvent.event.preventDefault();
+
+          this.zone.run(() => {
+            this.dragStart.next({ x: 0, y: 0 });
+          });
+
+          this.setCursor(this.dragCursor);
+
+          this.draggableHelper.currentDrag.next(currentDrag);
+        });
 
         pointerMove.takeLast(1).subscribe(({ x, y }) => {
           this.zone.run(() => {
@@ -186,8 +189,6 @@ export class DraggableDirective implements OnInit, OnChanges, OnDestroy {
           }
         });
 
-        this.pointerMove.next(pointerDownEvent);
-
         return pointerMove;
       })
       .share();

test/draggable.directive.spec.ts

@@ -58,11 +58,11 @@ describe('draggable directive', () => {
     const draggableElement: HTMLElement =
       fixture.componentInstance.draggable.element.nativeElement;
     triggerDomEvent('mousedown', draggableElement, { clientX: 5, clientY: 10 });
+    triggerDomEvent('mousemove', draggableElement, { clientX: 7, clientY: 10 });
     expect(fixture.componentInstance.dragStart).to.have.been.calledWith({
       x: 0,
       y: 0
     });
-    triggerDomEvent('mousemove', draggableElement, { clientX: 7, clientY: 10 });
     expect(fixture.componentInstance.dragging).to.have.been.calledWith({
       x: 2,
       y: 0
@@ -284,11 +284,11 @@ describe('draggable directive', () => {
     const draggableElement: HTMLElement =
       fixture.componentInstance.draggable.element.nativeElement;
     triggerDomEvent('mousedown', draggableElement, { clientX: 5, clientY: 10 });
+    triggerDomEvent('mousemove', draggableElement, { clientX: 7, clientY: 10 });
     expect(fixture.componentInstance.dragStart).to.have.been.calledWith({
       x: 0,
       y: 0
     });
-    triggerDomEvent('mousemove', draggableElement, { clientX: 7, clientY: 10 });
     expect(fixture.componentInstance.dragging).to.have.been.calledWith({
       x: 2,
       y: 0
@@ -354,21 +354,23 @@ describe('draggable directive', () => {
     const draggableElement: HTMLElement =
       fixture.componentInstance.draggable.element.nativeElement;
     triggerDomEvent('mousedown', draggableElement, { clientX: 5, clientY: 10 });
-    expect(fixture.componentInstance.dragStart).to.have.been.calledWith({
-      x: 0,
-      y: 0
-    });
     triggerDomEvent('mousemove', draggableElement, { clientX: 3, clientY: 10 });
+    expect(fixture.componentInstance.dragStart).not.to.have.been.called;
     expect(fixture.componentInstance.dragging).not.to.have.been.calledWith({
       x: -2,
       y: 0
     });
     triggerDomEvent('mousemove', draggableElement, { clientX: 7, clientY: 8 });
+    expect(fixture.componentInstance.dragStart).not.to.have.been.called;
     expect(fixture.componentInstance.dragging).not.to.have.been.calledWith({
       x: 2,
       y: -2
     });
     triggerDomEvent('mousemove', draggableElement, { clientX: 7, clientY: 12 });
+    expect(fixture.componentInstance.dragStart).to.have.been.calledWith({
+      x: 0,
+      y: 0
+    });
     expect(fixture.componentInstance.dragging).to.have.been.calledWith({
       x: 2,
       y: 2
@@ -424,13 +426,13 @@ describe('draggable directive', () => {
     triggerDomEvent('touchstart', draggableElement, {
       touches: [{ clientX: 5, clientY: 10 }]
     });
+    triggerDomEvent('touchmove', draggableElement, {
+      targetTouches: [{ clientX: 7, clientY: 10 }]
+    });
     expect(fixture.componentInstance.dragStart).to.have.been.calledWith({
       x: 0,
       y: 0
     });
-    triggerDomEvent('touchmove', draggableElement, {
-      targetTouches: [{ clientX: 7, clientY: 10 }]
-    });
     expect(fixture.componentInstance.dragging).to.have.been.calledWith({
       x: 2,
       y: 0
@@ -460,13 +462,13 @@ describe('draggable directive', () => {
     triggerDomEvent('touchstart', draggableElement, {
       touches: [{ clientX: 5, clientY: 10 }]
     });
+    triggerDomEvent('touchmove', draggableElement, {
+      targetTouches: [{ clientX: 7, clientY: 10 }]
+    });
     expect(fixture.componentInstance.dragStart).to.have.been.calledWith({
       x: 0,
       y: 0
     });
-    triggerDomEvent('touchmove', draggableElement, {
-      targetTouches: [{ clientX: 7, clientY: 10 }]
-    });
     expect(fixture.componentInstance.dragging).to.have.been.calledWith({
       x: 2,
       y: 0
@@ -520,22 +522,6 @@ describe('draggable directive', () => {
     ).to.equal(touchMoveUnsubscribe);
   });
 
-  it('should fire the dragEnd event when starting a drag but not actually moving it', () => {
-    const draggableElement: HTMLElement =
-      fixture.componentInstance.draggable.element.nativeElement;
-    triggerDomEvent('mousedown', draggableElement, { clientX: 5, clientY: 10 });
-    expect(fixture.componentInstance.dragStart).to.have.been.calledWith({
-      x: 0,
-      y: 0
-    });
-    triggerDomEvent('mouseup', draggableElement, { clientX: 5, clientY: 10 });
-    expect(fixture.componentInstance.dragEnd).to.have.been.calledWith({
-      x: 0,
-      y: 0
-    });
-    expect(draggableElement.style.transform).not.to.be.ok;
-  });
-
   it('should disable the mouse move cursor', () => {
     fixture.componentInstance.dragCursor = '';
     fixture.detectChanges();
@@ -546,4 +532,28 @@ describe('draggable directive', () => {
     triggerDomEvent('mouseleave', draggableElement);
     expect(draggableElement.style.cursor).not.to.be.ok;
   });
+
+  it('should not call dragStart and dragEnd events when clicking on the draggable', () => {
+    const draggableElement: HTMLElement =
+      fixture.componentInstance.draggable.element.nativeElement;
+    triggerDomEvent('mousedown', draggableElement, { clientX: 5, clientY: 10 });
+    expect(fixture.componentInstance.dragStart).not.to.have.been.called;
+    expect(fixture.componentInstance.dragging).not.to.have.been.called;
+    triggerDomEvent('mouseup', draggableElement, { clientX: 5, clientY: 10 });
+    expect(fixture.componentInstance.dragEnd).not.to.have.been.called;
+    expect(draggableElement.style.transform).not.to.be.ok;
+  });
+
+  it('should call the drag start, dragging and end events in order', () => {
+    const draggableElement: HTMLElement =
+      fixture.componentInstance.draggable.element.nativeElement;
+    triggerDomEvent('mousedown', draggableElement, { clientX: 5, clientY: 10 });
+    triggerDomEvent('mousemove', draggableElement, { clientX: 7, clientY: 10 });
+    triggerDomEvent('mouseup', draggableElement, { clientX: 7, clientY: 8 });
+    sinon.assert.callOrder(
+      fixture.componentInstance.dragStart,
+      fixture.componentInstance.dragging,
+      fixture.componentInstance.dragEnd
+    );
+  });
 });