Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dragging elements on left border causes deltaX to jump between negative and positive values #170

Closed
aeneasr opened this issue Jul 4, 2016 · 10 comments

Comments

@aeneasr
Copy link
Contributor

aeneasr commented Jul 4, 2016

This issue is a copy of react-grid-layout/react-resizable#38 because it looks like the root cause is something within this library.

I have a resizable div which is float: right. The handler is on the left side. Everything works fine when the handler is on the right side but I'm having some troubles with the one on the right.

The following gif shows two screens. One, where the resizable is floating left and one where it's floating right. The floating left example is included to show you that it generally works.

jumpingstuff

After some debugging and seeing that it works fine if I scale the inner element only, I believe that the issue is caused because the origin of the element wrapping resizeable (which is top left I guess?) changes:

jumpingstuff3

In pseudocode, the react structure of the borken example is:

<div width="123px" float="right">
  <ReactResizable>
      <children />
  </ReactResizable>
</div>

The react structure of the example without jumping is:

<div float="right">
  <ReactResizable>
    <div width="123px">
      <children />
    </div>
  </ReactResizable>
</div>

In the first case, <ReactResizable> changes the position of the top left corner, which might have something to do with the jumping. In the second, the position of <ReactResizable> stays the same while the right side of the <div> is being changed.

I believe that the root cause for this issue is this library (react-draggable), because:

deltaX in resizeHandler is jumping around between negative and positive values when the issue occurs:

deltajumper

This does not happen when dragging an object on the right side, which is working fine:

delta2

resizeHandler is a DraggableEventHandler, so the wrong values are originating somewhere here.

@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 14, 2016

I could really use some help here, this library is a larger one and it is really hard to find the cause by oneself without knowing about the internals. It is very unfortunate to see that despite my attempts to give you as much insight as possible, this issue is simply being ignored although activity on other issues exists by major contributors like @STRML . Maybe I'll save the troubles next time with recording gifs and debugging and just write: "you lib is broken kkthxbye". Open sourcing things is also about supporting the community who uses it. Simply ignoring (maybe more difficult) issues isn't cool.

Having said that, I would love to fix this with a PR but I need guidance to look in the right places.

@STRML
Copy link
Collaborator

STRML commented Jul 14, 2016

@arekkas We all have day jobs and do this in our spare time. I appreciate the work you've put into recording this but I just haven't had the time to review.

This is free code and nothing is owed to you.

Open sourcing things is also about supporting the community who uses it.

No, it is not. It is about sharing work you have done. There is no obligation to provide free support for free software. Would you rather we simply kept everything in-house and never shared?

@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 14, 2016

I understand that completely, dropping off a message: "Sorry, i'm really busy and don't have the time to look into it right now. Hopefully I can help next week." or delegating it to a community member who might know where to look would absolutely be sufficient.

No, it is not. It is about sharing work you have done. There is no obligation to provide free support for free software. Would you rather we simply kept everything in-house and never shared?

It's not about giving free support, it's about being responsive. You have no obligation to fix other people's problems, but part of open source is the community aspect in it.

Anyways, thank you for your library and reply. I would be very happy if you find some time to think about this issue and give me a one-liner explaining where I could start looking.

@STRML
Copy link
Collaborator

STRML commented Jul 14, 2016

Create a jsfiddle replicating the issue and I should be able to help.

@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 14, 2016

Thank you! I pulled myself together and dug through the code. The issue, as suspected, is caused by the parent element whose width and position are changing. I was able to fix this issue by forcing to always use document.body and 0,0 as the origin in domFns.es6:

      const offsetParent = document.body;
      const offsetParentRect = { left: 0, top: 0 };

Do you think that this will have negative side effects? If not, I would happily create a PR. If yes, I will try to recreate this issue on esfiddle.

@STRML
Copy link
Collaborator

STRML commented Jul 14, 2016

It will cause issues; it uses the nearest offset parent so that nested Draggables within scrollable boxes (see the Demo) can work, even when scrolling. This makes it so we don't have to attach a scroll handler. See a398097

@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 14, 2016

Ah I see, thank you. I will try to reproduce this in a jsfiddle now. Appreciate your help :)

@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 14, 2016

I tried a like an hour but wasn't able to include react-draggable in jsfiddle. Do you know an easy solution for it? :) React / Babel work fine.

@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 14, 2016

In the original proposal, scrollableAncestor could be defined. Would you accept a PR that implements such a functionality? I sort of belive that choosing a different parent is the only way to solve this. I would allow to pass a css selector or DOM element, something like:

<DraggableCore originNode={document.body} />

or:

<DraggableCore originNodeSelector="body.someclass:first" />

and:

export function offsetXYFromParentOf(evt: {clientX: number, clientY: number}, node: HTMLElement & {offsetParent: HTMLElement}, origin?: HTMLElement): ControlPosition {
  const offsetParent = origin || node.offsetParent || document.body;
  const offsetParentRect = offsetParent === document.body ? {left: 0, top: 0} : offsetParent.getBoundingClientRect();

  const x = evt.clientX + offsetParent.scrollLeft - offsetParentRect.left;
  const y = evt.clientY + offsetParent.scrollTop - offsetParentRect.top;

  return {x, y};
}

Alternatively we could add a simple bool - as there are two cases anyways: use body (changing parent position) or use parent (scrollable div):

<DraggableCore bodyIsOrigin />

Again, thank you for taking the time on this. I think I am really close to resolving it and would love to contribute to this project.

aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 17, 2016
…bleCore>

Allows nodes to use the body as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 17, 2016
…bleCore>

Allows nodes to use the body as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 17, 2016
…bleCore>

Allows nodes to use the body as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
@aeneasr
Copy link
Contributor Author

aeneasr commented Jul 17, 2016

I was able to resolve my issues with #173

aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 19, 2016
…bleCore>

Allows nodes to use the body as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 19, 2016
…Core>

Allows nodes to use an arbitrary node as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 19, 2016
…Core>

Allows nodes to use an arbitrary node as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 19, 2016
…Core>

Allows nodes to use an arbitrary node as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 19, 2016
…Core>

Allows nodes to use an arbitrary node as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 19, 2016
…Core>

Allows nodes to use an arbitrary node as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 19, 2016
…Core>

Allows nodes to use an arbitrary node as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
aeneasr pushed a commit to aeneasr/react-draggable that referenced this issue Jul 19, 2016
…Core>

Allows nodes to use an arbitrary node as origin instead of the parent. This is useful, when the parent's position is changing. When used, resolves react-grid-layout#170

This issue was introduced by a398097
@STRML STRML closed this as completed in f63f3d1 Jul 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants