Wrong temp unit read out after switch

[KatieWoe]

Test device Samsung Operating System Win 11 Browser Firefox + NVDA Problem description For https://github.com/phetsims/qa/issues/1080 NVDA will read off the old unit of temperature before switching to the new one when it is changed with the drop down box Steps to reproduce

1. With NVDA, go to the first or second screen
2. Go to the temp drop down.
3. Select a different unit of temp
4. The old temp will be read out before the new one

Visuals

https://github.com/phetsims/greenhouse-effect/assets/41024075/78e2938b-1d76-418f-b19e-8e93e9d27ad9

Troubleshooting information:

!!!!! DO NOT EDIT !!!!! Name: ‪Greenhouse Effect‬ URL: https://phet-dev.colorado.edu/html/greenhouse-effect/1.3.0-dev.11/phet/greenhouse-effect_all_phet.html Version: 1.3.0-dev.11 2024-04-26 19:11:48 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:125.0) Gecko/20100101 Firefox/125.0 Language: en-US Window: 1536x731 Pixel Ratio: 1.25/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 Vendor: Mozilla (ANGLE (Intel, Intel(R) HD Graphics Direct3D11 vs_5_0 ps_5_0), or similar) Vertex: attribs: 16 varying: 30 uniform: 4096 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 32767x32767 OES_texture_float: true Dependencies JSON: {}

[jessegreenberg]

I see this happening on my machine as well. I think it is a problem in ComboBox because of this order of operations: https://github.com/phetsims/sun/blob/deca9aef0c7699701cd4a016a47e6ac704bd608f/js/ComboBoxListBox.ts#L119-L124

Focus is set, then the Property changes, and then the accessible name of the button changes here:

https://github.com/phetsims/sun/blob/ca2815c7e8e4b8fe611759dc356d4b6339e00eac/js/ComboBoxButton.ts#L268-L271

[jessegreenberg]

@jbphet and I looked at the problem together and agreed that is likely the problem. I will change ComboBox stack to manually update the name before focus is set.

[jessegreenberg]

I tried out this patch:

