fix(drag-drop): fix drag start delay behavior to allow scrolling (#16224) #16228

Aboisier · 2019-06-07T13:27:56Z

The current implementation of the drag start delay does not allow scrolling on mobile devices.
Instead, the draggable element gets teleported to the cursor once the delay is elapsed.

In order to handle this use case, we cancel the drag sequence if the cursor moves before
the drag start delay is elapsed and we disable native drag interactions only when the
drag sequence is started instead of when it is initialized.

The drag start delay was also integrated to the drag drop demo.

Fixes #16224

googlebot · 2019-06-07T13:28:00Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

Aboisier · 2019-06-10T20:18:00Z

I signed it!

googlebot · 2019-06-10T20:18:03Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Aboisier · 2019-06-18T12:59:34Z

Hey, @crisbeto ! I don't mean to be bothersome or anything, I noticed most PRs get reviewed within a few day. Since it's been almost two weeks since I created this PR, I was wondering if it fell between the cracks or if it was normal for outside contributions to take a bit longer.

Thanks a lot!

crisbeto · 2019-06-23T06:56:25Z

src/cdk/drag-drop/directives/drag.spec.ts

@@ -596,18 +596,28 @@ describe('CdkDrag', () => {
      expect(dragElement.style.transform).toBeFalsy();
    }));

-    it('should enable native drag interactions if dragging is disabled', fakeAsync(() => {
+    it('should enable native drag interactions if not dragging', fakeAsync(() => {


It seems like the old test case is still valid. Should we have one that checks when it's disabled and another one if not dragging?

That makes sense! I'll add it back. I foresee only one small tweak I'm going to have to do to the test; the webkitUserDrag will now be falsy by default. This is to allow scrolling.

crisbeto · 2019-06-23T06:58:27Z

src/cdk/drag-drop/directives/drag.spec.ts

@@ -798,22 +847,17 @@ describe('CdkDrag', () => {
      let currentTime = 0;

      const fixture = createComponent(StandaloneDraggable);
-      fixture.componentInstance.dragStartDelay = '1000';
+      fixture.componentInstance.dragStartDelay = '500';


The changes shouldn't have an effect on this test. Why did this need to be reduced?

The test used to:

Click (T 0)

Move the mouse (T 750)

Expect that the element did not move (T 750)

Move the mouse again (T 1250)

Expect the object to have moved (T 1250)

This did not reflect the new drag start delay behaviour I am suggesting. The first mouse move, because it is performed before the drag start delay, now cancels the drag sequence. The moving before the delay is elapsed is tested here. This test ensures that moving after the delay is elapsed moves the element.

crisbeto · 2019-06-23T07:02:17Z

src/cdk/drag-drop/drag-ref.ts

@@ -504,12 +504,18 @@ export class DragRef<T = any> {
      const distanceX = Math.abs(pointerPosition.x - this._pickupPositionOnPage.x);
      const distanceY = Math.abs(pointerPosition.y - this._pickupPositionOnPage.y);
      const isOverThreshold = distanceX + distanceY >= this._config.dragStartThreshold;
+      const isDelayElapsed = (Date.now() >= this._dragStartTime + (this.dragStartDelay || 0));


You don't need the parentheses around the entire expression here.

crisbeto · 2019-06-23T07:06:32Z

src/cdk/drag-drop/drag-ref.ts


      // Only start dragging after the user has moved more than the minimum distance in either
      // direction. Note that this is preferrable over doing something like `skip(minimumDistance)`
      // in the `pointerMove` subscription, because we're not guaranteed to have one move event
      // per pixel of movement (e.g. if the user moves their pointer quickly).
-      if (isOverThreshold && (Date.now() >= this._dragStartTime + (this.dragStartDelay || 0))) {
+      if (isOverThreshold) {
+        if (!isDelayElapsed) {


If I'm following it correctly, it seems like we'll keep starting and ending dragging sequences until the delay has elapsed. Don't we want to not start the sequence in the first place? This feels like it could lead some issues like events being dispatched when they shouldn't be.

If the pointer moves before the drag start delay has elapsed, then the drag sequence is ended. Since ending the sequence removes the subscription to pointermove, this method won't be called until the user releases the click and initializes a new sequence. This means the sequence will not be started/ended a bunch of times. At least, that's my understanding. I might be missing something here!

Alright, I tried it out and it doesn't seem to be an issue.

crisbeto

Sorry for the delays, the notification for this keeps getting buried in other emails. Left a few more comments, but it should be good to go afterwards.

crisbeto · 2019-06-30T07:50:34Z

src/cdk/drag-drop/drag-ref.ts

+   * Clears subscriptions and stops the dragging sequence.
+   * @param event Browser event object that ended the sequence.
+   */
+  private _endDragSequence = (event: MouseEvent | TouchEvent) => {


This doesn't have to be an arrow function since we're not binding it as an event listener. You can make it into a regular method.

crisbeto · 2019-06-30T07:54:13Z

src/cdk/drag-drop/drag-ref.ts

@@ -504,12 +504,18 @@ export class DragRef<T = any> {
      const distanceX = Math.abs(pointerPosition.x - this._pickupPositionOnPage.x);
      const distanceY = Math.abs(pointerPosition.y - this._pickupPositionOnPage.y);
      const isOverThreshold = distanceX + distanceY >= this._config.dragStartThreshold;
+      const isDelayElapsed = (Date.now() >= this._dragStartTime + this.dragStartDelay || 0);


This can be moved inside the isOverThreshold check and potentially inlined.

crisbeto · 2019-06-30T07:57:37Z

src/cdk/drag-drop/drag-ref.ts


      // Only start dragging after the user has moved more than the minimum distance in either
      // direction. Note that this is preferrable over doing something like `skip(minimumDistance)`
      // in the `pointerMove` subscription, because we're not guaranteed to have one move event
      // per pixel of movement (e.g. if the user moves their pointer quickly).
-      if (isOverThreshold && (Date.now() >= this._dragStartTime + (this.dragStartDelay || 0))) {
+      if (isOverThreshold) {
+        if (!isDelayElapsed) {


Alright, I tried it out and it doesn't seem to be an issue.

crisbeto · 2019-06-30T08:00:02Z

src/dev-app/drag-drop/drag-drop-demo.html

+  <h2>Drag start delay</h2>
+
+  <mat-form-field>
+    <input matInput placeholder="Drag start delay" value="100" [(ngModel)]="dragStartDelay">


Can we leave this at 0 so it matches the default?

crisbeto · 2019-06-30T08:01:01Z

src/dev-app/drag-drop/drag-drop-demo.ts

@@ -21,6 +21,7 @@ import {CdkDragDrop, moveItemInArray, transferArrayItem} from '@angular/cdk/drag
 })
 export class DragAndDropDemo {
  axisLock: 'x' | 'y';
+  dragStartDelay: number = 0;


You can omit the : number here. We're only explicit about it in the source code so that our doc generation tooling can pick it up.

Aboisier · 2019-07-03T14:24:47Z

Don't worry about it, thanks for the review! I believe I addressed all your comments. It seems like there's an issue with the build though. I assume it will be fixed by #16436 .

EDIT: It indeed fixed it. 👍

) The current implementation of the drag start delay does not allow scrolling on mobile devices. Instead, the draggable element gets teleported to the cursor once the delay is elapsed. In order to handle this use case, we cancel the drag sequence if the cursor moves before the drag start delay is elapsed and we disable native drag interactions only when the drag sequence is started instead of when it is initialized. The drag start delay was also integrated to the drag drop demo. Fixes #16224

) Addresses PR comments. Adds a test, removes unnecessary parentheses.

) Addresses PR comments

crisbeto

LGTM

…ular#16228) The current implementation of the drag start delay does not allow scrolling on mobile devices. Instead, the draggable element gets teleported to the cursor once the delay is elapsed. In order to handle this use case, we cancel the drag sequence if the cursor moves before the drag start delay is elapsed and we disable native drag interactions only when the drag sequence is started instead of when it is initialized. The drag start delay was also integrated to the drag drop demo. Fixes angular#16224 (cherry picked from commit 738f10c)

angular-automatic-lock-bot · 2019-09-11T01:02:54Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

Aboisier requested a review from crisbeto as a code owner June 7, 2019 13:27

googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Jun 7, 2019

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jun 10, 2019

crisbeto reviewed Jun 23, 2019

View reviewed changes

crisbeto reviewed Jun 30, 2019

View reviewed changes

Antoine Boisier-Michaud added 3 commits July 3, 2019 11:10

fix(drag-drop): fix drag start delay behavior to allow scrolling (#16224

1f38e31

) Addresses PR comments. Adds a test, removes unnecessary parentheses.

fix(drag-drop): fix drag start delay behavior to allow scrolling (#16224

dd53d41

) Addresses PR comments

crisbeto approved these changes Jul 5, 2019

View reviewed changes

crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jul 5, 2019

jelbourn merged commit 738f10c into angular:master Jul 9, 2019

Aboisier deleted the drag-and-drop-start-delay-fix branch July 9, 2019 16:25

Aboisier mentioned this pull request Jul 16, 2019

fix(drag-drop): fix drag start delay behavior to allow scrolling (#16224) #16537

Closed

crisbeto mentioned this pull request Aug 4, 2019

cdkDragStartDelay behavior has changed in version 8.1.0 #16614

Open

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

fix(drag-drop): fix drag start delay behavior to allow scrolling (#16224) #16228

fix(drag-drop): fix drag start delay behavior to allow scrolling (#16224) #16228

Uh oh!

Conversation

Aboisier commented Jun 7, 2019

Uh oh!

googlebot commented Jun 7, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Aboisier commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Jun 10, 2019

Uh oh!

Aboisier commented Jun 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aboisier Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aboisier Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aboisier commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 11, 2019

Uh oh!

Uh oh!

Aboisier commented Jun 10, 2019 •

edited

Loading

Aboisier Jun 24, 2019 •

edited

Loading

Aboisier Jun 24, 2019 •

edited

Loading

Aboisier commented Jul 3, 2019 •

edited

Loading