From 7ee256fac23d243250f40f72017e099f78e4f08d Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Thu, 17 Jan 2019 11:51:18 -0700 Subject: [PATCH 1/9] factor out ComboBoxListBox, #445 Signed-off-by: Chris Malley --- js/ComboBox.js | 249 ++++++---------------------------------- js/ComboBoxListBox.js | 260 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 297 insertions(+), 212 deletions(-) create mode 100644 js/ComboBoxListBox.js diff --git a/js/ComboBox.js b/js/ComboBox.js index 08999305..dcc981db 100644 --- a/js/ComboBox.js +++ b/js/ComboBox.js @@ -1,9 +1,9 @@ // Copyright 2013-2018, University of Colorado Boulder /** - * Scenery-based combo box. Composed of a button and a list of items. - * The list of items is displayed when the button is pressed, and dismissed when an item is selected - * or the user clicks outside the list. The list can be displayed either above or below the button. + * Scenery-based combo box. Composed of a button and a popup listbox of items. + * The listbox is displayed when the button is pressed, and dismissed when an item is selected + * or the user clicks outside the list. The list can be displayed either above or below the button. * * @author Chris Malley (PixelZoom, Inc.) */ @@ -11,23 +11,18 @@ define( require => { 'use strict'; // modules - const AccessiblePeer = require( 'SCENERY/accessibility/AccessiblePeer' ); const ComboBoxButton = require( 'SUN/ComboBoxButton' ); const ComboBoxIO = require( 'SUN/ComboBoxIO' ); const ComboBoxItem = require( 'SUN/ComboBoxItem' ); - const ComboBoxListItemNode = require( 'SUN/ComboBoxListItemNode' ); - const Emitter = require( 'AXON/Emitter' ); - const EmitterIO = require( 'AXON/EmitterIO' ); + 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 Rectangle = require( 'SCENERY/nodes/Rectangle' ); const sun = require( 'SUN/sun' ); const Tandem = require( 'TANDEM/Tandem' ); const Vector2 = require( 'DOT/Vector2' ); - const VoidIO = require( 'TANDEM/types/VoidIO' ); // const const LIST_POSITION_VALUES = [ 'above', 'below' ]; // where the list pops up relative to the button @@ -53,7 +48,7 @@ define( require => { cornerRadius: 4, // {number} applied to list and button highlightFill: 'rgb( 245, 245, 245 )', // highlight behind items in the list - // Margins around the edges of the list when highlight is invisible. + // Margins around the edges of the button and listbox when highlight is invisible. // Highlight margins around the items in the list are set to 1/2 of these values. xMargin: 12, yMargin: 6, @@ -99,159 +94,6 @@ define( require => { // @private the display that clickToDismissListener is added to, because the scene may change, see sun#14 this.display = null; - // Compute max item dimensions - const maxItemWidth = _.maxBy( items, item => item.node.width ).node.width; - const maxItemHeight = _.maxBy( items, item => item.node.height ).node.height; - - // 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 #445 factor out ComboBoxListBox, to handle all list responsibilities - // @private the popup list - this.listBox = new Rectangle( 0, 0, listWidth, listHeight, { - cornerRadius: options.cornerRadius, - fill: options.listFill, - stroke: options.listStroke, - lineWidth: options.listLineWidth, - visible: false, - // Not instrumented for PhET-iO because the list's location isn't valid until it has been popped up. - // See https://github.com/phetsims/phet-io/issues/1102 - - // a11y - tagName: 'ul', - ariaRole: 'listbox', - groupFocusHighlight: true, - focusable: true - } ); - listParent.addChild( this.listBox ); - this.listParent = listParent; // @private - - // 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 listItemNode = event.currentTarget; // {Rectangle} - assert && assert( listItemNode instanceof ComboBoxListItemNode, 'expected a ComboBoxListItemNode' ); - - // a11y - keep this PDOM attribute in sync - this.updateActiveDescendant( listItemNode ); - - this.hideList(); - listItemNode.setHighlightVisible( false ); - event.abort(); // prevent nodes (eg, controls) behind the list from receiving the event - - property.value = listItemNode.item.value; - } - } ); - - // listener that we'll attach to each item in the list - const itemListener = { - enter( event ) { - event.currentTarget.setHighlightVisible( true ); - }, - exit( event ) { - event.currentTarget.setHighlightVisible( false ); - }, - down( event ) { - event.abort(); // prevent click-to-dismiss on the list - }, - up( event ) { - firedEmitter.emit1( event ); //TODO #405 emit1 is deprecated - } - }; - - // guard against the enter key triggering a keydown on a selected item AND then a click event on the button once - // focus moves over there. - //TODO address sun#447 - // let fromA11yEnterKey = false; - - // @private populate list with items - this.listItemNodes = []; - items.forEach( ( item, index ) => { - - // Create the list item node - 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 - } ); - listItemNode.addInputListener( itemListener ); - this.listItemNodes.push( listItemNode ); - this.listBox.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 listItemNode for disposal. - listItemNode.a11yClickListener = { - keydown: event => { - if ( KeyboardUtil.KEY_ENTER === event.domEvent.keyCode || KeyboardUtil.KEY_SPACE === event.domEvent.keyCode ) { - - //TODO address sun#447 - // fromA11yEnterKey = KeyboardUtil.KEY_ENTER === event.domEvent.keyCode; // only for the enter key - property.value = item.value; - this.hideList(); - this.button.focus(); - - // a11y - keep this PDOM attribute in sync - this.updateActiveDescendant( listItemNode ); - } - } - }; - listItemNode.addInputListener( listItemNode.a11yClickListener ); - } ); - - // @private {ComboBoxListItemNode} - a11y - // tracks which item node has focus to make it easy to focus next/previous item after keydown - this.focusedItem = null; - - // keep track of the input listener for removal - const keyDownListener = { - keydown: event => { - var domEvent = event.domEvent; - if ( domEvent.keyCode === KeyboardUtil.KEY_ESCAPE ) { - this.hideList(); - this.button.focus(); - } - else if ( domEvent.keyCode === KeyboardUtil.KEY_DOWN_ARROW || domEvent.keyCode === KeyboardUtil.KEY_UP_ARROW ) { - const direction = domEvent.keyCode === KeyboardUtil.KEY_DOWN_ARROW ? 1 : -1; - - // Get the next/previous item in the list and focus it. - for ( let i = 0; i < this.listBox.children.length; i++ ) { - if ( this.focusedItem === this.listBox.children[ i ] ) { - const nextItem = this.listBox.children[ i + direction ]; - if ( nextItem ) { - - // a11y - keep this PDOM attribute in sync - this.updateActiveDescendant( nextItem ); - - // previous item should not be focusable - this.focusedItem.focusable = false; - this.focusedItem = nextItem; - this.focusedItem.a11yFocusButton(); - break; - } - } - } - } - else if ( domEvent.keyCode === KeyboardUtil.KEY_TAB ) { - this.hideList(); - } - } - }; - this.listBox.addInputListener( keyDownListener ); - // @private button that shows the current selection this.button = new ComboBoxButton( property, items, { align: options.align, @@ -267,12 +109,30 @@ define( require => { } ); this.addChild( this.button ); - // a11y - the list is labeled by the button's label - this.listBox.addAriaLabelledbyAssociation( { - otherNode: this.button, - otherElementName: AccessiblePeer.LABEL_SIBLING, - thisElementName: AccessiblePeer.PRIMARY_SIBLING + // put optional label to left of button + if ( options.labelNode ) { + this.button.left = options.labelNode.right + options.labelXSpacing; + this.button.centerY = options.labelNode.centerY; + } + + //TODO #445 factor out ComboBoxListBox, to handle all list responsibilities + // @private the popup listbox + this.listBox = new ComboBoxListBox( property, items, this.button, + this.hideList.bind( this ), options.tandem.createTandem( 'listBox' ), { + align: options.align, + highlightFill: options.highlightFill, + xMargin: options.xMargin, + yMargin: options.yMargin, + cornerRadius: options.cornerRadius, + fill: options.listFill, + stroke: options.listStroke, + lineWidth: options.listLineWidth, + visible: false } ); + listParent.addChild( this.listBox ); + this.listParent = listParent; // @private + + this.mutate( options ); // Clicking on the button toggles visibility of the list this.button.addListener( () => { @@ -286,20 +146,13 @@ define( require => { // add the button accessibility listener this.button.addInputListener( { + + //TODO sun#445 handle this in ComboBoxListBox a11yclick: () => { //TODO sun#314 order dependency, requires that showList was called first by button listener if ( this.listBox.visible ) { - - for ( let i = 0; i < this.listBox.children.length; i++ ) { - - const listItemNode = this.listBox.children[ i ]; - - if ( property.value === listItemNode.item.value ) { - this.focusedItem = listItemNode; - this.focusedItem.a11yFocusButton(); - } - } + this.listBox.updateFocus(); } }, @@ -314,14 +167,6 @@ define( require => { } } ); - // layout - if ( options.labelNode ) { - this.button.left = options.labelNode.right + options.labelXSpacing; - this.button.centerY = options.labelNode.centerY; - } - - this.mutate( options ); - // @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 = { @@ -345,18 +190,16 @@ define( require => { // @private called by dispose this.disposeComboBox = () => { - if ( this.enabledProperty.hasListener( enabledObserver ) ) { - this.enabledProperty.unlink( enabledObserver ); + if ( this.display && this.display.hasInputListener( this.clickToDismissListener ) ) { + this.display.removeInputListener( this.clickToDismissListener ); } - // Unregister listItemNode tandems as well - for ( let i = 0; i < this.listItemNodes; i++ ) { - this.listItemNodes[ i ].dispose(); + if ( this.enabledProperty.hasListener( enabledObserver ) ) { + this.enabledProperty.unlink( enabledObserver ); } - // remove a11y listeners - this.listBox.removeInputListener( keyDownListener ); - + // dispose of subcomponents + this.listBox.dispose(); this.button.dispose(); }; @@ -395,19 +238,6 @@ define( require => { return item; }, - // TODO: sun#314 we don't likely need this anymore - // @private - update this attribute on the listBox. This changes as you interact - // with the comboBox, as well as when an item is selected. - updateActiveDescendant( listItemNode ) { - - // overwrite purposefully - this.listBox.activeDescendantAssociations = [ { - otherNode: listItemNode, - thisElementName: AccessiblePeer.PRIMARY_SIBLING, - otherElementName: AccessiblePeer.PRIMARY_SIBLING - } ]; - }, - /** * Shows the combo box list * @public @@ -438,11 +268,6 @@ define( require => { this.display.removeInputListener( this.clickToDismissListener ); } - // a11y - make sure focused list item is no longer focusable - if ( this.focusedItem ) { - this.focusedItem.focusable = false; - } - this.listBox.visible = false; this.phetioEndEvent(); diff --git a/js/ComboBoxListBox.js b/js/ComboBoxListBox.js new file mode 100644 index 00000000..686d37ea --- /dev/null +++ b/js/ComboBoxListBox.js @@ -0,0 +1,260 @@ +// Copyright 2019, University of Colorado Boulder + +/** + * The popup listbox for a ComboBox. + * + * @author Chris Malley (PixelZoom, Inc.) + */ +define( require => { + 'use strict'; + + // modules + const AccessiblePeer = require( 'SCENERY/accessibility/AccessiblePeer' ); + const ComboBoxListItemNode = require( 'SUN/ComboBoxListItemNode' ); + const Emitter = require( 'AXON/Emitter' ); + const EmitterIO = require( 'AXON/EmitterIO' ); + const KeyboardUtil = require( 'SCENERY/accessibility/KeyboardUtil' ); + const Panel = require( 'SUN/Panel' ); + const sun = require( 'SUN/sun' ); + const Tandem = require( 'TANDEM/Tandem' ); + const VBox = require( 'SCENERY/nodes/VBox' ); + const VoidIO = require( 'TANDEM/types/VoidIO' ); + + class ComboBoxListBox extends Panel { + + /** + * @param {Property} property + * @param {ComboBoxItem[]} items + * @param {ComboBoxButton} button + * @param {function} hideCallback + * @param {Tandem} tandem + * @param {Object} [options] + */ + constructor( property, items, button, hideCallback, tandem, options ) { + + options = _.extend( { + + // fill for the highlight behind items in the list + highlightFill: 'rgb( 245, 245, 245 )', + + // Panel options + xMargin: 12, + yMargin: 6, + cornerRadius: 4, + + // a11y + tagName: 'ul', + ariaRole: 'listbox', + groupFocusHighlight: true, + focusable: true + + // Not instrumented for PhET-iO because the list's location isn't valid until it has been popped up. + // See https://github.com/phetsims/phet-io/issues/1102 + + }, options ); + + // TODO sun#405 It seems it would be better to use FireListener on each ComboBoxListItemNode + const firedEmitter = new Emitter( { + tandem: tandem.createTandem( 'firedEmitter' ), + phetioType: EmitterIO( [ { name: 'event', type: VoidIO } ] ), // TODO sun#405 Should this be EventIO or DOMEventIO? + listener: event => { + + const listItemNode = event.currentTarget; + assert && assert( listItemNode instanceof ComboBoxListItemNode, 'expected a ComboBoxListItemNode' ); + + // a11y - keep this PDOM attribute in sync + this.updateActiveDescendant( listItemNode ); + + hideCallback(); + listItemNode.setHighlightVisible( false ); + event.abort(); // prevent nodes (eg, controls) behind the list from receiving the event + + property.value = listItemNode.item.value; + } + } ); + + // listener that we'll attach to each item in the list + const itemListener = { + enter( event ) { + event.currentTarget.setHighlightVisible( true ); + }, + exit( event ) { + event.currentTarget.setHighlightVisible( false ); + }, + down( event ) { + event.abort(); // prevent click-to-dismiss on the list + }, + up( event ) { + firedEmitter.emit1( event ); //TODO #405 emit1 is deprecated + } + }; + + // Compute max item dimensions + const maxItemWidth = _.maxBy( items, item => item.node.width ).node.width; + const maxItemHeight = _.maxBy( items, item => item.node.height ).node.height; + + // Uniform dimensions for all highlighted items in the list + const highlightWidth = maxItemWidth + options.xMargin; + const highlightHeight = maxItemHeight + options.yMargin; + + // @private populate list with items + const listItemNodes = []; + items.forEach( ( item, index ) => { + + // Create the list item node + 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 ? tandem.createTandem( item.tandemName ) : Tandem.optional, + a11yLabel: item.a11yLabel + } ); + listItemNodes.push( listItemNode ); + + listItemNode.addInputListener( itemListener ); + + //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 listItemNode for disposal. + listItemNode.a11yClickListener = { + keydown: event => { + if ( KeyboardUtil.KEY_ENTER === event.domEvent.keyCode || KeyboardUtil.KEY_SPACE === event.domEvent.keyCode ) { + + //TODO address sun#447 + // fromA11yEnterKey = KeyboardUtil.KEY_ENTER === event.domEvent.keyCode; // only for the enter key + property.value = item.value; + hideCallback(); + button.focus(); + + // a11y - keep this PDOM attribute in sync + this.updateActiveDescendant( listItemNode ); + } + } + }; + listItemNode.addInputListener( listItemNode.a11yClickListener ); + } ); + + const content = new VBox( { + spacing: 0, + children: listItemNodes + } ); + + // Adjust margins to account for highlight overlap + options.xMargin = options.xMargin / 2; + options.yMargin = options.yMargin / 2; + + super( content, options ); + + // a11y - the list is labeled by the button's label + this.addAriaLabelledbyAssociation( { + otherNode: button, + otherElementName: AccessiblePeer.LABEL_SIBLING, + thisElementName: AccessiblePeer.PRIMARY_SIBLING + } ); + + // @public {ComboBoxListItemNode|null} the ComboBoxListItemNode that has focus + this.focusedItemNode = null; + + //TODO #314 document + const keyDownListener = { + keydown: event => { + var domEvent = event.domEvent; + if ( domEvent.keyCode === KeyboardUtil.KEY_ESCAPE ) { + hideCallback(); + button.focus(); + } + else if ( domEvent.keyCode === KeyboardUtil.KEY_DOWN_ARROW || domEvent.keyCode === KeyboardUtil.KEY_UP_ARROW ) { + const direction = domEvent.keyCode === KeyboardUtil.KEY_DOWN_ARROW ? 1 : -1; + + // Get the next/previous listItemNode in the list and focus it. + for ( let i = 0; i < listItemNodes.length; i++ ) { + if ( this.focusedItemNode === listItemNodes[ i ] ) { + const nextListItemNode = listItemNodes[ i + direction ]; + if ( nextListItemNode ) { + + // a11y - keep this PDOM attribute in sync + this.updateActiveDescendant( nextListItemNode ); + + // previous item should not be focusable + this.focusedItemNode.focusable = false; + this.focusedItemNode = nextListItemNode; + this.focusedItemNode.a11yFocusButton(); + break; + } + } + } + } + else if ( domEvent.keyCode === KeyboardUtil.KEY_TAB ) { + hideCallback(); + } + } + }; + 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; + } + } ); + + // @private + this.disposeComboBoxListBox = () => { + for ( let i = 0; i < listItemNodes; i++ ) { + listItemNodes[ i ].dispose(); // to unregister tandem + } + }; + + // @private needed by methods + this.property = property; + this.listItemNodes = listItemNodes; + } + + /** + * @public + * @override + */ + dispose() { + this.disposeComboBoxListBox(); + super.dispose(); + } + + /** + * Updates the focus to match the currently selected value. + * @public + */ + 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(); + } + } + } + + // TODO: sun#314 we don't likely need this anymore + /** + * Updates this attribute on the listbox. + * This changes as you interact with the comboBox, as well as when an item is selected. + * @param {ComboBoxListItemNode} listItemNode + * @private + */ + updateActiveDescendant( listItemNode ) { + + // overwrite purposefully + this.activeDescendantAssociations = [ { + otherNode: listItemNode, + thisElementName: AccessiblePeer.PRIMARY_SIBLING, + otherElementName: AccessiblePeer.PRIMARY_SIBLING + } ]; + } + } + + return sun.register( 'ComboBoxListBox', ComboBoxListBox ); +} ); \ No newline at end of file From cd8a224756150b035d774eb3f24b8ac8657b97c8 Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Thu, 17 Jan 2019 11:52:24 -0700 Subject: [PATCH 2/9] TODO #445 Signed-off-by: Chris Malley --- js/ComboBoxListBox.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/ComboBoxListBox.js b/js/ComboBoxListBox.js index 686d37ea..458e9847 100644 --- a/js/ComboBoxListBox.js +++ b/js/ComboBoxListBox.js @@ -25,7 +25,7 @@ define( require => { /** * @param {Property} property * @param {ComboBoxItem[]} items - * @param {ComboBoxButton} button + * @param {ComboBoxButton} button TODO sun#445 would be nice if listbox didn't need to know about button * @param {function} hideCallback * @param {Tandem} tandem * @param {Object} [options] From 8020b30ab72685fd6e8950694fd69baba7f2e87e Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Thu, 17 Jan 2019 11:56:43 -0700 Subject: [PATCH 3/9] doc #445, instrument button #451 Signed-off-by: Chris Malley --- js/ComboBox.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/js/ComboBox.js b/js/ComboBox.js index dcc981db..332a9844 100644 --- a/js/ComboBox.js +++ b/js/ComboBox.js @@ -104,8 +104,12 @@ define( require => { yMargin: options.yMargin, baseColor: options.buttonFill, stroke: options.buttonStroke, - lineWidth: options.buttonLineWidth + lineWidth: options.buttonLineWidth, + //TODO sun#314 need to pass a11yLabel? + + // phet-io + tandem: options.tandem.createTandem( 'button ' ) } ); this.addChild( this.button ); @@ -115,7 +119,6 @@ define( require => { this.button.centerY = options.labelNode.centerY; } - //TODO #445 factor out ComboBoxListBox, to handle all list responsibilities // @private the popup listbox this.listBox = new ComboBoxListBox( property, items, this.button, this.hideList.bind( this ), options.tandem.createTandem( 'listBox' ), { From 470b495c861e66635036052f8467c47ed3e76b83 Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Thu, 17 Jan 2019 12:07:59 -0700 Subject: [PATCH 4/9] add displayOnlyProperty to ComboBoxButton, #451 Signed-off-by: Chris Malley --- js/ComboBoxButton.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/js/ComboBoxButton.js b/js/ComboBoxButton.js index 611e2f68..694379c9 100644 --- a/js/ComboBoxButton.js +++ b/js/ComboBoxButton.js @@ -11,6 +11,7 @@ define( require => { // modules const AccessiblePeer = require( 'SCENERY/accessibility/AccessiblePeer' ); + const BooleanProperty = require( 'AXON/BooleanProperty' ); const HStrut = require( 'SCENERY/nodes/HStrut' ); const Node = require( 'SCENERY/nodes/Node' ); const Path = require( 'SCENERY/nodes/Path' ); @@ -174,6 +175,18 @@ define( require => { this.disposeComboBoxButton = () => { property.unlink( propertyObserver ); }; + + // @private for use via PhET-iO, see https://github.com/phetsims/sun/issues/451 + // This is NOT reset when the Reset All button is pressed. + this.displayOnlyProperty = new BooleanProperty( false, { + tandem: options.tandem.createTandem( 'displayOnlyProperty' ), + phetioDocumentation: 'disables interaction with the ComboBox and makes it appear like value display' + } ); + this.displayOnlyProperty.link( displayOnly => { + arrow.visible = !displayOnly; + separator.visible = !displayOnly; + this.pickable = !displayOnly; + } ); } /** From 57664c92e4564b3f25804ce061a1b2284e71a4db Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Thu, 17 Jan 2019 12:13:11 -0700 Subject: [PATCH 5/9] review phetioDocumentation, #451 Signed-off-by: Chris Malley --- js/ComboBoxButton.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/js/ComboBoxButton.js b/js/ComboBoxButton.js index 694379c9..173401e3 100644 --- a/js/ComboBoxButton.js +++ b/js/ComboBoxButton.js @@ -180,7 +180,8 @@ define( require => { // This is NOT reset when the Reset All button is pressed. this.displayOnlyProperty = new BooleanProperty( false, { tandem: options.tandem.createTandem( 'displayOnlyProperty' ), - phetioDocumentation: 'disables interaction with the ComboBox and makes it appear like value display' + phetioDocumentation: + 'disables interaction with the ComboBox and makes it appear like a display that shows the current selection' } ); this.displayOnlyProperty.link( displayOnly => { arrow.visible = !displayOnly; From ed0f18af23d43fb1c827c510a7c87d4b0391a375 Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Thu, 17 Jan 2019 12:14:19 -0700 Subject: [PATCH 6/9] revise phetioDocumentation, #451 Signed-off-by: Chris Malley --- js/ComboBoxButton.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/js/ComboBoxButton.js b/js/ComboBoxButton.js index 173401e3..423821d0 100644 --- a/js/ComboBoxButton.js +++ b/js/ComboBoxButton.js @@ -180,8 +180,8 @@ define( require => { // This is NOT reset when the Reset All button is pressed. this.displayOnlyProperty = new BooleanProperty( false, { tandem: options.tandem.createTandem( 'displayOnlyProperty' ), - phetioDocumentation: - 'disables interaction with the ComboBox and makes it appear like a display that shows the current selection' + phetioDocumentation: 'disables interaction with the ComboBox button ' + + 'and makes it appear like a display that shows the current selection' } ); this.displayOnlyProperty.link( displayOnly => { arrow.visible = !displayOnly; From d8f543b3fd899e34624b0bec12eb2a7eea0f25ea Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Thu, 17 Jan 2019 13:42:13 -0700 Subject: [PATCH 7/9] better default for options.yMargin, #430 Signed-off-by: Chris Malley --- js/ComboBox.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/ComboBox.js b/js/ComboBox.js index 332a9844..81fae2ae 100644 --- a/js/ComboBox.js +++ b/js/ComboBox.js @@ -51,7 +51,7 @@ define( require => { // Margins around the edges of the button and listbox when highlight is invisible. // Highlight margins around the items in the list are set to 1/2 of these values. xMargin: 12, - yMargin: 6, + yMargin: 8, // button buttonFill: 'white', From a1b13b3226e20a0746147d933d7cf08d7148b223 Mon Sep 17 00:00:00 2001 From: Chris Malley Date: Thu, 17 Jan 2019 13:43:45 -0700 Subject: [PATCH 8/9] better default for options.yMargin, #430 Signed-off-by: Chris Malley --- js/ComboBoxButton.js | 2 +- js/ComboBoxListBox.js | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/js/ComboBoxButton.js b/js/ComboBoxButton.js index 423821d0..43d61dd0 100644 --- a/js/ComboBoxButton.js +++ b/js/ComboBoxButton.js @@ -47,7 +47,7 @@ define( require => { baseColor: 'white', buttonAppearanceStrategy: RectangularButtonView.FlatAppearanceStrategy, xMargin: 12, - yMargin: 6, + yMargin: 8, stroke: 'black', lineWidth: 1, diff --git a/js/ComboBoxListBox.js b/js/ComboBoxListBox.js index 458e9847..21052f45 100644 --- a/js/ComboBoxListBox.js +++ b/js/ComboBoxListBox.js @@ -39,8 +39,7 @@ define( require => { // Panel options xMargin: 12, - yMargin: 6, - cornerRadius: 4, + yMargin: 8, // a11y tagName: 'ul', @@ -93,7 +92,7 @@ define( require => { const maxItemWidth = _.maxBy( items, item => item.node.width ).node.width; const maxItemHeight = _.maxBy( items, item => item.node.height ).node.height; - // Uniform dimensions for all highlighted items in the list + // Uniform dimensions for all highlighted items in the list, highlight overlaps margin by 50% const highlightWidth = maxItemWidth + options.xMargin; const highlightHeight = maxItemHeight + options.yMargin; From 7e991eb9d834e5fdbc29aea5b9ab77b29370bdeb Mon Sep 17 00:00:00 2001 From: samreid Date: Thu, 17 Jan 2019 13:57:51 -0700 Subject: [PATCH 9/9] Remove whitespace from tandem strings, see https://github.com/phetsims/sun/issues/451 --- js/ComboBox.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/ComboBox.js b/js/ComboBox.js index 81fae2ae..f3f5af4e 100644 --- a/js/ComboBox.js +++ b/js/ComboBox.js @@ -109,7 +109,7 @@ define( require => { //TODO sun#314 need to pass a11yLabel? // phet-io - tandem: options.tandem.createTandem( 'button ' ) + tandem: options.tandem.createTandem( 'button' ) } ); this.addChild( this.button );