fix(drag): always put element under the mouse when dragging an element by VincentMolinie · Pull Request #2263 · gridstack/gridstack.js

VincentMolinie · 2023-04-13T08:35:00Z

Description

The position of elements dragged was badly calculated and resulted in weird behaviour in a lot of cases like:

Disable automatic scroll, scroll page while dragging
Zoom out/in using the scale css property and try to drag a component

Checklist

Created tests which fail without the change (if possible)
All tests passing (yarn test)
Extended the README / documentation, if necessary

Issues fixed

#1275

VincentMolinie · 2023-04-14T13:16:56Z

@adumesny could you take a look 🙏

adumesny · 2023-04-14T19:25:41Z

thanks, will take a look. the title sounds scary, if all you wanted to fix was scrolling and scaling divs.

adumesny

so looks like you are handling parent div scale (what about general transform including translate ?) for dragging only, what about for sizing do we need the same ?

VincentMolinie · 2023-04-18T08:38:53Z

I'm still working on it, I'll let you know as soon as I fixed all the issues

hosnyDev · 2023-04-18T20:52:54Z

@VincentMolinie @adumesny please can u fix this issue ASAP 😊

VincentMolinie · 2023-04-19T00:54:32Z

Everything should be fixed now :)

VincentMolinie · 2023-04-19T00:54:56Z

@VincentMolinie @adumesny please can u fix this issue ASAP 😊

Yes don't worry that was my top priority 😄

VincentMolinie · 2023-04-19T00:55:28Z

@VincentMolinie @adumesny please can u fix this issue ASAP 😊

Yes don't worry that was my top priority 😄
It's just not that easy to fix ^^

hosnyDev · 2023-04-19T00:56:50Z

@VincentMolinie appreciated TYT "D

VincentMolinie · 2023-04-19T08:53:57Z

Well I don't need to take my time, as I say I did fix every issues I could find 👼

VincentMolinie · 2023-04-20T10:59:14Z

@adumesny it's back to you 😄

adumesny · 2023-05-01T02:30:30Z

I pulled your changes locally, and demo/offset.html no longer works... can't trade scaling for offset.

Also I even tried just scale and it's badly broken too ! so not sure what you tested this on...

    <div style="transform: scale(0.5, 0.5)">
      <div class="grid-stack"></div>
    </div>

at this point I'm not sure what you are trying to fix (the code change looks much more complicated than I would expect) and didn't plan on fixing scale transform - I don't understand the use case for it. Browser scaling (for accessibility and others) works just fine, so why woudl you scale a div like above ???

VincentMolinie · 2023-05-09T15:19:11Z

Good catch @adumesny , sorry about that. I fixed those issue and I added a demo for the scale so we can test it easily

VincentMolinie · 2023-05-23T12:15:40Z

if some people would like to use this fix I published a temporary version here until it's officially released 😄 : git+https://github.com/VincentMolinie/gridstack.js.git#public/scroll-and-scale

adumesny · 2023-05-27T14:34:52Z

