ComboBoxListBox: use a standard scenery listener

[pixelzoom]

Related to #445, in CombBoxListBox.

Sometime in the future, investigate replacing fireEmitter and selectionListener with a standard scenery listener (PressListener, FireListener,...)

[zepumph]

This should be fixed generally by https://github.com/phetsims/sun/issues/463

[zepumph]

I looked into doing this and felt like I had got close, but found it problematic because FireListener attaches to the pointer, and such doesn't have event.currentTarget. Master is currently adding the listener to the Node, so we do get event.currentTarget.

Here is the patch I was thinking of

```diff Index: js/ComboBoxListBox.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/ComboBoxListBox.js (revision 0de96f862051f40be4cccb687f7a4bdc8ae42b45) +++ js/ComboBoxListBox.js (date 1571103073014) @@ -9,10 +9,8 @@ 'use strict'; // modules - const Action = require( 'AXON/Action' ); const ComboBoxListItemNode = require( 'SUN/ComboBoxListItemNode' ); - const Event = require( 'SCENERY/input/Event' ); - const EventType = require( 'TANDEM/EventType' ); + const FireListener = require( 'SCENERY/listeners/FireListener' ); const KeyboardUtil = require( 'SCENERY/accessibility/KeyboardUtil' ); const Panel = require( 'SUN/Panel' ); const sun = require( 'SUN/sun' ); @@ -56,52 +54,27 @@ assert && assert( options.xMargin > 0 && options.yMargin > 0, 'margins must be > 0, xMargin=' + options.xMargin + ', yMargin=' + options.yMargin ); - //TODO sun#462 replace fireEmitter and selectionListener with a standard scenery listener - // Pops down the list box and sets the property.value to match the chosen item. - const fireAction = new Action( event => { + const fireListener = new FireListener( { + fire: event => { - const listItemNode = event.currentTarget; - assert && assert( listItemNode instanceof ComboBoxListItemNode, 'expected a ComboBoxListItemNode' ); + const listItemNode = event.currentTarget; + assert && assert( listItemNode instanceof ComboBoxListItemNode, 'expected a ComboBoxListItemNode' ); - // hide the list - hideListBoxCallback(); + // hide the list + hideListBoxCallback(); - // prevent nodes (eg, controls) behind the list from receiving the event - event.abort(); + // prevent nodes (eg, controls) behind the list from receiving the event + event.abort(); - // set value based on which item was chosen in the list box - property.value = listItemNode.item.value; - }, { - parameters: [ { phetioPrivate: true, valueType: Event } ], - - // phet-io - tandem: tandem.createTandem( 'fireAction' ), - phetioEventType: EventType.USER - } ); - - //TODO sun#462 replace fireEmitter and selectionListener with a standard scenery listener - // Handles selection from the list box. - const selectionListener = { - - up( event ) { - fireAction.execute( event ); - }, + // set value based on which item was chosen in the list box + property.value = listItemNode.item.value; - // Handle keyup on each item in the list box, for a11y. - //TODO sun#447, scenery#931 we're using keyup because keydown fires continuously - keyup: event => { - if ( KeyboardUtil.KEY_ENTER === event.domEvent.keyCode || KeyboardUtil.KEY_SPACE === event.domEvent.keyCode ) { - fireAction.execute( event ); + if ( event.isA11y() ) { focusButtonCallback(); } }, - - // handle activation from an assistive device that may not use a keyboard (such as mobile VoiceOver) - click: event => { - fireAction.execute( event ); - focusButtonCallback(); - } - }; + tandem: tandem.createTandem( 'fireListener' ) + } ); // Compute max item dimensions const maxItemWidth = _.maxBy( items, item => item.node.width ).node.width; @@ -111,8 +84,9 @@ const highlightWidth = maxItemWidth + options.xMargin; const highlightHeight = maxItemHeight + options.yMargin; + const listItemNodes = []; // {ComboBoxListItemNode[]} + // Create a node for each item in the list, and attach a listener. - const listItemNodes = []; // {ComboBoxListItemNode[]} items.forEach( ( item, index ) => { // Create the list item node @@ -129,7 +103,7 @@ } ); listItemNodes.push( listItemNode ); - listItemNode.addInputListener( selectionListener ); + listItemNode.addInputListener( fireListener ); } ); const content = new VBox( { Index: js/listeners/FireListener.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/listeners/FireListener.js (revision 2a4b2a83b2b0ce578e10c5b7a19c8d4c308d45bd) +++ js/listeners/FireListener.js (date 1571100448000) @@ -12,6 +12,7 @@ 'use strict'; const Emitter = require( 'AXON/Emitter' ); + const Event = require( 'SCENERY/input/Event' ); const EventType = require( 'TANDEM/EventType' ); const inherit = require( 'PHET_CORE/inherit' ); const PressListener = require( 'SCENERY/listeners/PressListener' ); @@ -49,7 +50,8 @@ // @private {Emitter} this.firedEmitter = new Emitter( { tandem: options.tandem.createTandem( 'firedEmitter' ), - phetioEventType: EventType.USER + phetioEventType: EventType.USER, + parameters: [ { phetioPrivate: true, valueType: Event } ] } ); this.firedEmitter.addListener( options.fire ); } @@ -69,7 +71,7 @@ sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( 'FireListener fire' ); sceneryLog && sceneryLog.InputListener && sceneryLog.push(); - this.firedEmitter.emit(); + this.firedEmitter.emit( event ); sceneryLog && sceneryLog.InputListener && sceneryLog.pop(); }, @@ -118,7 +120,7 @@ PressListener.prototype.release.call( this, event, function() { // Notify after the rest of release is called in order to prevent it from triggering interrupt(). if ( !self._fireOnDown && self.isHoveringProperty.value && !self.interrupted ) { - self.fire(); + self.fire( event ); } callback && callback(); } ); ```

