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

Reordering bugs #74

Open
TimNZ opened this issue Jul 24, 2016 · 7 comments
Open

Reordering bugs #74

TimNZ opened this issue Jul 24, 2016 · 7 comments

Comments

@TimNZ
Copy link

TimNZ commented Jul 24, 2016

Awesome library, thanks.

I find very quickly in the online demo that reordering tabs is buggy.
Try your demo, reorder a few tabs, select them, reorder again, you should see the problem quickly, resize window etc.

Also for some reason it was becoming very slow at times like there is a tight code loop.

I can trigger this quite easily but can't tell you specific sequence to replicate it.

@jbilcke
Copy link

jbilcke commented Aug 15, 2016

I can confirm that after a while the demo freezes, unfreezes, freezes again.. when dragging a tab around, which is strange (is that a new bug? nobody else sees that?)

@georgeOsdDev georgeOsdDev added this to the 0.9.0 milestone Aug 16, 2016
@warent
Copy link

warent commented Aug 17, 2016

I'm starting to understand where this bug is coming from. It's actually two bugs

I believe the freezing/unfreezing is coming from having to render the Big Content tab repeatedly. Something is happening where eventually mouse events such as "click" or "drag" is forcing the Big Content to render again.

Second bug: there's an issue where sometimes you try to drag a tab, it appears away from the cursor and you're dragging it with an offset far away from the actual cursor position.

To reproduce this:

  1. Decide on one of the middle tabs to test
  2. Aim your cursor to a position slightly to the right-center of the tab
  3. Drag it to the left, but don't swap positions with the tab to the left
  4. Drop
  5. Repeat 2-4 and notice that the tab offset grows further away from the cursor

You can get the same effect going in the right direction with a left-center offset. In fact, you can even correct the bad offset by "balancing" it out like this. My theory is something bad is happening with the handleDragStop event

@warent
Copy link

warent commented Aug 17, 2016

I think we may see a clue in the handleDragStart event. If you add a third parameter, you'll get a react-draggable DraggableData object with:

type DraggableData = {
  node: HTMLElement,
  // lastX + deltaX === x
  x: number, y: number,
  deltaX: number, deltaY: number,
  lastX: number, lastY: number
};

i.e.

handleDragStart(key, e, data) {
    console.log(data);
    this.dragging = true;
}

It seems DraggableData.x == 0 on healthy tabs. Tabs that are dragging with a buggy offset have a large DraggableData.x value

@warent
Copy link

warent commented Aug 17, 2016

It looks like there's a series of things happening here...

https://github.com/mzabriskie/react-draggable/blob/85420898fcac7bdaf75587bae550293cdcb58f99/lib/DraggableCore.es6#L256

  handleDrag: EventHandler<MouseEvent> = (e) => {

    // Get the current drag point from the event. This is used as the offset.
    const position = getControlPosition(e, this.state.touchIdentifier, this);
    if (position == null) return;
    let {x, y} = position;

This leads us to
https://github.com/mzabriskie/react-draggable/blob/85420898fcac7bdaf75587bae550293cdcb58f99/lib/utils/positionFns.es6#L68

// Get {x, y} positions from event.
export function getControlPosition(e: MouseEvent, touchIdentifier: ?number, draggableCore: DraggableCore): ?ControlPosition {
  const touchObj = typeof touchIdentifier === 'number' ? getTouch(e, touchIdentifier) : null;
  if (typeof touchIdentifier === 'number' && !touchObj) return null; // not the right touch
  const node = ReactDOM.findDOMNode(draggableCore);
  // User can provide an offsetParent if desired.
  const offsetParent = draggableCore.props.offsetParent || node.offsetParent || node.ownerDocument.body;
  return offsetXYFromParent(touchObj || e, offsetParent);
}

And finally
https://github.com/mzabriskie/react-draggable/blob/85420898fcac7bdaf75587bae550293cdcb58f99/lib/utils/domFns.es6#L98

export function offsetXYFromParent(evt: {clientX: number, clientY: number}, offsetParent: HTMLElement): ControlPosition {
  const isBody = offsetParent === offsetParent.ownerDocument.body;
  const offsetParentRect = isBody ? {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};
}

I think our only options are one of the following:

  • Have CustomDraggable extend DraggableCore and implement the functions manually
  • Clone react-draggable and implement middleware to intercept the offsets
  • Use a different library than react-draggable

Thoughts?

@georgeOsdDev
Copy link
Owner

@warent

Thanks your investigation!
Bug1: for big content issue is due to reflow. I think it will be resolve with css.
#68

Bug2:
I could reproduce what you mean.
https://gyazo.com/e2753b8be4cfcb8b8d036f680ae3b864

Can we override with this file?
https://github.com/georgeOsdDev/react-draggable-tab/blob/master/src/components/CustomDraggable.js
I'm welcome PR!

@warent
Copy link

warent commented Aug 26, 2016

It's on my list. I'll be working on this eventually

@miaLashell
Copy link

miaLashell commented Aug 16, 2017

Was the inclusion of CustomDraggable supposed to fix the offset problem? I still have an offset problem after about 5 tab position changes.
@warent @georgeOsdDev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants