should Event be delivered to a listener that is added to a node further up the Trail?

[pixelzoom]

I encountered this while implementing ComboBox. A 'down' event on the combo box's button pops up the list and adds a listener to the scene. The scene listener is intended to process subsequent 'down' events anywhere on the scene, to dismiss the list. What I didn't count on is that the button 'down' event is delivered to the scene listener, making the list pop down immediately.

I worked around it in BLL by adding a flag that causes the scene listener to fire on the second 'down' event that it receives. Not pretty, and it will fail if this behavior changes.

JO demonstrated that this is consistent with DOM event handling. Is it how we want Scenery to behave?

[pixelzoom]

ComboBox was moved to sun. If this behavior changes, we'll need to remove the flag (enableClickToDismissListener) or ComboBox will be broken.

[jonathanolson]

Adding to developer meeting to discuss whether Scenery should take a snapshot of listeners before firing the event (so that freshly added listeners wouldn't be called).

Arguments for this:

• It's how we handle most other listeners (like with Property/Emitter), and would simplify the referenced code.

Implementation:

• I'd create an object pool for listener-node pairs, and push pairs to an array that serializes the call order (minimizes objects). Looks like it can be done (without re-entrance of dispatching events, seems safe) without creating new objects.

[pixelzoom]

+1 for notifying only listeners that are registered at the time that the event is fired.

-1 for notifying listeners that are added after the event is fired (and for consistency, you would presumably need to not notify listeners that were removed after the event fired).

[jonathanolson]

I'm currently leaning towards this change (@pixelzoom's +1), as long as testing reveals it doesn't have an undue performance burden.

[samreid]

Agreed with @pixelzoom's comment and I cannot recall any cases where I am purposefully relying on notifying listeners added after the event starts (though would not be surprised to see a small handful pop up).

[jonathanolson]

That's enough consensus for me, removing meeting tag.

[pixelzoom]

While working on phetsims/sun#366, I noticed that this issue is still hanging out in ComboBox:

    //TODO https://github.com/phetsims/scenery/issues/58
    /**
     * @private
     * Because clickToDismissListener is added to the scene, it receives the 'down' event that
     * buttonNode received to register the listener. This is because scenery propagates events
     * up the event trail, and the scene is further up the trail than the button.  This flag
     * is used to ignore the first 'down' event, which is the one that the button received.
     * If we don't do this, then we never see the list because it is immediately popped down.
     * This behavior may change, and is being discussed in scenery#58.
     */
    this.enableClickToDismissListener = false;

5+ years seems a little long to resolve this. On the other hand, it's working and not bothering anyone. I'll just mention @ariel-phet here, in case he wants to visit this issue.

@samreid edit: handing => hanging

[pixelzoom]

... and it looks like we had a consensus on how to proceed in #58 (comment) (March 2016).

[jonathanolson]

This would be nice to handle when I'm more back on input listeners.

[zepumph]

Tagging this issue. As part of work with ComboBox in phetsims/sun#314, I touched code that pointed to this issue, specifically the use of this.enableClickToDismissListener.

[pixelzoom]

Over in phetsims/sun#446, @zepumph and I converted ComboBoxButton to a RectangularPushButton. That seemed to eliminate the need for this.enableClickToDismissListener, and in fact it didn't work - never dismissed the list when clicking outside it. So it was removed. See phetsims/sun@deaae9d.

It might be good to have @jonathanolson take a peek, since I don't fully understand why this.enableClickToDismissListener was added in the first place, and I don't understand why changing to a RectangularPushButton made it break.

[jonathanolson]

I'm not quite sure I see the entire history, but it appears to have been vestigial. How much more time is this worth, given that things work now?

[pixelzoom]

I guess I'll just go ahead and close this, because (as I said above) I don't understand what was done here. And I can't recall what the original problem was from 5+ years ago.