+    if (style.position === 'relative' || style.position === 'absolute' || style.position === 'fixed') {
+      return el;
+    } else {
+      return this.getPositionContainerElement(el.parentElement);


there is no 'this' in static methods. use return Utils.getPositionContainerElement(...)
also no need to have else {} (a bit more compact, although less readable)

check for el.parentElement being undefined

For the check for el.parentElement being undefined, it's already checked by the first line, we do a !this.el so it's actually !el.parentElement

you still have this.xyz in a static method!

adumesny · 2023-05-27T17:07:00Z

your code is still completely breaking the main example demo/two.html (that and nested.html are the main 2 I always test against) at a minimum.

when you say you have fixed all issue, I'm not sure what you are using.
Again, why do we need to suport div scaling ? at this point it is more risk breaking things than is worth... Also you NEED to test mobile devices as I don't want to have to test+fix mobile touch all over as well...

start drag make object jump to (0,0)

adumesny · 2023-05-29T17:22:30Z

I would strongly suggest you use the existing code but add scaling to the mix, rather than a complete re-write which has proven to be unreliable so far... twice, it broke very basic flows...

VincentMolinie · 2023-06-01T18:37:57Z

I retested all the demo. now it should be good sorry about that 😥

DigitagDev · 2023-06-05T11:59:23Z

@adumesny I work on a project where I drag items from a toolbar and drop them onto a grid. I even tried using the scale() function on the parent div to include a zoom effect on the area of my grid. Considering that several grids in the project need to have real scale, where a grid could occupy almost the entire screen or be a 200x200px grid with very small items, a zoom tool would be almost essential in my project. I'm mentioning this to clarify why I'm also trying to use the scale zoom. Unfortunately, browser support for this type of situation is only available for the entire page, not specifically for a div.

leemoria · 2023-06-07T07:42:45Z

I also need scale，Have you some methods to resolve？@adumesny

adumesny · 2023-06-08T16:08:57Z

I'll take a look this weekend...

adumesny · 2023-06-12T14:51:06Z

ran out of time over the weekend (had bugs to fix) and I will be on PTO soon, so this will have to wait till I get back the weekend in ~2 weeks

VincentMolinie · 2023-06-15T16:03:43Z

I found some issues, so you can delay your review 😞

VincentMolinie · 2023-06-20T16:04:01Z

Okey I finally found the issue, and the reason why I had such a difference between the demo and my own project.
The Helper is positioned in fixed when dragging. When having a position fixed it's positioned according to the viewport OR the first parent that has a transform. Which explain the weird position issue I had

So everything should be good now 🤞

adumesny · 2023-08-06T22:04:53Z

      }