```patch Subject: [PATCH] Update usages of KeyboardListener and KeyboardDragListener after changes from https://github.com/phetsims/scenery/issues/1570 --- Index: js/ComboBoxButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ComboBoxButton.ts b/js/ComboBoxButton.ts --- a/js/ComboBoxButton.ts (revision 1b3d9f64a52e21f05aa8f5bc17afa9d0417fa9f2) +++ b/js/ComboBoxButton.ts (date 1715367827416) @@ -25,7 +25,6 @@ import PatternStringProperty from '../../axon/js/PatternStringProperty.js'; import Property from '../../axon/js/Property.js'; import DerivedProperty from '../../axon/js/DerivedProperty.js'; -import DynamicProperty from '../../axon/js/DynamicProperty.js'; // constants const ALIGN_VALUES = [ 'left', 'center', 'right' ] as const; @@ -61,6 +60,12 @@ // set to true to block voicing to occur upon this button's next focus event. private _blockNextVoicingFocusListener: boolean; + private readonly comboBoxVoicingNameResponsePattern: TReadOnlyProperty | string; + + private voicingPatternStringProperty: TReadOnlyProperty | null = null; + + private a11yNamePropertyMap: ComboBoxA11yNamePropertyMap; + private readonly disposeComboBoxButton: () => void; // needed by methods @@ -231,6 +236,8 @@ options.localPreferredWidthProperty.link( preferredWidthListener ); this._blockNextVoicingFocusListener = false; + this.comboBoxVoicingNameResponsePattern = options.comboBoxVoicingNameResponsePattern; + this.a11yNamePropertyMap = a11yNamePropertyMap; this.voicingFocusListener = () => { @@ -239,9 +246,6 @@ this._blockNextVoicingFocusListener = false; }; - // Keep track for disposal - let voicingPatternstringProperty: TReadOnlyProperty | null = null; - const itemProperty = new DerivedProperty( [ property ], value => { const item = _.find( items, item => item.value === value )!; assert && assert( item, `no item found for value: ${value}` ); @@ -252,10 +256,6 @@ return nodes[ items.indexOf( item ) ]; } ); - const a11yNameProperty: TReadOnlyProperty = new DynamicProperty( itemProperty, { - derive: item => a11yNamePropertyMap.get( item.value )! - } ); - // Show the corresponding item's Node on the button. nodeProperty.link( node => { // remove the node for the previous item @@ -266,20 +266,8 @@ } ); // Update the button's accessible name when the item changes. - a11yNameProperty.link( a11yName => { - // pdom - this.innerContent = a11yName; - - // 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 || '' - }, { tandem: Tandem.OPT_OUT } ); + itemProperty.link( item => { + this.setAccessibleNameFromValue( item.value ); } ); // Add aria-labelledby attribute to the button. @@ -308,7 +296,7 @@ itemProperty.dispose(); options.localPreferredWidthProperty.unlink( preferredWidthListener ); - voicingPatternstringProperty && voicingPatternstringProperty.dispose(); + this.voicingPatternStringProperty && this.voicingPatternStringProperty.dispose(); }; this.arrow = arrow; @@ -324,6 +312,25 @@ this.separatorLine.visible = !displayOnly; } + public setAccessibleNameFromValue( value: T ): void { + const newNameProperty = this.a11yNamePropertyMap.get( value )!; + assert && assert( newNameProperty !== null, 'no accessible name found for value' ); + + // pdom + this.innerContent = newNameProperty.value; + + // TODO: We should support this changing, see https://github.com/phetsims/sun/issues/865 + const patternProperty = typeof this.comboBoxVoicingNameResponsePattern === 'string' ? + new Property( this.comboBoxVoicingNameResponsePattern ) : + this.comboBoxVoicingNameResponsePattern; + + this.voicingPatternStringProperty && this.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 = this.voicingPatternStringProperty = new PatternStringProperty( patternProperty, { + value: newNameProperty.value || '' + }, { tandem: Tandem.OPT_OUT } ); + } + /** * Call to block voicing from occurring upon this button's next focus event. * For use by ComboBox. Index: js/ComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ComboBox.ts b/js/ComboBox.ts --- a/js/ComboBox.ts (revision 1b3d9f64a52e21f05aa8f5bc17afa9d0417fa9f2) +++ b/js/ComboBox.ts (date 1715367950636) @@ -312,8 +312,12 @@ this.hideListBox.bind( this ), // callback to hide the list box () => { this.button.blockNextVoicingFocusListener(); + console.log( this.button.innerContent ); this.button.focus(); }, + ( newValue: T ) => { + this.button.setAccessibleNameFromValue( newValue ); + }, this.button, options.tandem.createTandem( 'listBox' ), { align: options.align, Index: js/ComboBoxListBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ComboBoxListBox.ts b/js/ComboBoxListBox.ts --- a/js/ComboBoxListBox.ts (revision 1b3d9f64a52e21f05aa8f5bc17afa9d0417fa9f2) +++ b/js/ComboBoxListBox.ts (date 1715367914602) @@ -58,8 +58,10 @@ * @param property * @param items * @param nodes + * @param a11yNamePropertyMap * @param hideListBoxCallback - called to hide the list box * @param focusButtonCallback - called to transfer focus to the combo box's button + * @param updateAccessibleNameCallback - called to update the accessible name of the combo box * @param voiceOnSelectionNode - Node to voice the response when selecting a combo box item. * @param tandem * @param providedOptions @@ -71,6 +73,7 @@ a11yNamePropertyMap: ComboBoxA11yNamePropertyMap, hideListBoxCallback: () => void, focusButtonCallback: () => void, + updateAccessibleNameCallback: ( value: T ) => void, voiceOnSelectionNode: VoicingNode, tandem: Tandem, providedOptions?: ComboBoxListBoxOptions @@ -116,6 +119,8 @@ const oldValue = property.value; + updateAccessibleNameCallback( this.selectionOnFireAction.item.value ); + // So that something related to the ComboBox has focus before changing Property value. // See https://github.com/phetsims/sun/issues/721 focusButtonCallback(); ```

And confirmed that the next accessible name is set on the button before focus is moved to it. But it did NOT fix the problem. So, something else is going on or the accessibility tree/NVDA "virtual buffer" is stale when this happens. Since this doesn't fix it, I am not going to commit it yet.

[jessegreenberg]

I was able to reproduce the problem in this minimal test case:

<h1>Testing</h1>
<div id='div'>
  <h4 id='heading'>Temperature</h4>
  <button id='button' aria-labelledby='heading button'>degrees Celcius</button>
</div>
<ul role="listbox"
    aria-labelledby="heading"
    tabindex="-1"
    id='listbox'>
  <li tabindex="0" role="option">Kelvin</li>
  <li tabindex="0" role="option">degrees Celsius</li>
  <li tabindex="0" role="option">degrees Fahrenheit</li>
</ul>

<script>
  const listbox = document.getElementById( 'listbox' );
  const button = document.getElementById( 'button' );

  // when the list box is clicked, update the name and then focus it
  listbox.addEventListener( 'keydown', ( event ) => {
    if ( event.key === 'Enter' ) {
      button.innerText = event.target.innerText;
      button.focus();
    }
  } );
</script>

The problem is introduced by the aria-labelledby on the <button> - When the value includes its own id, the accessible name is stale.

I would like to consult with @terracoda about what to do. We should submit a bug report. But we should also decide if we should keep that aria-labelledby.

[jessegreenberg]

@terracoda confirmed that this is not a bug on MacOS VoiceOver. We recommend submitting a bug report to NVDA, and not making any additional changes to the sim. Ill open an issue in a11y-research for that and update the "screen reader bug tracking" document.

@arouinfar is that OK with you? If so, we think this can be closed.

@KatieWoe @jbphet also FYI.

[arouinfar]

Thanks @jessegreenberg , that sounds reasonable to me. Please submit a bug report to NVDA and close this issue.

[jessegreenberg]

Thanks @arouinfar - I opened https://github.com/phetsims/a11y-research/issues/196 for submitting an issue. Closing.