From b47526a0d1fff3b08778959b4161b29130fd22ec Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Mon, 14 Jan 2019 18:41:18 -0700 Subject: [PATCH] big ComboBox refactors, https://github.com/phetsims/sun/issues/430, https://github.com/phetsims/sun/issues/428, https://github.com/phetsims/sun/issues/419 --- js/ComboBox.js | 218 ++++++++---------- js/ComboBoxButton.js | 201 ++++++++++++++++ js/ComboBoxButtonNode.js | 194 ---------------- ...BoxItemNode.js => ComboBoxListItemNode.js} | 71 +++--- js/demo/DemosScreenView.js | 6 +- 5 files changed, 343 insertions(+), 347 deletions(-) create mode 100644 js/ComboBoxButton.js delete mode 100644 js/ComboBoxButtonNode.js rename js/{ComboBoxItemNode.js => ComboBoxListItemNode.js} (52%) diff --git a/js/ComboBox.js b/js/ComboBox.js index dcd59bb8..c4ff2049 100644 --- a/js/ComboBox.js +++ b/js/ComboBox.js @@ -12,10 +12,10 @@ define( require => { // modules const AccessiblePeer = require( 'SCENERY/accessibility/AccessiblePeer' ); - const ComboBoxButtonNode = require( 'SUN/ComboBoxButtonNode' ); + const ComboBoxButton = require( 'SUN/ComboBoxButton' ); const ComboBoxIO = require( 'SUN/ComboBoxIO' ); const ComboBoxItem = require( 'SUN/ComboBoxItem' ); - const ComboBoxItemNode = require( 'SUN/ComboBoxItemNode' ); + const ComboBoxListItemNode = require( 'SUN/ComboBoxListItemNode' ); const Emitter = require( 'AXON/Emitter' ); const EmitterIO = require( 'AXON/EmitterIO' ); const inherit = require( 'PHET_CORE/inherit' ); @@ -33,6 +33,15 @@ define( require => { const LIST_POSITION_VALUES = [ 'above', 'below' ]; const ALIGN_VALUES = [ 'left', 'right', 'center' ]; + //TODO fix renamed options in client code + // X listCornerRadius -> cornerRadius + // X itemHighlightFill -> highlightFill + // X itemHighlightStroke deleted + // X buttonCornerRadius: 8, + // buttonXMargin: 10, + // buttonYMargin: 4, + + //TODO sun#430 change order of items and property /** * @param {*[]} items - must be created using ComboBox.createItem * @param {Property} property @@ -44,34 +53,34 @@ define( require => { options = _.extend( { + align: 'left', // see ALIGN_VALUES listPosition: 'below', // {string} where the list pops up relative to the button - align: 'left', // {string} alignment of items on the button and in the list, see ALIGN_VALUES - labelNode: null, // optional label, placed to the left of the combo box + labelNode: null, // {Node|null} optional label, placed to the left of the combo box labelXSpacing: 10, // horizontal space between label and combo box enabledProperty: new Property( true ), disabledOpacity: 0.5, // {number} opacity used to make the control look disabled + cornerRadius: 8, // {number} applied to list and button + highlightFill: 'rgb( 245, 245, 245 )', // highlight behind items in the list + xMargin: 12, // margin on left and right of the list when highlight is invisible + yMargin: 8, // margin on top and bottom of the list when highlight is invisible + + // button + buttonFill: 'white', + buttonStroke: 'black', + buttonLineWidth: 1, + arrowHeight: null, // {number|null} defaults to max item height // list - listYMargin: 4, listFill: 'white', listStroke: 'black', listLineWidth: 1, - listCornerRadius: 5, - - // items - itemXMargin: 6, - itemYMargin: 6, // Vertical margin applied to the top and bottom of each item in the popup list. - itemHighlightFill: 'rgb(245,245,245)', - itemHighlightStroke: null, - itemHighlightLineWidth: 1, // phet-io tandem: Tandem.required, phetioType: ComboBoxIO, phetioEventType: 'user' - //TODO sun#422 use nested options for ComboBoxButtonNode - }, ComboBoxButtonNode.DEFAULT_OPTIONS, options ); + }, options ); // validate option values assert && assert( options.disabledOpacity > 0 && options.disabledOpacity < 1, @@ -114,17 +123,22 @@ define( require => { down: this.hideListFromClick.bind( this ) }; - // determine uniform dimensions for button and list items (including margins) - const itemWidth = Math.max.apply( Math, _.map( items, 'node.width' ) ) + 2 * options.itemXMargin; - const itemHeight = Math.max.apply( Math, _.map( items, 'node.height' ) ) + 2 * options.itemYMargin; + // Compute max item dimensions + const maxItemWidth = _.maxBy( items, item => item.node.width ).node.width; + const maxItemHeight = _.maxBy( items, item => item.node.height ).node.height; - const listWidth = itemWidth + ( 2 * options.buttonXMargin ); - const listHeight = ( items.length * itemHeight ) + ( 2 * options.listYMargin ); + // Uniform dimensions for highlighted item in the list + const highlightWidth = maxItemWidth + options.xMargin; + const highlightHeight = maxItemHeight + options.yMargin; + + // List dimensions + const listWidth = highlightWidth + options.xMargin; + const listHeight = ( items.length * highlightHeight ) + options.yMargin; //TODO #430 factor out ListNode inner class, to handle all list responsibilities // @private the popup list this.listNode = new Rectangle( 0, 0, listWidth, listHeight, { - cornerRadius: options.listCornerRadius, + cornerRadius: options.cornerRadius, fill: options.listFill, stroke: options.listStroke, lineWidth: options.listLineWidth, @@ -141,49 +155,32 @@ define( require => { listParent.addChild( this.listNode ); this.listParent = listParent; // @private - // Highlights an item in the list - const highlightItem = itemNodeWrapper => { - assert && assert( itemNodeWrapper instanceof Rectangle, 'invalid itemNodeWrapper: ' + itemNodeWrapper ); - itemNodeWrapper.fill = options.itemHighlightFill; - itemNodeWrapper.stroke = options.itemHighlightStroke; - }; - - // Un-highlights an item in the list - const unhighlightItem = itemNodeWrapper => { - assert && assert( itemNodeWrapper instanceof Rectangle, 'invalid itemNodeWrapper: ' + itemNodeWrapper ); - itemNodeWrapper.fill = null; - itemNodeWrapper.stroke = null; - }; - - // TODO sun#405 It seems it would be better to use FireListener on each ComboBoxItemNode + // TODO sun#405 It seems it would be better to use FireListener on each ComboBoxListItemNode const firedEmitter = new Emitter( { tandem: options.tandem.createTandem( 'firedEmitter' ), phetioType: EmitterIO( [ { name: 'event', type: VoidIO } ] ), // TODO sun#405 Should this be EventIO or DOMEventIO? listener: event => { - const itemNodeWrapper = event.currentTarget; // {Rectangle} - - unhighlightItem( itemNodeWrapper ); + const listItemNode = event.currentTarget; // {Rectangle} + assert && assert( listItemNode instanceof ComboBoxListItemNode, 'expected a ComboBoxListItemNode' ); // a11y - keep this PDOM attribute in sync - this.updateActiveDescendant( itemNodeWrapper ); + this.updateActiveDescendant( listItemNode ); this.hideList(); + listItemNode.setHighlightVisible( false ); event.abort(); // prevent nodes (eg, controls) behind the list from receiving the event - //TODO #430 this is brittle, should be handled better when we factor out ListNode - const itemNode = itemNodeWrapper.children[ 0 ]; - assert && assert( itemNode instanceof ComboBoxItemNode, 'expected the wrapper child to be ComboBoxItemNode' ); - property.value = itemNode.item.value; + property.value = listItemNode.item.value; } } ); // listener that we'll attach to each item in the list const itemListener = { enter( event ) { - highlightItem( event.currentTarget ); + event.currentTarget.setHighlightVisible( true ); }, exit( event ) { - unhighlightItem( event.currentTarget ); + event.currentTarget.setHighlightVisible( false ); }, down( event ) { event.abort(); // prevent click-to-dismiss on the list @@ -198,31 +195,29 @@ define( require => { let fromA11yEnterKey = false; // @private populate list with items - this.itemNodes = []; + this.listItemNodes = []; items.forEach( ( item, index ) => { // Create the list item node - const itemNode = new ComboBoxItemNode( item, itemWidth, itemHeight, { - xMargin: options.itemXMargin, - tandem: item.tandemName ? options.tandem.createTandem( item.tandemName ) : Tandem.optional + const listItemNode = new ComboBoxListItemNode( item, highlightWidth, highlightHeight, { + align: options.align, + highlightFill: options.highlightFill, + + // highlight overlaps half of margins + xMargin: 0.5 * options.xMargin, + left: 0.5 * options.xMargin, + top: ( 0.5 * options.yMargin ) + ( index * highlightHeight ), + tandem: item.tandemName ? options.tandem.createTandem( item.tandemName ) : Tandem.optional, + a11yLabel: item.a11yLabel } ); - this.itemNodes.push( itemNode ); - - // ComboBoxItemNodes are shared between the list and button, so parent the itemNode with a Rectangle that we can highlight. - const itemNodeWrapper = new Rectangle( 0, 0, itemNode.width, itemNode.height, { - children: [ itemNode ], - inputListeners: [ itemListener ], - align: options.align, //TODO sun#430 options.align is currently broken - left: options.buttonXMargin, - top: options.listYMargin + ( index * itemHeight ), - cursor: 'pointer' - } ); - this.listNode.addChild( itemNodeWrapper ); + listItemNode.addInputListener( itemListener ); + this.listItemNodes.push( listItemNode ); + this.listNode.addChild( listItemNode ); //TODO sun#314 a11yClickListener should not be assigned here, it should be set via options or a setter method // a11y - select the property and close on a click event from assistive technology, must be removed in disposal - // of combobox item. Keep track of it on the itemNode for disposal. - itemNode.a11yClickListener = itemNode.addInputListener( { + // of combobox item. Keep track of it on the listItemNode for disposal. + listItemNode.a11yClickListener = { keydown: event => { if ( KeyboardUtil.KEY_ENTER === event.domEvent.keyCode || KeyboardUtil.KEY_SPACE === event.domEvent.keyCode ) { fromA11yEnterKey = KeyboardUtil.KEY_ENTER === event.domEvent.keyCode; // only for the enter key @@ -231,13 +226,14 @@ define( require => { this.buttonNode.focus(); // a11y - keep this PDOM attribute in sync - this.updateActiveDescendant( itemNode ); + this.updateActiveDescendant( listItemNode ); } } - } ); + }; + listItemNode.addInputListener( listItemNode.a11yClickListener ); } ); - // @private {ComboBoxItemNode} - a11y + // @private {ComboBoxListItemNode} - a11y // tracks which item node has focus to make it easy to focus next/previous item after keydown this.focusedItem = null; @@ -276,36 +272,41 @@ define( require => { } } ); - // Cherry pick button options - const buttonOptions = _.pick( options, _.keys( ComboBoxButtonNode.DEFAULT_OPTIONS ) ); - buttonOptions.arrowDirection = ( options.listPosition === 'below' ) ? 'down' : 'up'; - - // @private button, will be set to correct value when property observer is registered - this.buttonNode = new ComboBoxButtonNode( this.getItemNode( property.value ), buttonOptions); - this.addChild( this.buttonNode ); + // @private button that shows the current selection + this.button = new ComboBoxButton( property, items, { + align: options.align, + arrowHeight: options.arrowHeight, + arrowDirection: ( options.listPosition === 'below' ) ? 'down' : 'up', + cornerRadius: options.cornerRadius, + xMargin: options.xMargin, + yMargin: options.yMargin, + fill: options.buttonFill, + stroke: options.buttonStroke, + lineWidth: options.buttonLineWidth + } ); + this.addChild( this.button ); // a11y - the list is labeled by the button's label this.listNode.addAriaLabelledbyAssociation( { - otherNode: this.buttonNode, + otherNode: this.button, otherElementName: AccessiblePeer.LABEL_SIBLING, thisElementName: AccessiblePeer.PRIMARY_SIBLING } ); // button interactivity - this.buttonNode.cursor = 'pointer'; - this.buttonNode.addInputListener( { + this.button.addInputListener( { down: this.showList.bind( this ) } ); - //TODO sun#314 This should not be done by reaching into buttonNode. a11yListener should be set via options or a setter method - // add the buttonNode accessibility listener - this.buttonNode.a11yListener = { + //TODO sun#314 This should not be done by reaching into button. a11yListener should be set via options or a setter method + // add the button accessibility listener + this.button.a11yListener = { click: () => { // if already visible, hide it if ( this.listNode.visible ) { this.hideList(); - this.buttonNode.focus(); + this.button.focus(); } // Otherwise show the list and manage focus properly. But be tolerant of the "double enter" loop case, where @@ -316,13 +317,10 @@ define( require => { // focus the selected item for ( let i = 0; i < this.listNode.children.length; i++ ) { - //TODO #430 this is brittle, should be handled better when we factor out ListNode - const itemNodeWrapper = this.listNode.children[ i ]; - const itemNode = itemNodeWrapper.children[ 0 ]; - assert && assert( itemNode instanceof ComboBoxItemNode, 'expected ComboBoxItemNode' ); + const listItemNode = this.listNode.children[ i ]; - if ( property.value === itemNode.item.value ) { - this.focusedItem = itemNode; + if ( property.value === listItemNode.item.value ) { + this.focusedItem = listItemNode; this.focusedItem.a11yFocusButton(); } } @@ -343,20 +341,14 @@ define( require => { } } }; - this.buttonNode.addInputListener( this.buttonNode.a11yListener ); + this.button.addInputListener( this.button.a11yListener ); // layout if ( options.labelNode ) { - this.buttonNode.left = options.labelNode.right + options.labelXSpacing; - this.buttonNode.centerY = options.labelNode.centerY; + this.button.left = options.labelNode.right + options.labelXSpacing; + this.button.centerY = options.labelNode.centerY; } - // when property changes, update button, and for a11y the list in the PDOM - const propertyObserver = value => { - this.buttonNode.setItemNode( this.getItemNode( value ) ); - }; - property.link( propertyObserver ); - this.mutate( options ); // enable/disable the combo box @@ -369,22 +361,19 @@ define( require => { // @private called by dispose this.disposeComboBox = () => { - if ( property.hasListener( propertyObserver ) ) { - property.unlink( propertyObserver ); - } if ( this.enabledProperty.hasListener( enabledObserver ) ) { this.enabledProperty.unlink( enabledObserver ); } - // Unregister itemNode tandems as well - for ( let i = 0; i < this.itemNodes; i++ ) { - this.itemNodes[ i ].dispose(); + // Unregister listItemNode tandems as well + for ( let i = 0; i < this.listItemNodes; i++ ) { + this.listItemNodes[ i ].dispose(); } // remove a11y listeners this.listNode.removeInputListener( handleKeyDown ); - this.buttonNode.dispose(); + this.button.dispose(); }; // support for binder documentation, stripped out in builds and only runs when ?binder is specified @@ -422,28 +411,13 @@ define( require => { return item; }, - /** - * Gets the ComboBoxItemNode associated with a specified value. - * @param {*} value - * @returns {ComboBoxItemNode} itemNode - * @private - */ - getItemNode( value ) { - const item = this.getItem( value ); - const itemNode = _.find( this.itemNodes, child => { - return ( child instanceof ComboBoxItemNode && child.item === item ); - } ); - assert && assert( itemNode, 'no ComboBoxItemNode is associated with item: ' + item ); - return itemNode; - }, - // @private - update this attribute on the listNode. This changes as you interact // with the comboBox, as well as when an item is selected. - updateActiveDescendant( itemNode ) { + updateActiveDescendant( listItemNode ) { // overwrite purposefully this.listNode.activeDescendantAssociations = [ { - otherNode: itemNode, + otherNode: listItemNode, thisElementName: AccessiblePeer.PRIMARY_SIBLING, otherElementName: AccessiblePeer.PRIMARY_SIBLING } ]; @@ -510,13 +484,13 @@ define( require => { */ moveList() { if ( this.listPosition === 'above' ) { - const pButtonGlobal = this.localToGlobalPoint( new Vector2( this.buttonNode.left, this.buttonNode.top ) ); + const pButtonGlobal = this.localToGlobalPoint( new Vector2( this.button.left, this.button.top ) ); const pButtonLocal = this.listParent.globalToLocalPoint( pButtonGlobal ); this.listNode.left = pButtonLocal.x; this.listNode.bottom = pButtonLocal.y; } else { - const pButtonGlobal = this.localToGlobalPoint( new Vector2( this.buttonNode.left, this.buttonNode.bottom ) ); + const pButtonGlobal = this.localToGlobalPoint( new Vector2( this.button.left, this.button.bottom ) ); const pButtonLocal = this.listParent.globalToLocalPoint( pButtonGlobal ); this.listNode.left = pButtonLocal.x; this.listNode.top = pButtonLocal.y; diff --git a/js/ComboBoxButton.js b/js/ComboBoxButton.js new file mode 100644 index 00000000..991bbce3 --- /dev/null +++ b/js/ComboBoxButton.js @@ -0,0 +1,201 @@ +// Copyright 2019, University of Colorado Boulder + +//TODO sun#430 extend RectangularPushButton +/** + * The button on a combo box box. Displays the current selection on the button. + * + * @author Chris Malley (PixelZoom, Inc.) + */ +define( require => { + 'use strict'; + + // modules + const AccessiblePeer = require( 'SCENERY/accessibility/AccessiblePeer' ); + const HStrut = require( 'SCENERY/nodes/HStrut' ); + const Line = require( 'SCENERY/nodes/Line' ); + const Node = require( 'SCENERY/nodes/Node' ); + const Path = require( 'SCENERY/nodes/Path' ); + const Rectangle = require( 'SCENERY/nodes/Rectangle' ); + const Shape = require( 'KITE/Shape' ); + const sun = require( 'SUN/sun' ); + const Tandem = require( 'TANDEM/Tandem' ); + + // constants + const ALIGN_VALUES = [ 'left', 'center', 'right' ]; + const ARROW_DIRECTION_VALUES = [ 'up', 'down' ]; + + class ComboBoxButton extends Node { + + /** + * @param {Property} property + * @param {ComboBoxItem[]} items + * @param {Object} [options] + */ + constructor( property, items, options ) { + + options = _.extend( { + + cursor: 'pointer', + align: 'left', // see ALIGN_VALUES + arrowHeight: null, // {number|null} if null, will be computed based on max item height + arrowDirection: 'down', // see ARROW_DIRECTION_VALUES + arrowFill: 'black', + xMargin: 10, // margin on the left and right of the item + yMargin: 10, // margin on the top and bottom of the item + fill: 'white', + stroke: 'black', + lineWidth: 1, + cornerRadius: 8, + + // phet-io + tandem: Tandem.optional, + + // a11y + tagName: 'button', + labelTagName: 'span', + containerTagName: 'div', + a11yLabel: null + + }, options ); + + assert && assert( ALIGN_VALUES.includes( options.align ), + 'invalid align: ' + options.align ); + assert && assert( ARROW_DIRECTION_VALUES.includes( options.arrowDirection ), + 'invalid arrowDirection: ' + options.arrowDirection ); + + // Compute max item size + const maxItemWidth = _.maxBy( items, item => item.node.width ).node.width; + const maxItemHeight = _.maxBy( items, item => item.node.height ).node.height; + + // Compute arrow size + const arrowHeight = options.arrowHeight || ( 0.7 * maxItemHeight ); // height of equilateral triangle + const arrowWidth = 2 * arrowHeight * Math.sqrt( 3 ) / 3; // side of equilateral triangle + + // button background, behind the item and the arrow + const backgroundWidth = maxItemWidth + ( 4 * options.xMargin ) + arrowWidth; + const backgroundHeight = maxItemHeight + ( 2 * options.yMargin); + const background = new Rectangle( 0, 0, backgroundWidth, backgroundHeight, { + cornerRadius: options.cornerRadius, + fill: options.fill, + stroke: options.stroke, + lineWidth: options.lineWidth + } ); + + // invisible horizontal strut behind item, to make alignment of item easier + const itemStrut = new HStrut( maxItemWidth, { + left: options.xMargin, + centerY: background.centerY + } ); + + // vertical separator between item and arrow + const separator = new Line( 0, 0, 0, backgroundHeight, { + stroke: 'black', + lineWidth: options.lineWidth, + centerX: itemStrut.right + options.xMargin, + centerY: background.centerY + } ); + + // arrow that points up or down, to indicate which way the list pops up + let arrowShape = null; + if ( options.arrowDirection === 'up' ) { + arrowShape = new Shape() + .moveTo( 0, arrowHeight ) + .lineTo( arrowWidth / 2, 0 ) + .lineTo( arrowWidth, arrowHeight ) + .close(); + } + else { + arrowShape = new Shape() + .moveTo( 0, 0 ) + .lineTo( arrowWidth, 0 ) + .lineTo( arrowWidth / 2, arrowHeight ) + .close(); + } + const arrow = new Path( arrowShape, { + fill: options.arrowFill, + left: separator.centerX + options.xMargin, + centerY: background.centerY + } ); + + // Wrapper for the selected item's Node. Do not transform ComboBoxItem.node because it is shared with list! + const itemNodeWrapper = new Node(); + + assert && assert( !options.children, 'ComboBoxButton sets children' ); + options.children = [ background, itemStrut, itemNodeWrapper, separator, arrow ]; + super( options ); + + // When property's value changes, show the corresponding item's Node on the button. + const propertyObserver = value => { + + // remove the node for the previous item + itemNodeWrapper.removeAllChildren(); + + // find and add the node for the new item + let node = null; + for ( var i = 0; i < items.length && !node; i++ ) { + if ( items[ i ].value === value ) { + node = items[ i ].node; + } + } + assert && assert( node, 'no item found for value: ' + value ); + itemNodeWrapper.addChild( node ); + + // adjust alignment + itemNodeWrapper.centerY = background.centerY; + if ( options.align === 'left' ) { + itemNodeWrapper.left = itemStrut.left; + } + else if ( options.align === 'right' ) { + itemNodeWrapper.right = itemStrut.right; + } + else { + itemNodeWrapper.centerX = itemStrut.centerX; + } + }; + property.link( propertyObserver ); + + //TODO #314 missing visibility annotation + this.labelContent = options.a11yLabel; + + //TODO #314 missing visibility annotation + // the button is labelledby its own label, and then (second) by itself. Order matters! + assert && assert( !options.ariaLabelledbyAssociations, 'ComboBoxButton sets ariaLabelledbyAssociations' ); + this.ariaLabelledbyAssociations = [ + { + otherNode: this, + otherElementName: AccessiblePeer.LABEL_SIBLING, + thisElementName: AccessiblePeer.PRIMARY_SIBLING + }, + { + otherNode: this, + otherElementName: AccessiblePeer.PRIMARY_SIBLING, + thisElementName: AccessiblePeer.PRIMARY_SIBLING + } + ]; + + // signify to AT that this button opens a menu + this.setAccessibleAttribute( 'aria-haspopup', 'listbox' ); + + //TODO #314 this should be private, set via options or a setter methods + // @public - if assigned, it will be removed on disposal. + this.a11yListener = null; + + // @private + this.disposeComboBoxButton = () => { + property.unlink( propertyObserver ); + this.a11yListener && this.removeInputListener( this.a11yListener ); + }; + } + + /** + * @public + * @override + */ + dispose() { + this.disposeComboBoxButton(); + super.dispose(); + } + } + + return sun.register( 'ComboBoxButton', ComboBoxButton ); +} ); \ No newline at end of file diff --git a/js/ComboBoxButtonNode.js b/js/ComboBoxButtonNode.js deleted file mode 100644 index e7528d35..00000000 --- a/js/ComboBoxButtonNode.js +++ /dev/null @@ -1,194 +0,0 @@ -// Copyright 2019, University of Colorado Boulder - -//TODO sun#430 this should extend RectangularPushButton -/** - * The button on a combo box box. This was originally an inner class of ComboBox (ComboBox.ButtonNode), - * but was promoted to its own source file when it grew more complex due to PhET-iO and a11y instrumentation. - * - * @author Chris Malley (PixelZoom, Inc.) - */ -define( require => { - 'use strict'; - - // modules - const AccessiblePeer = require( 'SCENERY/accessibility/AccessiblePeer' ); - const Line = require( 'SCENERY/nodes/Line' ); - const Node = require( 'SCENERY/nodes/Node' ); - const Path = require( 'SCENERY/nodes/Path' ); - const Rectangle = require( 'SCENERY/nodes/Rectangle' ); - const Shape = require( 'KITE/Shape' ); - const sun = require( 'SUN/sun' ); - const Tandem = require( 'TANDEM/Tandem' ); - - // constants - const ARROW_DIRECTION_VALUES = [ 'up', 'down' ]; - - //TODO sun#422 use nested options for ComboBoxButtonNode - const DEFAULT_OPTIONS = { - - // used by ComboBox and ComboBoxButtonNode - arrowDirection: 'down', // {string} direction that the arrow points - - // used exclusively by ComboBoxButtonNode - buttonFill: 'white', - buttonStroke: 'black', - buttonLineWidth: 1, - buttonCornerRadius: 8, - buttonXMargin: 10, - buttonYMargin: 4, - - // a11y - used exclusively by ComboBoxButtonNode - a11yButtonLabel: '' // {string} accessible label for the button that opens this combobox - }; - - class ComboBoxButtonNode extends Node { - - /** - * @param {Node} itemNode - * @param {Object} [options] - */ - constructor( itemNode, options ) { - - options = _.extend( {}, DEFAULT_OPTIONS, { - - // phet-io - tandem: Tandem.optional, // ComboBoxButtonNode is not currently instrumented - - // a11y - tagName: 'button', - labelTagName: 'span', - containerTagName: 'div' - - }, options ); - - assert && assert( ARROW_DIRECTION_VALUES.includes( options.arrowDirection ), - 'invalid arrowDirection: ' + options.arrowDirection ); - - super(); - - // @private - this.buttonXMargin = options.buttonXMargin; - this.buttonYMargin = options.buttonYMargin; - - //TODO #314 missing visibility annotation - this.labelContent = options.a11yButtonLabel; - - //TODO #314 missing visibility annotation - // the button is labelledby its own label, and then (second) by itself. Order matters! - assert && assert( !options.ariaLabelledbyAssociations, 'ComboBoxButtonNode sets ariaLabelledbyAssociations' ); - this.ariaLabelledbyAssociations = [ - { - otherNode: this, - otherElementName: AccessiblePeer.LABEL_SIBLING, - thisElementName: AccessiblePeer.PRIMARY_SIBLING - }, - { - otherNode: this, - otherElementName: AccessiblePeer.PRIMARY_SIBLING, - thisElementName: AccessiblePeer.PRIMARY_SIBLING - } - ]; - - // signify to AT that this button opens a menu - this.setAccessibleAttribute( 'aria-haspopup', 'listbox' ); - - //TODO #314 this should be private, and its value should be passed set via options or a setter methods - // @public - if assigned, it will be removed on disposal. - this.a11yListener = null; - - // up or down arrow - const arrow = new Path( null, { - fill: 'black' - } ); - const arrowWidth = 0.5 * itemNode.height; - const arrowHeight = arrowWidth * Math.sqrt( 3 ) / 2; // height of equilateral triangle - if ( options.arrowDirection === 'up' ) { - arrow.shape = new Shape().moveTo( 0, arrowHeight ).lineTo( arrowWidth / 2, 0 ).lineTo( arrowWidth, arrowHeight ).close(); // up arrow - } - else { - arrow.shape = new Shape().moveTo( 0, 0 ).lineTo( arrowWidth, 0 ).lineTo( arrowWidth / 2, arrowHeight ).close(); // down arrow - } - - // button background - const backgroundWidth = itemNode.width + ( 4 * this.buttonXMargin ) + arrow.width; - const backgroundHeight = itemNode.height + ( 2 * this.buttonYMargin ); - - // @private - this.background = new Rectangle( 0, 0, backgroundWidth, backgroundHeight, { - cornerRadius: options.buttonCornerRadius, - fill: options.buttonFill, - stroke: options.buttonStroke, - lineWidth: options.buttonLineWidth - } ); - - // vertical separator to left of arrow - const separator = new Line( 0, 0, 0, backgroundHeight, { - stroke: 'black', - lineWidth: options.buttonLineWidth - } ); - - // @private parent for the selected ComboBoxItemNode - this.itemNodeParent = new Node(); - - // rendering order - this.addChild( this.background ); - this.addChild( arrow ); - this.addChild( separator ); - this.addChild( this.itemNodeParent ); - - this.setItemNode( itemNode ); - - // layout - separator.left = this.itemNodeParent.right + this.buttonXMargin; - separator.top = this.background.top; - arrow.left = separator.right + this.buttonXMargin; - arrow.centerY = this.background.centerY; - - // @private - this.disposeComboBoxButtonNode = () => { - this.a11yListener && this.removeInputListener( this.a11yListener ); - }; - - this.mutate( options ); - } - - /** - * Sets the item that is displayed on the button. - * @param {ComboBoxItemNode} itemNode - */ - setItemNode( itemNode ){ - - // Remove the previous itemNode - assert && assert( this.itemNodeParent.getChildrenCount() <= 1, - 'itemNodeParent should never have more than 1 child' ); - this.itemNodeParent.removeAllChildren(); - - // Add the new itemNode and adjust layout. Do not transform itemNode, because it's shared with the list. - this.itemNodeParent.addChild( itemNode ); - this.itemNodeParent.left = this.background.left + this.buttonXMargin; - this.itemNodeParent.top = this.background.top + this.buttonYMargin; - - // TODO sun#314 is there a better way to do this? - itemNode.a11yShowItem( false ); - - // Only set if defined, since it is an option, see ComboBox.createItem - if ( itemNode.a11yLabel ) { - this.innerContent = itemNode.a11yLabel; - } - } - - /** - * @public - * @override - */ - dispose() { - this.disposeComboBoxButtonNode(); - super.dispose(); - } - } - - //TODO sun#430 make this go away, use nested options.buttonOptions in ComboBox? - ComboBoxButtonNode.DEFAULT_OPTIONS = DEFAULT_OPTIONS; - - return sun.register( 'ComboBoxButtonNode', ComboBoxButtonNode ); -} ); \ No newline at end of file diff --git a/js/ComboBoxItemNode.js b/js/ComboBoxListItemNode.js similarity index 52% rename from js/ComboBoxItemNode.js rename to js/ComboBoxListItemNode.js index 23b3cbe6..8b5bf052 100644 --- a/js/ComboBoxItemNode.js +++ b/js/ComboBoxListItemNode.js @@ -1,10 +1,9 @@ // Copyright 2019, University of Colorado Boulder /** - * Node for an item in a combo box. Typically instantiated by ComboBox, not by client code. This was originally - * an inner class of ComboBox (ComboBox.ItemNode), but was promoted to its own source file when it grew more - * complex due to PhET-iO and a11y instrumentation. - * + * Node for an item in a combo box list. + * Typically instantiated by ComboBox, not by client code. + * * @author Chris Malley (PixelZoom, Inc.) */ define( require => { @@ -18,7 +17,7 @@ define( require => { const sun = require( 'SUN/sun' ); const Tandem = require( 'TANDEM/Tandem' ); - class ComboBoxItemNode extends Rectangle { + class ComboBoxListItemNode extends Node { /** * @param {ComboBoxItem} item @@ -33,47 +32,49 @@ define( require => { // TODO sun#314 assert you may not be allowed to have accessibleContent on the item.node, since we set the innerContent on this LI options = _.extend( { + + cursor: 'pointer', align: 'left', xMargin: 6, + highlightFill: 'rgb( 245, 245, 245 )', // phet-io tandem: Tandem.required, // a11y tagName: 'li', - ariaRole: 'option' + ariaRole: 'option', + a11yLabel: null + }, options ); - // Holds our item.node, and positions it in the correct location. We don't want to mutate the item's node itself. - const itemWrapper = new Node( { + // Highlight rectangle + const highlightRectangle = new Rectangle( 0, 0, width, height ); + + // Wrapper for the item's Node. Do not transform ComboBoxItem.node because it is shared with ComboBoxButton! + const itemNodeWrapper = new Node( { children: [ item.node ], - pickable: false, centerY: height / 2 } ); if ( options.align === 'left' ) { - itemWrapper.left = options.xMargin; + itemNodeWrapper.left = highlightRectangle.left + options.xMargin; } else if ( options.align === 'right' ) { - itemWrapper.right = width - options.xMargin; + itemNodeWrapper.right = highlightRectangle.right - options.xMargin; } else { - // center - itemWrapper.centerX = width / 2; + itemNodeWrapper.centerX = highlightRectangle.centerX; } - assert && assert( !options.children, 'ComboBoxItemNode sets children' ); - options.children = [ itemWrapper ]; - - super( 0, 0, width, height, options ); + assert && assert( !options.children, 'ComboBoxListItemNode sets children' ); + options.children = [ highlightRectangle, itemNodeWrapper ]; - //TODO #314 this should be @public (read-only) - // @public - to keep track of it - this.a11yLabel = null; + super( options ); // Only set if defined, since it is an option, see ComboBox.createItem - if ( item.a11yLabel ) { - this.a11yLabel = item.a11yLabel; - this.innerContent = item.a11yLabel; //TODO #314 is this correct? if so, document why. + if ( options.a11yLabel ) { + this.a11yLabel = options.a11yLabel; + this.innerContent = options.a11yLabel; //TODO #314 is this correct? if so, document why. } //TODO #314 This is marked private, but is assigned in ComboBox. It should be set via options or a setter method. @@ -84,10 +85,22 @@ define( require => { this.item = item; // @private - this.itemWrapper = itemWrapper; + this.itemNodeWrapper = itemNodeWrapper; + this.highlightRectangle = highlightRectangle; + this.highlightFill = options.highlightFill; // the highlight wraps around the entire item rectangle - this.itemWrapper.focusHighlight = Shape.bounds( this.itemWrapper.parentToLocalBounds( this.localBounds ) ); + this.itemNodeWrapper.focusHighlight = Shape.bounds( this.itemNodeWrapper.parentToLocalBounds( this.localBounds ) ); + } + + /** + * @param {boolean} visible + * @public + */ + setHighlightVisible( visible ) { + + // Change fill instead of visibility so that we don't end up with vertical pointer gaps in the list + this.highlightRectangle.fill = visible ? this.highlightFill : null; } //TODO sun#314 doc/rename to toggleVisibility @@ -95,11 +108,11 @@ define( require => { * @param {boolean} visible */ a11yShowItem( visible ) { - this.itemWrapper.tagName = visible ? 'button' : null; + this.itemNodeWrapper.tagName = visible ? 'button' : null; this.tagName = visible ? 'li' : null; } - //TODO #314 ComboBoxItemNode instances are now shared between the list and button. How does that affect focus? Should focus be set on the ComboBoxItemNode's wrapper in the list? + //TODO #314 ComboBoxListItemNode instances are now shared between the list and button. How does that affect focus? Should focus be set on the ComboBoxListItemNode's wrapper in the list? /** * Focus the item in the list * @public @@ -118,10 +131,10 @@ define( require => { // the item in the button will not have a listener this.a11yClickListener && this.removeInputListener( this.a11yClickListener ); - this.itemWrapper.dispose(); + this.itemNodeWrapper.dispose(); //TODO #314 why is this needed? super.dispose(); } } - return sun.register( 'ComboBoxItemNode', ComboBoxItemNode ); + return sun.register( 'ComboBoxListItemNode', ComboBoxListItemNode ); } ); \ No newline at end of file diff --git a/js/demo/DemosScreenView.js b/js/demo/DemosScreenView.js index de7cfd47..ed4cc044 100644 --- a/js/demo/DemosScreenView.js +++ b/js/demo/DemosScreenView.js @@ -33,7 +33,8 @@ define( function( require ) { options = _.extend( { comboBoxLocation: new Vector2( 20, 20 ), // {Vector2} location of ComboBox used to select a demo comboBoxItemFont: new PhetFont( 20 ), // {Font} font used for ComboBox items - comboBoxItemYMargin: 6, // {number} y margin around ComboBox items + comboBoxItemXMargin: 12, // {number} x margin around ComboBox items + comboBoxItemYMargin: 12, // {number} y margin around ComboBox items selectedDemoLabel: null, // {string|null} label field of the demo to be selected initially // {boolean} see https://github.com/phetsims/sun/issues/386 @@ -86,7 +87,8 @@ define( function( require ) { var selectedDemoProperty = new Property( selectedDemo ); var comboBox = new ComboBox( comboBoxItems, selectedDemoProperty, listParent, { buttonFill: 'rgb( 218, 236, 255 )', - itemYMargin: options.comboBoxItemYMargin, + xMargin: options.comboBoxItemXMargin, + yMargin: options.comboBoxItemYMargin, top: options.comboBoxLocation.x, left: options.comboBoxLocation.y, tandem: options.tandem.createTandem( 'comboBox' )