Events can sometimes be dropped from the event queue

[rachel-fenichel]

Problem statement

• Bug report
• Feature request

Event filtering can drop events.

Change e1e9427 updated event filtering to not be O(n^2). Instead of splicing out events from the queue if they are duplicates, this change adds events to a new queue if necessary.

In the old event filtering code, failing every check for duplicates meant that the event was left in the queue. In the new code, an event is only put into the new queue if it is the first event that matches its hash. The new code looks like this:

  // Merge duplicates.
  for (var i = 0, event; event = queue[i]; i++) {
    if (!event.isNull()) {
      var key = [event.type, event.blockId, event.workspaceId].join(' ');
      var lastEvent = hash[key];
      if (!lastEvent) {
        hash[key] = event;
        mergedQueue.push(event);
      } else if (event.type == Blockly.Events.MOVE) {
        // Merge move events.
        lastEvent.newParentId = event.newParentId;
        lastEvent.newInputName = event.newInputName;
        lastEvent.newCoordinate = event.newCoordinate;
      } else if (event.type == Blockly.Events.CHANGE &&
          event.element == lastEvent.element &&
          event.name == lastEvent.name) {
        // Merge change events.
        lastEvent.newValue = event.newValue;
      } else if (event.type == Blockly.Events.UI &&
          event.element == 'click' &&
          (lastEvent.element == 'commentOpen' ||
           lastEvent.element == 'mutatorOpen' ||
           lastEvent.element == 'warningOpen')) {
        // Merge click events.
        lastEvent.newValue = event.newValue;
      }
    }
  }
  queue = mergedQueue;

and this is the only case where an event will be added to the new queue:

      var key = [event.type, event.blockId, event.workspaceId].join(' ');
      var lastEvent = hash[key];
      if (!lastEvent) {
        hash[key] = event;
        mergedQueue.push(event);
      } 

At a minimum we should add a final case:

else {
  mergedQueue.push(event);
}

But should the event stored in hash change as well?

Expected Behavior

Events should not be dropped.

Actual Behavior

Events can be dropped, but only if the timing is right.

Steps to Reproduce

I changed Events.fire to have a long delay before firing the event, in order to force multiple events to be on the queue at the same time for a simple case:

Blockly.Events.fire = function(event) {
  if (!Blockly.Events.isEnabled()) {
    return;
  }
  if (!Blockly.Events.FIRE_QUEUE_.length) {
    // First event added; schedule a firing of the event queue.
    setTimeout(Blockly.Events.fireNow_, 1000);
  }
  Blockly.Events.FIRE_QUEUE_.push(event);
};

Then

• Add a new block to the workspace
• Click on the workspace background, to deselect the block
• Enable events in the playground
• Click on the block

Sometimes only one event will show up (click) instead of the expected two (click and select). Increasing the timeout increases the probability that it will fail.

Stack Traces

N/A

Operating System and Browser

All

Additional Information

We found this because scratch-blocks fires a click event immediately followed by a stackclick event, and the second event was silently disappearing.

We didn't notice this before because in any case where we were watching the event stream, it was able to clear out the first event before the second event was added.

[rachel-fenichel]

@ewpatton @NeilFraser FYI

[rachel-fenichel]

Scratch-blocks dealt with this by reverting the change for now: scratchfoundation/scratch-blocks#1161 (review)

[ewpatton]

@rachel-fenichel I think the most appropriate thing would be to update the event in the hash table in addition to adding it to the event queue. Consider two mergeable events separated in time with a third non-mergeable event of the same type between them. If the non-mergeable event doesn't get put into the hash table, then the two mergeable events will still be merged even though this non-mergeable event exists. OTOH, I'm not aware of any use cases that would cause such a sequence to be generated (which case either behavior should be correct).

[rachel-fenichel]

@ewpatton I think that's right, though I'd like to see some tests.

There are actually plenty of cases of non-mergeable events, it's just that they usually are separated by enough time for the queue to clear. For instance, change events that change different elements on the same block.