PushButton context response occurs before listeners are called.

[pixelzoom]

This problem was noted in Vector Addition with a push button. The context response is occurring before listeners are called.

In PushButtonModel:

else if ( fire ) {

  // Produce sound before firing, in case firing causes the disposal of this PushButtonModel
  this.fireCompleteEmitter.emit();
  this.fire();
}

The order is wrong here -- listeners are notified that fire is complete before it has been called. And fireCompleteEmitter is used to queue a button's context response. In ButtonNode:

    const speakResponseListener = () => {
      this.addAccessibleResponse( this.accessibleContextResponse );
...
    };
    this.buttonModel.fireCompleteEmitter.addListener( speakResponseListener );

So what we are seeing in Vector Addition is that the context response is occuring before the listener is called. And we're never hearing the context response because it is interrupted by responses that occur in the listener.

It looks like this problem started in ca56f17 where it was added as a workaround specific to sound. Then in 869faa9, @jessegreenberg generalized to fireCompleteEmitter.

[pixelzoom]

In Vector Addition, I had to use this workaround for the eraser button:

      this.eraserButton = new VectorAdditionEraserButton( numberOfVectorsOnGraphProperty, {
        listener: () => {
          this.interruptSubtreeInput(); // cancel all interactions for the scene
          scene.erase();

          // Add context response here instead of via options.accessibleContextResponse so that this response happens
          // last and is not interrupted by the responses caused by removing each vector.
          // See https://github.com/phetsims/vector-addition/issues/369
          this.eraserButton!.addAccessibleContextResponse( VectorAdditionStrings.a11y.eraserButton.accessibleContextResponseStringProperty.value );
        },
        ...
      } );

Whereas this would be the proper/preferred implementation:

export default class VectorAdditionEraserButton extends EraserButton {

  public constructor( numberOfVectorsOnGraphProperty: TReadOnlyProperty<number>,
                      providedOptions: VectorAdditionEraserButtonOptions ) {

    const options = optionize<VectorAdditionEraserButtonOptions, SelfOptions, EraserButtonOptions>()( {

      ...
      accessibleContextResponse: VectorAdditionStrings.a11y.eraserButton.accessibleContextResponseStringProperty
    }, providedOptions );

[jessegreenberg]

The comment in #636 (comment) indicates the order change was for CT/disposal and not related to actually hearing sound. I wonder if it is still necessary and why it isn't required for the other button models.

[jessegreenberg]

Its easy to reproduce the problem in the sun demo with

  const buttonB = new RectangularPushButton( {
    content: new Text( '--- B ---', { font: BUTTON_FONT } ),
    listener: () => {
      console.log( 'buttonB fired' );
      buttonB.dispose();
    },
    baseColor: new Color( 250, 0, 0 ),
    enabledProperty: buttonsEnabledProperty
  } );

[jessegreenberg]

In the above commit, I changed the order so that thefireCompleteEmitter emits after the fire call - but it only if not disposed. That will still address the CT errors from #636 (comment).

The result is that if the button self-disposes, we will not hear sound/description. I think that is OK because that is the behavior for all other sun buttons. @jbphet can you please review this change? Do you think we need to apply this guard to all sun fireCompleteEmitter emits?

!this.fireCompleteEmitter.isDisposed && this.fireCompleteEmitter.emit();

Or would you recommend solving it a different way?

[jbphet]

To make this easier to test, I've added a button that disposes itself immediately to the sun demo in the "PushButtons" set. Screenshot: