components icon indicating copy to clipboard operation
components copied to clipboard

bug(cdkDrag): using cdkDragConstrainPosition causes erratic behavior

Open ma7moudat opened this issue 2 years ago • 14 comments

Is this a regression?

  • [X] Yes, this behavior used to work in the previous version

The previous version in which this bug was not present was

13.3.9

Description

cdkDrag works normally with box and axis constraints, but as soon as cdkDragConstrainPosition is used, the dragged element incorrectly jumps a few pixels down the y-axis.

Reproduction

Please refer to the reproduction:

https://angular-ivy-iry1fk.stackblitz.io

https://stackblitz.com/edit/angular-ivy-iry1fk?file=package.json

Expected Behavior

The dragged element should only move horizontally and not jump down the y-axis.

Actual Behavior

The element jumps a few pixels down.

Environment

  • Angular: 14.0.0
  • CDK/Material: 14.0.0
  • Browser(s): Chrome / Firefox
  • Operating System (e.g. Windows, macOS, Ubuntu): Windows

ma7moudat avatar Jun 24 '22 13:06 ma7moudat

This is likely the result of #25061 which made it so that the position constrain function determines the top/left coordinates of the element. You see it jump down a little because the "normal" dragging behavior also accounts for the pick up position within the element whereas the one returned from the constrain callback is the top/left. This is how the constrain position input was intended to work, but a poorly written unit test meant that it had broken at some point and we hadn't noticed.

crisbeto avatar Jun 25 '22 05:06 crisbeto

I must admit that I at this point cannot figure out how to use the cdkDragConstrainPosition after #25061 was merged. We had a working implementation with the old behavior.

I understand that the given point is now where the mouse is dragging on the page, and just returning the point will make the element render with the top/left at exactly that point. But even if I just offset that by say 10px, it seems to make the whole drag/drop experience act weird.

Maybe it is just missing documentation / information about how one is expected to implement the constraining logic I'm missing, but to me, the old behavior was at least a bit more intuitive.

mj3052 avatar Jun 27 '22 10:06 mj3052

This is likely the result of #25061 which made it so that the position constrain function determines the top/left coordinates of the element. You see it jump down a little because the "normal" dragging behavior also accounts for the pick up position within the element whereas the one returned from the constrain callback is the top/left. This is how the constrain position input was intended to work, but a poorly written unit test meant that it had broken at some point and we hadn't noticed.

Not sure I understand. Does that mean that it actually is a bug that needs fixing in cdkDrag? Or should I adjust my code to work with the new logic?

ma7moudat avatar Jun 27 '22 10:06 ma7moudat

The idea of that function has always been that the Point it returns is where you want the top/left corner of the dragged element to be. It's how it worked when I initially introduced it, but it regressed at some point. There was a unit test for it, but it was poorly written so it didn't catch the regression.

crisbeto avatar Jun 27 '22 10:06 crisbeto

Ah okay! Thanks. So if I understood correctly, it should work for me without the jumping if the Y of the returned point matches the getBoundingClientRect().top of the parent element (or the constraining element).

ma7moudat avatar Jun 27 '22 10:06 ma7moudat

@crisbeto I have the same problem, basically I want to use it for a split view, so I lock the axis in one direction and also want to constrain the position in the other direction. This is now next to impossible and is preventing us from updating to Angular 14. With Angular 13 it worked perfectly.

This is a simple example showing the problem: https://stackblitz.com/edit/angular-zsuwad-juyc6z?file=package.json,src%2Fapp%2Fcdk-drag-drop-axis-lock-example.html,src%2Fapp%2Fcdk-drag-drop-axis-lock-example.ts

With axes locked, the constraint causes erratic behavior.

ckorherr avatar Jul 12 '22 15:07 ckorherr

So the (not well documented) assumption is that if you're using constrainPosition, you're in complete control over the positioning. lockAxis and boundary may end messing with it since they're all modifying the same coordinates and can end up returning conflicting information.

In retrospect, lockAxis and boundary should've been implemented as functions for constrainPosition that are provided by cdk/drag-drop.

crisbeto avatar Jul 12 '22 19:07 crisbeto

Makes sense, I'll try that, thank you!

ckorherr avatar Jul 12 '22 20:07 ckorherr

Works perfect.

ckorherr avatar Jul 12 '22 20:07 ckorherr

The idea of that function has always been that the Point it returns is where you want the top/left corner of the dragged element to be. It's how it worked when I initially introduced it, but it regressed at some point. There was a unit test for it, but it was poorly written so it didn't catch the regression.

Can you provide an example how to calcuate returning point coordinates so it will drag the item without jumping to top left corner? So basicply the constrainPosition function will not do anything. From that point we can adjust the code for our needs.

`constrainPosition(point: Point, dragRef: DragRef) {

// calculate default beahviour. Move element from where you grab it.

return point;

}`

Thank you

ioanes avatar Jul 25 '22 11:07 ioanes

You'd have to offset it based on the position at which the user picked up the element, which I now realize isn't possible to read. I've opened up #25341 to expose it within the constrainPosition callback.

crisbeto avatar Jul 26 '22 07:07 crisbeto

Watch out guys/gals, just spent 2 hours debugging why my dragged element jumps like crazy and the reason is that you receive user pointer position and you're supposed to return top left corner position. So I guess not much has changed and still have to use pickup position in the calculations. At least now I no longer have to calculate it but receive it as the last function. It would be nice to actually receive the top left corner point, apply limiting function and return top left corner point.

kvetis avatar Aug 19 '22 10:08 kvetis

@ioanes To get the top left corner from the point, you need to subtract the pickup position in draggable from the Point you get as the first argument:

/** Fullfills cdkDragPosition input for constraining drag element. */
  constrainPosition: CdkDrag['constrainPosition'] = (
    { x, y }: Point,
    _ref,
    _dims,
    // You get the pickup position which is the offset from the top left corner of the image
    pickup: Point,
  ) => {

    // Keep whatever logic you had before v14.
   
    // Convert x and y to top left corner of the image
    x -= pickup.x;
    y -= pickup.y;

    return { x, y };
  };

You need to be on the latest version though, since @crisbeto added the pickup parameter a month ago. Before that, you had to calculate it yourself.

kvetis avatar Aug 23 '22 08:08 kvetis

Thanks for example code @kvetis ;)

Thats why I asked, since pickup position wasnt available in constrainPosition at that time and calculating it by myself when it can be exposed like @crisbeto did was a perfect solution.

Also Thanks @crisbeto for a quick response and fix.

ioanes avatar Aug 23 '22 09:08 ioanes

This bug also seems to impact other parts of cdkDrag. Specifically when cdkDragConstrainPosition when used with cdkDragBoundary also seems to have erratic behavior.

Reproduction See below https://angular-ivy-9unppr.stackblitz.io https://stackblitz.com/edit/angular-ivy-9unppr?file=src/app/app.component.html

Environment

Angular: 15.0.3
CDK/Material: 15.0.3
Browser(s): Chrome / Firefox
Operating System (e.g. Windows, macOS, Ubuntu): Windows

Expected Behavior Should stay within the drag boundary and be able to go to the edges of the boundary

Actual Behavior Seems that the drag boundary arbitrarily moves

HuntosShuntos avatar Dec 17 '22 16:12 HuntosShuntos

@crisbeto

Is there anything I/someone can do to get this moving forward somehow? Or at least get some clarification about what is going to happen with the strange behavior still seen in some cases. We are currently stuck building our own patched version using the old behavior before #25061 for our project, as we're using cdkDragBoundary with cdkDragConstrainPosition in quite a few places.

I am open to contribute in solving this if required. It seems the example of @HuntosShuntos is showing the problem we're having, which was introduced by #25061.

If cdkDragBoundary is not going to work together with cdkDragConstrainPosition I think it should at least be documented that this is the case.

mj3052 avatar Aug 28 '23 11:08 mj3052

@mj3052 I haven't had time to look into this, because of different team priorities. I'd be open to reviewing and landing a fix though.

crisbeto avatar Aug 28 '23 11:08 crisbeto

i stumbled across this problem too, i thought i go crazy until i saw this issue! here is my example: https://stackblitz.com/edit/hvp9cm

i cant move the element to the top left corner. (somehow its depending on where i click on the element).

IMO, this is a huge issue and should get a higher priority.

PentoreXannaci avatar Aug 29 '23 14:08 PentoreXannaci

This is still blocking us from updating as well. We are running angular 15 with cdk 13 for now.

leanix-micwel avatar Aug 29 '23 14:08 leanix-micwel

I've tried to implement an initial fix in PR #27730. All tests seem to be passing.

@crisbeto Let me know what needs to be done in order to get this further. I am not sure about how you guys normally use the dev-app, but I added two more cases in it (one using just the boundary and one combining the boundary with a constraint), to make sure it works in all cases.

I can easily remove this if need be.

mj3052 avatar Aug 30 '23 13:08 mj3052

@mj3052 Now working perfectly fine (tested in 16.2.12). Thanks so much for fixing this! 🥇

marvinrohde-leanix avatar Dec 04 '23 13:12 marvinrohde-leanix

Closing since this should be fixed now.

crisbeto avatar Dec 12 '23 12:12 crisbeto