ComboBox: factor out classes for subcomponents

[pixelzoom]

ComboBox's list is currently just a Rectangle, and all of the responsibilities related to the list are spread out across ComboBox's constructor and methods. Creating a ListNode inner class to handle list responsibilities would go a long ways towards cleaning up the PhET-iO (#405) and a11y (#314) instrumentation.

[pixelzoom]

@ariel-phet Permission to work on this? It may be a multi-day project.

[ariel-phet]

@pixelzoom permission granted... ComboBox is used so much that having it in good shape seems very worthwhile for the project as whole.

[pixelzoom]

I think it might also be worthwhile to formalize "combo box list" by making it a class. It's currently duck typed, and that makes it difficult to document.

[zepumph]

Could we make a new folder in sun for ComboBox related code? If there are 5 classes I would prefer that organization to having all of them in the same file. That may help with readability too. Though it would make a need for many require statements to be refactored.

[pixelzoom]

Yep -- I'll evaluate breaking ComboBox's inner classes out into separate .js files.

[pixelzoom]

@zepumph FYI, so far I've factored out ComboBoxItem, ComboBoxItemNode, and ComboBoxButton into separate .js files. Coming soon is ComboBoxListNode.

[pixelzoom]

In light of #419, I'm rethinking the responsibilities and sharing of ComboBoxItemNode. Now I think that what should be shared by the button and list is ComboBoxItem.node, and ComboBoxItemNode should become the thing that appears in the list, supports highlighting, etc.

[pixelzoom]

In the previous commit, clicking on the button toggles the list's visibility:

    // Clicking on the button toggles visibility of the list
    this.button.addListener( () => {
      if ( !this.listBox.visible ) {
        this.showList();
      }
      else {
        this.hideList();
      }
    } );

And clickToDismissListener is short-circuited when the click is over the button:

    this.clickToDismissListener = {
      down: event => {

        //TODO sun#430 is Trail.nodes public? Is there a better way to determine if we clicked over this.button?
        // Ignore if we click over the button, since the button will handle hiding the list.
        if ( event.trail.nodes.indexOf( this.button ) === -1 ) {
          this.hideList();
        }
      }
    };

This seems to do the job. But see the TODO above.Trail does not have visibility annotations for its fields, so I don't know if event.trail.nodes is @public. (I've asked @jonathanolson on Slack.) And I'm surprised that Trail doesn't have a method that determines whether a specific Node is part of the Trail. E.g. something like:

/**
 * Does this Trail contain the specified Node?
 * @param {Node} node
 */
containsNode( node ) {
  return event.trail.nodes.indexOf( node ) !== -1;
}

[jessegreenberg]

Actually, I see clickToDismissListener's down function being called before the this.button's listener.

That is right, because the button's listener is called on up. I was trying to prevent the button from being pressed down in the first place.

[pixelzoom]

If trail.nodes turns out to be @private, another approach would be to remove the button listener when clickToDismissListener is added, then restore the button listener when clickToDismissListener is removed.

[pixelzoom]

Blocked by phetsims/scenery#927.

[pixelzoom]

I see that other sims are accessing trail.nodes, doing the same sort of test that I'm doing here in ComboBox. So let's proceed with this solution. If @jonathanolson decides in phetsims/scenery#927 that trail.nodes should be private, then a method to perform the test will be added to Trail. So...

@jessegreenberg regarding your feedback in #430 (comment)...

(1) is being addressed by #452.

(2) has been addressed as described in #430 (comment).

Anything else to do here? If not, feel free to close this issue.

[jessegreenberg]

OK, thanks for going through #452. I tested #430 (comment), it looks good and works good. I am going to remove this TODO now:

//TODO sun#430 is Trail.nodes public? Is there a better way to determine if we clicked over this.button?

Closing.

[pixelzoom]

Thanks for the review @jessegreenberg.

FYI, I added this TODO where you removed the sun#430 TODO:

//TODO scenery#927 is trail.nodes public? should we add Trail.containsNode?