Doubleclick - Drag bug [Fixes #333]

[mdtusz]

Fixes #333

In Brief

• shorten DBLCLICKDELAY from 600 to 300
• remove pauseForDrag method and calls - this prevented the next mouse actions from completing until after a DBLCLICKDELAY delay
• removed unused artifact code from pre-open-source days

Concerns

I can't see any areas that rely on the behaviour of the pauseForDrag call, and nothing appears to be broken by the changes, but it is possible there is some user-interaction that may not behave properly if this delay is removed.

@alexcjohnson @etpinard

[alexcjohnson]

Interesting... I'm not entirely sure what condition pauseForDrag was supposed to catch but I do recall it being another edge case. I suppose it's hard to figure out from git because it's from long before reorganizing the repo, huh?

So that we don't recreate whatever problem it was though, what about a different solution: whenever finishDrag gets called, call clearTimeout on the one in pauseForDrag. That's probably the issue, right? That finishDrag gets called once for the doubleclick and then again when the timeout triggers, even though at that point your mouse is down so a finishDrag erroneously resets the flags telling us what's happening? Alternatively we could abort finishDrag if it looks like the mouse is down, but in that case I'd worry about what happens when you mouse out of the window while dragging.

[mdtusz]

Hmm. I was hoping that either you or Etienne would be familiar with pauseForDrag.

The core issue is that gd._dragging is set to true, and isn't set back to false (by finishDrag) soon enough so that this block is never passed.

Regardless, I'm unsure that the code has another purpose now - as far as I can reason about, it is simply a delay of DBLCLICKDELAY where no other "actions" can be made, but this seems to be arbitrary.

If we can decipher why it should be left in, I'm happy to change it, but I would prefer to have a small interaction bug crop up in the future so that we can write a tested and robust solution, rather than to leave in code that might be important, but nobody quite knows for sure. Additionally, adding a clearTimeout would be fairly messy as there is no shared scope (other than window) for pauseForDrag and finishDrag. It could be set as a gd property, but it is already a fairly cluttered object as it is.

[alexcjohnson]

Ok, makes sense. I found where this went in, it was when I made the plotlyjs showcase for streambed - https://github.com/plotly/streambed/pull/1176

Unfortunately I don't see that I discussed why at that point, but at the very least we should be able to run the examples in there against this branch and see if something fails. Remember to try all 3 different kinds of Cartesian drag/zoom regions (plot, single axis, and corner) as well as plots with enough data that you need more than DBLCLICKDELAY to redraw - that's what the comment seems to say, that if you don't do this, then in some cases you get a redraw (and its associated events, which may be what I cared most about for the showcase, but the redraw itself could be problematic too if it flashes or slows stuff too much) on the first click of what will ultimately be a double click.

[etpinard]

FYI @mdtusz ⏫

[alexcjohnson]

Maybe I'm getting old and slow, but since I merged this into my WIP I find DBLCLICKDELAY is too fast for me... I only successfully generate a doubleclick about 2/3 of the time. Was the drop from 600 to 300 important to this fix, or did it just reduce the frequency of the bug before you implemented the real fix? Can we bump it back up? Really annoying that JS doesn't have access to the OS-defined doubleclick timing but I guess there's nothing we can do about that.

[mdtusz]

We can use the OS double click delay, but as far as I know, only through doubleclick events - I've never found a ms value as an accessible property though. The change was made initially in an effort to fix the bug, but then left because 600ms seemed like it was too long - mobile browsers used to use 300ms as the delay time to look for double taps, and Windows default timing is set to 500ms (I have no idea what the OSX default is - I assume the same though) - I wanted to err on the side of speediness.

That said, I have no issue changing it back!

[alexcjohnson]

Ok, I'd vote for 500 then, Windows probably sets a precedent that will work for most people.