+      if (node) {
+        const rect = this.el.getBoundingClientRect();
+        this._originalMousePositionInsideElement = { x: s.clientX - rect.left, y: s.clientY - rect.top };


'_' is reserved for private vars, not protected. also rename to 'origRelativeMouse' and create based on this.mouseDownEvent otherwise you are off by 3+ pixels. ALSO don't really need that since we have this.mouseDownEvent (less vars to track)

I'm using s which is the downEvent.el can move so no, we need to keep that variable

adumesny · 2023-08-06T22:19:12Z

      position: {
-        left: rect.left - containmentRect.left,
-        top: rect.top - containmentRect.top
+        left: rect.left / scaleX,


so this has a very different meaning - was relative position, and now it's not (and scaled). Thsi will affect user of those values...

It was private API. Everyone knows that using private API might break even when just patching a version 🤔. I can't really see the issue here 😅

it's acutally protected (though it does have the _ which is usually reserved for private) so any subclass would break. but I have it @internal so no exported types for TS subclassing. need to clean and make private again.
but DDUIData is NOT private last time I checked and you changed the meaning...

I didn't changed the meaning. This is the real position of the element. temporalRect has been updated to be at the right position

adumesny · 2023-08-06T22:21:21Z

+    if (style.position === 'relative' || style.position === 'absolute' || style.position === 'fixed') {
+      return el;
+    } else {
+      return this.getPositionContainerElement(el.parentElement);


you still have this.xyz in a static method!

adumesny · 2023-08-06T22:41:40Z

still doens't behave the right way.

two.html the item jumps to cursor (0,0) when starting a new (helper) drag say from the middle instead of being under the mouse

instead of https://gridstackjs.com/demo/two.html

also whe scaling (with zoom IN is much more optvious) you can see some weird wiggle animation where you must be changing the top & left position even though only scale bottom right happens. Didn't look at your code but it's not right.

20230806_154047.mp4

VincentMolinie · 2023-08-29T12:54:12Z

Everything is fixed @adumesny, thanks again for your review and sorry for the delay, I was in holidays ^^

adumesny · 2023-08-29T18:38:46Z

      position: {
-        left: rect.left - containmentRect.left,
-        top: rect.top - containmentRect.top
+        left: rect.left / scaleX,


it's acutally protected (though it does have the _ which is usually reserved for private) so any subclass would break. but I have it @internal so no exported types for TS subclassing. need to clean and make private again.
but DDUIData is NOT private last time I checked and you changed the meaning...

adumesny · 2023-08-29T18:51:33Z

+    let el = element;
+
+    // Check if element is visible, otherwise the width/height will be of 0
+    while (el && !el.offsetParent) {


"element is visible" and offsetParent are the same ???? comment is off

offsetParent is null when an element is invisible 😉

I know clientHeight\Width are 0 when invisible (makes sense), but didn't know about offsetParent (less obvious). I would sue that (which matches your comment)

adumesny · 2023-08-29T19:11:07Z

-    };
-  }
-
  /** @internal TODO: set to public as called by DDDroppable! */


you've ppossibly changed the meaning of DDUIData here, yet this is used by DDDroppable according to comment, and I don't see changes in this review.... did you follow through and see how it is used and if this might be a breaking change ?

It's still the position of the helper 🤔

adumesny · 2023-08-29T19:23:31Z

+    const containerElement = Utils.getPositionContainerElement(this.el.parentElement);
+    const containerRect = containerElement.getBoundingClientRect();
+
    const newRect = { // Note: originalRect is a complex object, not a simple Rect, so copy out.


to be honest I don't know _getChange() is used anymore...

Yes it is. It is actually all the logic of the resize. It's called every time we resize

adumesny · 2023-08-29T19:25:32Z

-    const minHeight = this.option.minHeight || oHeight;
+    const { scaleX, scaleY } = Utils.getScaleForElement(this.el);
+    const o = this.option;
+    const maxWidth = o.maxWidth ? o.maxWidth * scaleX : Number.MAX_SAFE_INTEGER;


if I recall this.options.maxWidth = n.maxW * cellWidth, so they still need to be scalled ?

Yes because the scale apply to the grid so if the cell are 10px and you scale to 2 you will have actual cell of 20px

adumesny · 2023-08-29T19:27:15Z

-      this.el.style[key] = value - containmentRect[key] + 'px';
-    });
+    const { scaleX, scaleY } = Utils.getScaleForElement(this.el);
+    this.el.style.width = `${Math.floor(this.temporalRect.width / scaleX)}px`;


why floor() and not round() ? if I recall you didn't have anyhting before and claimed all was working. what changed ?

Nothing floor was working that's it. I can do round to

I replaced it by a round

VincentMolinie · 2023-09-04T09:50:07Z

Back to you @adumesny 😉

adumesny · 2023-09-05T01:00:04Z

still broken... while main demos seem fine (two.html and nested.html) offset.html doesn't work now.

20230904_175826.mp4

VincentMolinie · 2023-09-20T12:13:09Z

The refacto did introduce a bug on the getContainerForPositionFixedElement

VincentMolinie · 2023-09-26T08:40:25Z

@adumesny can you take a look at it 👼 ?

adumesny · 2023-09-26T15:26:30Z

swamped at work and can't this weekend... I'll try to find time.

adumesny · 2023-09-30T17:06:50Z

great, I tested a bunch of examples and mobile and it's working now. Will commit and go over the code again. thanks.

…n element (gridstack#2263)" re-opens gridstack#1275, and fix gridstack#2498 gridstack#2497 gridstack#2491 This reverts commit b559a6f.

gridstack#2263) * fix(drag): place the element dragged always below the mouse * fix: review fixes * fix: fix data transfer demo * fix: review fixes * fix: fix position according to first transform parent * review fixes * review fixes * review fixes * fix: fix the refacto --------- Co-authored-by: Vincent Molinié <vincent.m@forestadmin.com>

VincentMolinie force-pushed the fix/scroll-and-scale branch from 75fe294 to 2dd7bf0 Compare April 14, 2023 13:22

hosnyDev approved these changes Apr 15, 2023

View reviewed changes

adumesny reviewed Apr 15, 2023

View reviewed changes

Comment thread src/dd-draggable.ts Outdated

Comment thread src/gridstack.ts Outdated

Comment thread src/utils.ts Outdated

Comment thread src/utils.ts

adumesny changed the title ~~fix(drag): place the element dragged always below the mouse~~ fix(drag): handle div scale during dragging Apr 15, 2023

adumesny reviewed Apr 17, 2023

View reviewed changes

Comment thread src/dd-draggable.ts Outdated

VincentMolinie changed the title ~~fix(drag): handle div scale during dragging~~ fix(drag): handle element scale during dragging / resizing Apr 19, 2023

VincentMolinie changed the title ~~fix(drag): handle element scale during dragging / resizing~~ fix(drag): always put element under the mouse when dragging an element May 4, 2023

adumesny reviewed May 27, 2023

View reviewed changes

VincentMolinie force-pushed the fix/scroll-and-scale branch 2 times, most recently from 830b8d3 to c73ac4b Compare June 1, 2023 18:35

VincentMolinie force-pushed the fix/scroll-and-scale branch from c73ac4b to d6c9439 Compare June 12, 2023 10:32

VincentMolinie force-pushed the fix/scroll-and-scale branch from 61c0605 to 09d376c Compare June 21, 2023 09:07

adumesny reviewed Aug 6, 2023

View reviewed changes

adumesny reviewed Aug 29, 2023

View reviewed changes

Vincent Molinié added 8 commits September 4, 2023 11:49

fix(drag): place the element dragged always below the mouse

c7993c1

fix: review fixes

49f6f0c

fix: fix data transfer demo

ba8c4a3

fix: review fixes

b033642

fix: fix position according to first transform parent

90dd601

review fixes

58723d2

review fixes

e123898

review fixes

41a3da3

VincentMolinie force-pushed the fix/scroll-and-scale branch from 3e2f7c8 to 41a3da3 Compare September 4, 2023 09:49

fix: fix the refacto

f4fbd23

adumesny merged commit b559a6f into gridstack:master Sep 30, 2023

VincentMolinie deleted the fix/scroll-and-scale branch October 9, 2023 14:10

adumesny mentioned this pull request Oct 15, 2023

Revert "fix(drag): always put element under the mouse when dragging a… #2504

Merged

3 tasks

Uh oh!

Conversation

VincentMolinie commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Issues fixed

Uh oh!

VincentMolinie commented Apr 14, 2023

Uh oh!

adumesny commented Apr 14, 2023

Uh oh!

adumesny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VincentMolinie commented Apr 18, 2023

Uh oh!

hosnyDev commented Apr 18, 2023

Uh oh!

VincentMolinie commented Apr 19, 2023

Uh oh!

VincentMolinie commented Apr 19, 2023

Uh oh!

VincentMolinie commented Apr 19, 2023

Uh oh!

hosnyDev commented Apr 19, 2023

Uh oh!

VincentMolinie commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentMolinie commented Apr 20, 2023

Uh oh!

adumesny commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentMolinie commented May 9, 2023

Uh oh!

VincentMolinie commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

adumesny commented May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adumesny commented May 29, 2023

Uh oh!

VincentMolinie commented Jun 1, 2023

Uh oh!

DigitagDev commented Jun 5, 2023

Uh oh!

leemoria commented Jun 7, 2023

Uh oh!

adumesny commented Jun 8, 2023

Uh oh!

adumesny commented Jun 12, 2023

Uh oh!

VincentMolinie commented Jun 15, 2023

Uh oh!

VincentMolinie commented Jun 20, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

VincentMolinie commented Apr 13, 2023 •

edited

Loading

VincentMolinie commented Apr 19, 2023 •

edited

Loading

adumesny commented May 1, 2023 •

edited

Loading

VincentMolinie commented May 23, 2023 •

edited

Loading

adumesny commented May 27, 2023 •

edited

Loading

adumesny commented Aug 6, 2023 •

edited

Loading