Skip to content

Commit

Permalink
revert ComboBox changes from #865
Browse files Browse the repository at this point in the history
  • Loading branch information
@jessegreenberg
jessegreenberg committed Nov 8, 2024
1 parent 3e5364f commit fabd4c4
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 84 deletions.
9 changes: 2 additions & 7 deletions js/CarouselComboBox.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import sharedSoundPlayers from '../../tambo/js/sharedSoundPlayers.js';
import TSoundPlayer from '../../tambo/js/TSoundPlayer.js';
import Tandem from '../../tandem/js/Tandem.js';
import Carousel, { CarouselOptions } from './Carousel.js';
import ComboBox, { ComboBoxAccessibleNamePropertyMap, ComboBoxItem } from './ComboBox.js';
import { ComboBoxItem } from './ComboBox.js';
import ComboBoxButton, { ComboBoxButtonOptions } from './ComboBoxButton.js';
import { getGroupItemNodes } from './GroupItemOptions.js';
import PageControl, { PageControlOptions } from './PageControl.js';
Expand All @@ -55,9 +55,6 @@ export type CarouselComboBoxOptions = SelfOptions & StrictOmit<ParentOptions, 'c

export default class CarouselComboBox<T> extends WidthSizable( Node ) {

// See ComboBox for a description of this property.
public readonly accessibleNamePropertyMap: ComboBoxAccessibleNamePropertyMap<T>;

private readonly disposeCarouselComboBox: () => void;

/**
Expand Down Expand Up @@ -126,8 +123,6 @@ export default class CarouselComboBox<T> extends WidthSizable( Node ) {

super();

this.accessibleNamePropertyMap = ComboBox.getAccessibleNamePropertyMap( comboBoxItems );

// Make items in the carousel have the same width and height.
const alignGroup = new AlignGroup();

Expand Down Expand Up @@ -163,7 +158,7 @@ export default class CarouselComboBox<T> extends WidthSizable( Node ) {
} );

// Pressing this button pops the carousel up and down
const button = new ComboBoxButton( property, comboBoxItems, contentNodes, this.accessibleNamePropertyMap, combineOptions<ComboBoxButtonOptions>( {
const button = new ComboBoxButton( property, comboBoxItems, contentNodes, combineOptions<ComboBoxButtonOptions>( {
listener: () => {
carouselAndPageControl.visible = !carouselAndPageControl.visible;
},
Expand Down
38 changes: 3 additions & 35 deletions js/ComboBox.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ import DerivedProperty from '../../axon/js/DerivedProperty.js';
import Multilink from '../../axon/js/Multilink.js';
import PhetioProperty from '../../axon/js/PhetioProperty.js';
import Property from '../../axon/js/Property.js';
import TinyProperty from '../../axon/js/TinyProperty.js';
import TReadOnlyProperty, { isTReadOnlyProperty } from '../../axon/js/TReadOnlyProperty.js';
import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js';
import dotRandom from '../../dot/js/dotRandom.js';
import Matrix3 from '../../dot/js/Matrix3.js';
import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js';
Expand Down Expand Up @@ -75,8 +74,6 @@ export type ComboBoxItemNoNode<T> = StrictOmit<ComboBoxItem<T>, 'createNode'>;
export type ComboBoxListPosition = typeof LIST_POSITION_VALUES[number];
export type ComboBoxAlign = typeof ALIGN_VALUES[number];

export type ComboBoxAccessibleNamePropertyMap<T> = Map<T, TReadOnlyProperty<string | null>>;

// The definition for how ComboBox sets its accessibleName and helpText in the PDOM. Forward it onto its button. See
// ComboBox.md for further style guide and documentation on the pattern.
const ACCESSIBLE_NAME_BEHAVIOR: PDOMBehaviorFunction = ( node, options, accessibleName, otherNodeCallbacks ) => {
Expand Down Expand Up @@ -162,11 +159,6 @@ export default class ComboBox<T> extends WidthSizable( Node ) {
// List of nodes created from ComboBoxItems to be displayed with their corresponding value. See ComboBoxItem.createNode().
public readonly nodes: Node[];

// A map from values to dynamic accessible names. This is required for correct operation, since we need to be able to
// modify accessible names dynamically (without requiring all ComboBox clients to do the wiring). Since we can't rely on
// Properties being passed in, we'll need to create Properties here.
public readonly accessibleNamePropertyMap: ComboBoxAccessibleNamePropertyMap<T>;

// button that shows the current selection (internal)
public button: ComboBoxButton<T>;

Expand Down Expand Up @@ -277,11 +269,10 @@ export default class ComboBox<T> extends WidthSizable( Node ) {
super();

this.nodes = nodes;
this.accessibleNamePropertyMap = ComboBox.getAccessibleNamePropertyMap( items );

this.listPosition = options.listPosition;

this.button = new ComboBoxButton( property, items, nodes, this.accessibleNamePropertyMap, {
this.button = new ComboBoxButton( property, items, nodes, {
align: options.align,
arrowDirection: ( options.listPosition === 'below' ) ? 'down' : 'up',
cornerRadius: options.cornerRadius,
Expand All @@ -307,7 +298,7 @@ export default class ComboBox<T> extends WidthSizable( Node ) {
} );
this.addChild( this.button );

this.listBox = new ComboBoxListBox( property, items, nodes, this.accessibleNamePropertyMap,
this.listBox = new ComboBoxListBox( property, items, nodes,
this.hideListBox.bind( this ), // callback to hide the list box
() => {
this.button.blockNextVoicingFocusListener();
Expand Down Expand Up @@ -550,29 +541,6 @@ export default class ComboBox<T> extends WidthSizable( Node ) {
} );
}

public static getAccessibleNamePropertyMap<T>( items: ComboBoxItem<T>[] ): ComboBoxAccessibleNamePropertyMap<T> {
const map = new Map<T, TReadOnlyProperty<string | null>>();

// Connect accessibleNamePropertyMap, creating Properties as needed.
items.forEach( item => {
let property: TReadOnlyProperty<string | null>;

if ( isTReadOnlyProperty( item.accessibleName ) ) {
property = item.accessibleName;
}
else if ( typeof item.accessibleName === 'string' ) {
property = new TinyProperty( item.accessibleName );
}
else {
property = new TinyProperty( null );
}

map.set( item.value, property );
} );

return map;
}

public static ComboBoxIO = new IOType( 'ComboBoxIO', {
valueType: ComboBox,
documentation: 'A combo box is composed of a push button and a listbox. The listbox contains items that represent ' +
Expand Down
16 changes: 5 additions & 11 deletions js/ComboBoxButton.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

import DerivedProperty from '../../axon/js/DerivedProperty.js';
import DynamicProperty from '../../axon/js/DynamicProperty.js';
import Multilink from '../../axon/js/Multilink.js';
import PatternStringProperty from '../../axon/js/PatternStringProperty.js';
import Property from '../../axon/js/Property.js';
Expand All @@ -23,7 +22,7 @@ import nullSoundPlayer from '../../tambo/js/nullSoundPlayer.js';
import Tandem from '../../tandem/js/Tandem.js';
import ButtonNode from './buttons/ButtonNode.js';
import RectangularPushButton, { RectangularPushButtonOptions } from './buttons/RectangularPushButton.js';
import ComboBox, { ComboBoxAccessibleNamePropertyMap, ComboBoxItemNoNode } from './ComboBox.js';
import ComboBox, { ComboBoxItemNoNode } from './ComboBox.js';
import sun from './sun.js';
import SunConstants from './SunConstants.js';

Expand Down Expand Up @@ -71,7 +70,6 @@ export default class ComboBoxButton<T> extends RectangularPushButton {
property: TProperty<T>,
items: ComboBoxItemNoNode<T>[],
nodes: Node[],
accessibleNamePropertyMap: ComboBoxAccessibleNamePropertyMap<T>,
providedOptions?: ComboBoxButtonOptions
) {

Expand Down Expand Up @@ -253,10 +251,6 @@ export default class ComboBoxButton<T> extends RectangularPushButton {
return nodes[ items.indexOf( item ) ];
} );

const a11yNameProperty: TReadOnlyProperty<string | null> = new DynamicProperty( itemProperty, {
derive: item => accessibleNamePropertyMap.get( item.value )!
} );

// Show the corresponding item's Node on the button.
nodeProperty.link( node => {
// remove the node for the previous item
Expand All @@ -267,19 +261,19 @@ export default class ComboBoxButton<T> extends RectangularPushButton {
} );

// Update the button's accessible name when the item changes.
a11yNameProperty.link( a11yName => {
itemProperty.link( item => {

// pdom
this.innerContent = a11yName;
this.innerContent = item.accessibleName || null;

// TODO: We should support this changing, see https://github.com/phetsims/sun/issues/865
const patternProperty = typeof options.comboBoxVoicingNameResponsePattern === 'string' ?
new Property( options.comboBoxVoicingNameResponsePattern ) :
options.comboBoxVoicingNameResponsePattern;

voicingPatternstringProperty && voicingPatternstringProperty.dispose();
// TODO: DO NOT have this getting recreated, we can simply create one up front, see https://github.com/phetsims/sun/issues/865
this.voicingNameResponse = voicingPatternstringProperty = new PatternStringProperty( patternProperty, {
value: a11yName || ''
value: item.accessibleName || ''
}, { tandem: Tandem.OPT_OUT } );
} );

Expand Down
8 changes: 2 additions & 6 deletions js/ComboBoxListBox.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import TSoundPlayer from '../../tambo/js/TSoundPlayer.js';
import EventType from '../../tandem/js/EventType.js';
import PhetioAction from '../../tandem/js/PhetioAction.js';
import Tandem from '../../tandem/js/Tandem.js';
import ComboBox, { ComboBoxAccessibleNamePropertyMap, ComboBoxItemNoNode } from './ComboBox.js';
import ComboBox, { ComboBoxItemNoNode } from './ComboBox.js';
import ComboBoxListItemNode, { ComboBoxListItemNodeOptions } from './ComboBoxListItemNode.js';
import Panel, { PanelOptions } from './Panel.js';
import sun from './sun.js';
Expand Down Expand Up @@ -67,7 +67,6 @@ export default class ComboBoxListBox<T> extends Panel {
property: TProperty<T>,
items: ComboBoxItemNoNode<T>[],
nodes: Node[],
accessibleNamePropertyMap: ComboBoxAccessibleNamePropertyMap<T>,
hideListBoxCallback: () => void,
focusButtonCallback: () => void,
voiceOnSelectionNode: VoicingNode,
Expand Down Expand Up @@ -170,11 +169,8 @@ export default class ComboBoxListBox<T> extends Panel {
const listItemNodes: ComboBoxListItemNode<T>[] = [];
items.forEach( ( item, index ) => {

const a11yNameProperty = accessibleNamePropertyMap.get( item.value )!;
assert && assert( a11yNameProperty );

// Create the list item node
const listItemNode = new ComboBoxListItemNode( item, nodes[ index ], a11yNameProperty, highlightWidthProperty, highlightHeightProperty,
const listItemNode = new ComboBoxListItemNode( item, nodes[ index ], highlightWidthProperty, highlightHeightProperty,
combineOptions<ComboBoxListItemNodeOptions>( {
align: options.align,
highlightFill: options.highlightFill,
Expand Down
36 changes: 11 additions & 25 deletions js/ComboBoxListItemNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
* @author Chris Malley (PixelZoom, Inc.)
*/

import DerivedProperty from '../../axon/js/DerivedProperty.js';
import PatternStringProperty from '../../axon/js/PatternStringProperty.js';
import Property from '../../axon/js/Property.js';
import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js';
Expand Down Expand Up @@ -51,7 +50,6 @@ export default class ComboBoxListItemNode<T> extends Voicing( Node ) {
public constructor(
item: ComboBoxItemNoNode<T>,
node: Node,
a11yNameProperty: TReadOnlyProperty<string | null>,
highlightWidthProperty: TReadOnlyProperty<number>,
highlightHeightProperty: TReadOnlyProperty<number>,
providedOptions?: ComboBoxListItemNodeOptions
Expand Down Expand Up @@ -101,6 +99,17 @@ export default class ComboBoxListItemNode<T> extends Voicing( Node ) {
options.comboBoxVoicingNameResponsePattern.includes( '{{value}}' ),
'value needs to be filled in' );

// pdom: get innerContent from the item
options.innerContent = ( item.accessibleName || null );
options.voicingObjectResponse = ( item.accessibleName || null );
const patternProperty = typeof options.comboBoxVoicingNameResponsePattern === 'string' ?
new Property( options.comboBoxVoicingNameResponsePattern ) :
options.comboBoxVoicingNameResponsePattern;
const patternStringProperty = new PatternStringProperty( patternProperty, {
value: item.accessibleName!
}, { tandem: Tandem.OPT_OUT } );
options.voicingNameResponse = patternStringProperty;

// Highlight that is shown when the pointer is over this item. This is not the a11y focus rectangle.
const highlightRectangle = new Rectangle( {
cornerRadius: options.highlightCornerRadius
Expand Down Expand Up @@ -143,27 +152,6 @@ export default class ComboBoxListItemNode<T> extends Voicing( Node ) {
itemProxy.centerY = highlightProxy.centerY;
} );

const emptyA11yNameProperty = new DerivedProperty( [ a11yNameProperty ], ( a11yName: string | null ) => {
return a11yName ? a11yName : '';
} );

// TODO: Allow comboBoxVoicingNameResponsePattern to change, see https://github.com/phetsims/sun/issues/865
const patternProperty = typeof options.comboBoxVoicingNameResponsePattern === 'string' ?
new Property( options.comboBoxVoicingNameResponsePattern ) :
options.comboBoxVoicingNameResponsePattern;
// TODO: It seems unlinks are missing, and the existing code was broken, see https://github.com/phetsims/sun/issues/865
const patternStringProperty = new PatternStringProperty( patternProperty, {
value: emptyA11yNameProperty
}, { tandem: Tandem.OPT_OUT } );
this.voicingNameResponse = patternStringProperty;

const a11yNameListener = ( a11yName: string | null ) => {
// pdom: get innerContent from the item
this.innerContent = a11yName;
this.voicingObjectResponse = a11yName;
};
a11yNameProperty.link( a11yNameListener );

this.item = item;

// pdom focus highlight is fitted to this Node's bounds, so that it doesn't overlap other items in the list box
Expand All @@ -186,8 +174,6 @@ export default class ComboBoxListItemNode<T> extends Voicing( Node ) {
this.disposeComboBoxListItemNode = () => {
pressListener.dispose();
patternStringProperty.dispose();
emptyA11yNameProperty.dispose();
a11yNameProperty.unlink( a11yNameListener );
highlightWidthProperty.unlink( highlightWidthListener );
highlightHeightProperty.unlink( highlightHeightListener );
};
Expand Down

0 comments on commit fabd4c4

Please sign in to comment.