@jonathanolson is there a way to get currentTarget from a PressListener or subtype?

[jessegreenberg]

I encountered this in https://github.com/phetsims/sun/issues/556, using a Scenery listener that attaches itself to the Pointer will resolve that issue. That is important so that the Pointer doesn't snag other things if it moves off of the ComboBox.

[jessegreenberg]

I took a look and I also don't see a way to get the currentTarget (partly because we want FireListener to fire on up). But if we cannot have a currentTarget, we could have one FireListener per ComboBoxListItemNode, like this

```diff diff --git a/js/ComboBoxListBox.js b/js/ComboBoxListBox.js index acd18d41..f5970a1c 100644 --- a/js/ComboBoxListBox.js +++ b/js/ComboBoxListBox.js @@ -9,13 +9,11 @@ define( require => { 'use strict'; // modules - const Action = require( 'AXON/Action' ); const ComboBoxListItemNode = require( 'SUN/ComboBoxListItemNode' ); - const EventType = require( 'TANDEM/EventType' ); const KeyboardUtils = require( 'SCENERY/accessibility/KeyboardUtils' ); const merge = require( 'PHET_CORE/merge' ); const Panel = require( 'SUN/Panel' ); - const SceneryEvent = require( 'SCENERY/input/SceneryEvent' ); + const FireListener = require( 'SCENERY/listeners/FireListener' ); const sun = require( 'SUN/sun' ); const Tandem = require( 'TANDEM/Tandem' ); const VBox = require( 'SCENERY/nodes/VBox' ); @@ -57,53 +55,6 @@ define( require => { assert && assert( options.xMargin > 0 && options.yMargin > 0, 'margins must be > 0, xMargin=' + options.xMargin + ', yMargin=' + options.yMargin ); - //TODO sun#462 replace fireEmitter and selectionListener with a standard scenery listener - // Pops down the list box and sets the property.value to match the chosen item. - const fireAction = new Action( event => { - - const listItemNode = event.currentTarget; - assert && assert( listItemNode instanceof ComboBoxListItemNode, 'expected a ComboBoxListItemNode' ); - - // hide the list - hideListBoxCallback(); - - // prevent nodes (eg, controls) behind the list from receiving the event - event.abort(); - - // set value based on which item was chosen in the list box - property.value = listItemNode.item.value; - }, { - parameters: [ { phetioPrivate: true, valueType: SceneryEvent } ], - - // phet-io - tandem: tandem.createTandem( 'fireAction' ), - phetioEventType: EventType.USER - } ); - - //TODO sun#462 replace fireEmitter and selectionListener with a standard scenery listener - // Handles selection from the list box. - const selectionListener = { - - up( event ) { - fireAction.execute( event ); - }, - - // Handle keyup on each item in the list box, for a11y. - //TODO sun#447, scenery#931 we're using keyup because keydown fires continuously - keyup: event => { - if ( KeyboardUtils.KEY_ENTER === event.domEvent.keyCode || KeyboardUtils.KEY_SPACE === event.domEvent.keyCode ) { - fireAction.execute( event ); - focusButtonCallback(); - } - }, - - // handle activation from an assistive device that may not use a keyboard (such as mobile VoiceOver) - click: event => { - fireAction.execute( event ); - focusButtonCallback(); - } - }; - // Compute max item dimensions const maxItemWidth = _.maxBy( items, item => item.node.width ).node.width; const maxItemHeight = _.maxBy( items, item => item.node.height ).node.height; @@ -130,7 +81,28 @@ define( require => { } ); listItemNodes.push( listItemNode ); - listItemNode.addInputListener( selectionListener ); + //TODO sun#462 replace fireEmitter and selectionListener with a standard scenery listener + // Pops down the list box and sets the property.value to match the chosen item. + // const fireAction = new Action( event => { + const fireListener = new FireListener( { + fire: event => { + + // hide the list + hideListBoxCallback(); + + // prevent nodes (eg, controls) behind the list from receiving the event + event.abort(); + + // set value based on which item was chosen in the list box + property.value = item.value; + + if ( event.isA11y() ) { + focusButtonCallback(); + } + } + } ); + + listItemNode.addInputListener( fireListener ); } ); const content = new VBox( { ```

