delete redundant button listener, make listbox handle focus update, #445

phetsims · Jan 20, 2019 · 6dfdc6f · 6dfdc6f
1 parent 0e5106c
commit 6dfdc6f
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 37 deletions.
diff --git a/js/ComboBox.js b/js/ComboBox.js
@@ -16,7 +16,6 @@ define( require => {
   const ComboBoxListBox = require( 'SUN/ComboBoxListBox' );
   const inherit = require( 'PHET_CORE/inherit' );
   const InstanceRegistry = require( 'PHET_CORE/documentation/InstanceRegistry' );
-  const KeyboardUtil = require( 'SCENERY/accessibility/KeyboardUtil' );
   const Node = require( 'SCENERY/nodes/Node' );
   const Property = require( 'AXON/Property' );
   const sun = require( 'SUN/sun' );
@@ -146,33 +145,10 @@ define( require => {
       }
     } );
 
-    //TODO sun#445 why is this listener on button? this entire listener appears to be related to the list.
-    // add the button accessibility listener
-    this.button.addInputListener( {
-
-      a11yclick: () => {
-
-        //TODO sun#314 order dependency, requires that showListbox was called first by button listener
-        if ( this.listBox.visible ) {
-          this.listBox.updateFocus();
-        }
-      },
-
-      // listen for escape to hide the list when focused on the button
-      keydown: event => {
-        if ( this.listBox.visible ) {
-          if ( event.domEvent.keyCode === KeyboardUtil.KEY_ESCAPE ) {
-            this.hideListbox();
-          }
-        }
-      }
-    } );
-
     // @private listener for 'click outside to dismiss'
     // TODO sun#314 handle this logic for a11y too, perhaps on by monitoring the focusout event on the display's root PDOM element
     this.clickToDismissListener = {
       down: event => {
-
         //TODO scenery#927 is trail.nodes public? should we add Trail.containsNode?
         // Ignore if we click over the button, since the button will handle hiding the list.
         if ( event.trail.nodes.indexOf( this.button ) === -1 ) {

diff --git a/js/ComboBoxListBox.js b/js/ComboBoxListBox.js
@@ -196,13 +196,8 @@ define( require => {
       };
       this.addInputListener( keyDownListener );
 
-      // Clear focus when the listbox becomes invisible
-      this.on( 'visibility', () => {
-        if ( !this.visible && this.focusedItemNode ) {
-          this.focusedItemNode.focusable = false;
-          this.focusedItemNode = null;
-        }
-      } );
+      // Update focus when the listbox's visibility changes.
+      this.on( 'visibility', () => { this.updateFocus(); } );
 
       // @private
       this.disposeComboBoxListBox = () => {
@@ -227,16 +222,26 @@ define( require => {
 
     /**
      * Updates the focus to match the currently selected value.
-     * @public
+     * @private
      */
     updateFocus() {
-      for ( let i = 0; i < this.listItemNodes.length; i++ ) {
-        const listItemNode = this.listItemNodes[ i ];
-        if ( this.property.value === listItemNode.item.value ) {
-          this.focusedItemNode = listItemNode;
-          this.focusedItemNode.a11yFocusButton();
+      if ( this.visible ) {
+
+        // listbox is visible, focus on selected item
+        for ( let i = 0; i < this.listItemNodes.length; i++ ) {
+          const listItemNode = this.listItemNodes[ i ];
+          if ( this.property.value === listItemNode.item.value ) {
+            this.focusedItemNode = listItemNode;
+            this.focusedItemNode.a11yFocusButton();
+          }
         }
       }
+      else if ( this.focusedItemNode ) {
+
+        // listbox is invisible, clear focus
+        this.focusedItemNode.focusable = false;
+        this.focusedItemNode = null;
+      }
     }
 
     // TODO: sun#314 we don't likely need this anymore