almende / vis

⚠️ This project is not maintained anymore! Please go to https://github.com/visjs
7.85k stars 1.48k forks source link

Duplicate timeline events are fired for single user actions #577

Open brichbe opened 9 years ago

brichbe commented 9 years ago

Duplicate back-to-back onAdd/onUpdate/onRemove function callbacks are occurring when creating/editing/deleting timeline items.

Currently using version 3.9.0 -- it seems this issue was introduced recently.

Reproduce issue by using the 'Edit items' example at: http://visjs.org/examples/timeline/08_edit_items.html

Double-click empty space, or double-click an existing item, or select and choose to delete an existing item. In all these cases, the subsequent prompt dialogs are unexpectedly repeated.

brichbe commented 9 years ago

Just noticed this problem does not occur using Firefox v35, but does when using Chrome v39.0.2171.99.

xrysanthos commented 9 years ago

Guys, something broke in the latest release - v3.9.0. Before the 16th of January this worked ok. @brichbe The buggy behavior is reproduced in Firefox too.

losttheplot commented 9 years ago

I've just updated to 3.9.0 and am seeing the same snag: double callbacks. For example, onAdd creates two new items: d30290ac-c001-2fb7-ae0-4c2ea2765df3 and 2b5fe231-57a8-db9-9a8a-d81d1913175. However, if I introduce an alert to pause the action, I then only see one callback e.g.

onAdd: function (item, callback) {
    alert ("PAUSE"); 
    $("#shi_type_id > option").each(function() { this.text = this.title; });
    $.ajax({
        type: "POST",
        url: "ajax/ajax_shift.php",
        data: { ac: "getRates", shi_ref: item.id, shi_user_id: item.group, shi_start: item.start.toUTCString(), shi_end: item.end.toUTCString() },
        success: function (data) {
             [snip]

I'm no JS expert but it might be relevant to those who are.

josdejong commented 9 years ago

Thanks for the feedback. That's odd. So far I can't reproduce this, tried on both FF 35 and Chrome 39 running Linux Mint. @AlexDM0 can you reproduce this issue on Windows?

AlexDM0 commented 9 years ago

Hi all,

Yes, http://jsbin.com/gopuho/5/edit?html,output If I click one of the items, I get two pop-ups.

Regards

brichbe commented 9 years ago

FYI - it appears this issue has gotten "better" in v3.10 (maybe inadvertently?), but it seems some trace of this bug still exists.

I was very near to closing this issue because I re-tested and saw I could no longer reproduce it using the latest Firefox or Chrome. However, then I tried using IE11 and unfortunately saw the issue happen again, although it behaves differently now. When I first created this issue I saw the duplicate prompts happening consistently when using Chrome. Now when using IE11 I only see them when deleting items (not adding/editing/moving), and it no longer happens consistently -- the majority of the time it works fine then sporadically gives the double prompts. It happens "randomly" because I cannot reproduce the issue reliably, but maybe others will have different results.

brichbe commented 9 years ago

I reproduced this bug while running the IE profiler, so hopefully it'll help you investigate the underlying cause.

In this image, you see the standard expected behavior. The call sequence begins from the onTouchHandler which eventually invokes onRemove within the "Edit Items" example, which is where the single confirm prompt is invoked.

normaldelete

In this image, you see the buggy behavior. The call sequence begins the same from onTouchHandler and traverses down to the confirm prompt in the same sequence as before. But as you see, immediately after the first confirm prompt the entire sequence is repeated again beginning from onTouchHandler and traverses down again to repeat the confirm prompt a second time. In comparing the two images, you can also see the differences in the number of times the functions were called (e.g. triggerGesture is called 24 times and then tapGesture called 3 times in the "buggy" case, but other times they are called 16/2 times, respectively).

doublepromptdelete

josdejong commented 9 years ago

Is this still an issue? I've tried to reproduce without luck on Win 8.1: Chrome 41, FF 36, IE 11 and on Linux Mint: Chrome 41, FF 36.

The only thing that popped up in my mind in this regard is that when you use an alert or confirm, the event being executed is blocked. Browsers may not like that. I've made some minor changes in the code such that everything the code does regarding the event is done before calling the callbacks like onAdd and onRemove.

If the problem still persists, there is one other solution which will certainly help: call the callbacks on the next tick (setTimeout(function () {...}, 0), such that the current event is neatly finished before a blocking alert or confirm can be opened.

brichbe commented 9 years ago

I haven't tried your latest change in the develop branch, but I was able to reproduce this issue again using 3.11.0. It is still very sporadic and cannot be reproduced consistently -- it seems to happen at random. I opened the visjs.org edit_items example in IE11 and clicked around adding/editing/deleting items for a couple minutes before I received a double prompt on deleting an item.

Stopping event propagation before callback functions seems logical and will hopefully fix this entirely, but it'll be a couple days til I can try testing that.

Do those images I previously posted of the call stacks give you any indication of the root cause? When I glanced over those areas of the vis code a few weeks ago, I had a feeling this double prompt was potentially caused by extra events originating from hammer, but I don't know enough about hammer and your event processing algorithms to prove that. The fact that, in the buggy case, onTouchHandler was executed immediately after the first confirm prompt is what lead me to that theory. Since the confirm obviously doesn't generate a select/touch event, that call sequence implies whatever code generated the first event that invoked onTouchHandler, that same code must have generated a second "duplicate" event. So if hammer or something else is causing duplicate events to occasionally be fired, I'm not sure if preemptively stopping the propagation of the first event will affect the propagation of the "duplicate" second event.

josdejong commented 9 years ago

The call stacks help, but it remains difficult as long as I can't reproduce the issue myself...

Would be great if you can find time to try the latest version of vis.js in the develop branch.

brichbe commented 9 years ago

I just pulled the latest changes from the develop branch and could not reproduce this issue, so I'm considering it resolved. Thanks for fixing this!

josdejong commented 9 years ago

Awesome, thanks for the feedback.

brichbe commented 9 years ago

FYI - I'm seeing this issue again with the 4.0 release. Using Windows 7 and the latest version of Chrome, the example http://visjs.org/examples/timeline/editing/editingItemsCallbacks.html produces double callbacks for adding/editing/deleting. This problem does not occur for me in the latest Firefox or IE.

AlexDM0 commented 9 years ago

I can reproduce it as well. We'll look into this.

Thanks for the update!

edit: my bad, wrong button

josdejong commented 9 years ago

This has to do with alert/prompt/confirm being blocking during the handling of a browser event, which is tricky (and not constently handled by different browsers and OS'es).

I've updated the example using sweetalert, which is not blocking and hence works fine.

chrislong commented 8 years ago

I think I'm getting this bug in 4.16.1, with Chromium on Linux. When I click on the timeline, I get two select events instead of one. I looked into it a little and the second comes from a function called by a timer. It's not causing a problem for our app, but just thought I'd note that it's still there.

mojoaxel commented 7 years ago

@chrislong @brichbe Is this still an issue with the current version?

brichbe commented 7 years ago

I'm fine with closing this issue until the problem can be reproduced again with a working example.

cantsdmr commented 7 years ago

I'm now getting this error and I found the issue. I think a "click" event (user action) is interpreted both "tap" and "press" by hammer.js handlers. These handlers confuse vis event handlers so that vis calls _onSelectItem and _onMultiSelectItem handlers respectively. For example, when I try to click and hold an item in timeline, vis understands this event is "press" correctly. However basic click event is not working as it should be.

arthurkirkosa commented 7 years ago

@cantsdmr did you find a solution for your problem?

arthurkirkosa commented 7 years ago

@mojoaxel yes it is

cantsdmr commented 7 years ago

@arthurkirkosa not an elegant solution but it works. I distinguished the events using event names so I 'm getting rid of redundancy.


function selectCallback(properties){
  if (properties.event.type == "tap"){
      //here is actual logic.
  }
}
arthurkirkosa commented 7 years ago

@cantsdmr I went another way... this.timeline.itemSet.hammer.off('press'); since my app is used on desktops most of the time...

cantsdmr commented 7 years ago

@arthurkirkosa you're lucky :) but for those unlucky devs there must be a way without ugly workarounds.

AmebaBrain commented 7 years ago

in my case event duplication was caused by repeated call to timeline.on('select', onSelect); method. one happened in angular2 component ngOnInit, another one in ngOnChanges lifecycle hooks.

I checked Timeline object and found that in _callbacks.select section there are two onSelect callbacks were registered...!

After preventing repeated call to callback mapping procedure timeline.on duplicate events emition has been stopped

kadamska commented 6 years ago

I implemented @cantsdmr 's solution first - it worked for my custom code, but the item was getting deselected on the timeline - the yellow border and the "x" to delete was disappearing. @arthurkirkosa 's solution worked for me.

jgorene commented 6 years ago

I encounter the same problem (chrome, firefox.....). By a strange behavior indeed, the problem disappears completely using SweeAlert as @josdejong suggests (thank you by the way, even this discussion has been there for a while;). Finally, it even works like a charm !