@zepumph what do you think?

[jessegreenberg]

@zepumph and I tried to do this today but couldn't finish because FireListener/PressListener responds to click events, but we need the ComboBoxListItemNode to fire on keyup events. Here is the patch we tried:

```diff Index: js/ComboBoxListBox.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/ComboBoxListBox.js (revision 57ca44eb9624ac1c547b3b2ecc754531824bf648) +++ js/ComboBoxListBox.js (date 1611094682271) @@ -10,6 +10,7 @@ import merge from '../../phet-core/js/merge.js'; import KeyboardUtils from '../../scenery/js/accessibility/KeyboardUtils.js'; import SceneryEvent from '../../scenery/js/input/SceneryEvent.js'; +import FireListener from '../../scenery/js/listeners/FireListener.js'; import VBox from '../../scenery/js/nodes/VBox.js'; import multiSelectionSoundPlayerFactory from '../../tambo/js/multiSelectionSoundPlayerFactory.js'; import generalCloseSoundPlayer from '../../tambo/js/shared-sound-players/generalCloseSoundPlayer.js'; @@ -61,52 +62,35 @@ assert && assert( options.xMargin > 0 && options.yMargin > 0, 'margins must be > 0, xMargin=' + options.xMargin + ', yMargin=' + options.yMargin ); - //TODO sun#462 replace fireEmitter and selectionListener with a standard scenery listener // Pops down the list box and sets the property.value to match the chosen item. - const fireAction = new Action( event => { + const fireListener = new FireListener( { + fire: event => { - const listItemNode = event.currentTarget; - assert && assert( listItemNode instanceof ComboBoxListItemNode, 'expected a ComboBoxListItemNode' ); + let listItemNode = null; + for ( let i = event.trail.nodes.length - 1; i >= 0; i-- ) { + const trailNode = event.trail.nodes[ i ]; + if ( trailNode instanceof ComboBoxListItemNode ) { + listItemNode = trailNode; + break; + } + } + assert && assert( listItemNode, 'Expected event to contain a ComboBoxListItemNode' ); - // set value based on which item was chosen in the list box - property.value = listItemNode.item.value; + // set value based on which item was chosen in the list box + property.value = listItemNode.item.value; - // hide the list - hideListBoxCallback(); + // hide the list + hideListBoxCallback(); - // prevent nodes (eg, controls) behind the list from receiving the event - event.abort(); - }, { - parameters: [ { phetioPrivate: true, valueType: SceneryEvent } ], - - // phet-io - tandem: tandem.createTandem( 'fireAction' ), - phetioEventType: EventType.USER - } ); + // prevent nodes (eg, controls) behind the list from receiving the event + event.abort(); - //TODO sun#462 replace fireEmitter and selectionListener with a standard scenery listener - // Handles selection from the list box. - const selectionListener = { - - up( event ) { - fireAction.execute( event ); - }, - - // Handle keyup on each item in the list box, for a11y. - //TODO sun#447, scenery#931 we're using keyup because keydown fires continuously - keyup: event => { - if ( KeyboardUtils.KEY_ENTER === event.domEvent.keyCode || KeyboardUtils.KEY_SPACE === event.domEvent.keyCode ) { - fireAction.execute( event ); - focusButtonCallback(); - } + focusButtonCallback(); }, - // handle activation from an assistive device that may not use a keyboard (such as mobile VoiceOver) - click: event => { - fireAction.execute( event ); - focusButtonCallback(); - } - }; + // phet-io + tandem: tandem.createTandem( 'fireListener' ) + } ); // Compute max item dimensions const maxItemWidth = _.maxBy( items, item => item.node.width ).node.width; @@ -134,7 +118,7 @@ } ); listItemNodes.push( listItemNode ); - listItemNode.addInputListener( selectionListener ); + listItemNode.addInputListener( fireListener ); } ); const content = new VBox( { ```

We would like to see PressListener optionally fire from keydown/keyup events instead of click events so this kind of thing would work.

[zepumph]

Over in https://github.com/phetsims/scenery/issues/1137, we were able to get rid of one of these TODO links (https://github.com/phetsims/sun/commit/4e6d4c7e91638ed25eecef6f7576dda13f307ed4) because you can now focus elements even when input is mouse/touch. Focus highlights will only be shown when needed for keyboard.

[jonathanolson]

is there a way to get currentTarget from a PressListener or subtype?

That seems like a SceneryEvent feature, but targetNode can also be used. Might be good to have a call if I can help!