Leverage PressListener for accessibility

[jessegreenberg]

Motivated by #719, we were hoping to leverage PressListener for accessibility input.

One problem that we are facing now is that a11yClick in sun/ButtonModel is manipulating the Properties that are passed to PressListener, so accessibility input and pointer input are not able to work well together. But other problems are likely to come up because the two input systems are so separate. Specifically, we are running into problems with this order of operations:

- call ButtonModel.a11yClick
  - sets button.downProperty to true
  - Change sim screens
    - calls ButtonModel's created PressListener.interrupt()
      - Removes PressListener's `_pointerListener`
  - set button.downProperty.set( false ) (delayed by Timer, because we want to update appearance of button from a single click event and we  don't have access to keyup and keydown with most types of AT.
    - will try to remove ButtonModel's PressListener._pointerListener 

For instance, we were wondering about doing something like this, where a DOM click event could leverage PressListener:

    var pressListener = pushButtonModel.createListener( options.tandem.createTandem( 'pressListener' ) );
    this.addAccessibleInputListener( {
      click: function( event ) {
        if ( self.buttonModel.enabledProperty.get() ) {

          // doesn't work for dag, and eventually this could be implemented in something like pressListener.handleA11yClick()?
          pressListener.press( new Event( self.getTrails()[ 0 ], 'click', new Pointer( self.localToGlobalPoint( self.center ).divideScalar( 2 ), false ), event ), self );
          pressListener.release();
        }
      }
    } );
    this.addInputListener( pressListener );

In addition to consolidation, another benefit of moving to PressListener is that we could handle interupt more easily.

As a workaround, we have found that if we return immediately on PressListener.release() if a pointer is not defined (because the press was triggered by an accessibility event) then we can avoid the problem.

      if ( !this.pointer ) {
        sceneryLog && sceneryLog.InputListener && sceneryLog.pop();
        return;
      }

@jonathanolson do you have any insights on whether it is OK to add this workaround to PressListener.release, and whether leveraging PressListener for accessibility as described above seems OK? @zepumph and @jessegreenberg would be happy do discuss more over voice if that is best.

[jonathanolson]

Sounds like something that would be good to discuss over voice. When would work best to discuss? (I noticed I'll need to be out for part of core hours tomorrow, but otherwise I'm pretty open).

[jessegreenberg]

We discussed this today with @jonathanolson. We agreed that it would be nice if PressListener could be used more broadly for accessibility. This would reduce a lot of code that currently exists to call listeners from pointer events and call listeners from accessibility events, which are totally separated at the moment. We talked about a couple of options:

One option would be to add a public accessiblePressListener to PressListener that could be added with addAccessibleInputListener. This would have the implementation that is currently in ButtonModel with a11yClick, a11yFocus, and a11yBlur.

Another option would be to simply add click, focus, and blur to PressListener so that it itself could be added with addAccessibleInputListener. AccessibilityUtil.addDOMEventListeners only adds listeners for a specific list of DOM events so this would be a possibility. @zepumph and @jessegreenberg prefer this option.

Going with this strategy, we could still use a timeout to update the state of the button and its visual representation, which is contradictory to phetsims/sun#371. We feel more comfortable about this because the logic for accessibility will be in the same type as PressListener so there will be more control over the state of its Properties.

[jessegreenberg]

If we decide to continue with a timeout solution, the implementation of click in PressListener might look something like

    // requires Accessibility.addAccessibleInputListener to weed out things that are not DOM events
    click: function() {

      var event = Input.createAccessibleEvent( ...boundsInformation... );

      this.press( event, syntheticPointer, ... );
      Timer.setTimeout( function( {
        self.release()
      }, delay );
    },

[zepumph]

We found the option a11yEndListener weird, because it seemed specific, and not as general as we would want to see an option.

[pixelzoom]

If you’re leveraging PressListener for a11y… Beware that PressListener has problems that are currently blocking at least 2 sims. phetsims/ph-scale#71

[zepumph]

@jessegreenberg @mbarlow12 and @zepumph worked on this today. In the above commits we added click, focus, and blur to PressListener and also to the PressListener.a11yListener object. Then we add the a11yListener as a accessible input listener whereever we add the pressListener as a normal input listener. This worked well. Basically we made PressListener.click() become the new a11yClick(), which is no longer needed in buttonModel.

From here we just need to get a11yEndListener working. Until we do the first, we will not have the correct focus behavior for the menu buttons when exiting dialogs. Because of this I'm adding that this blocks publication. @jessegreenberg and I will try to get to this in the next week or less.

[zepumph]

After this step, we will touch base with @jonathanolson again.

[jessegreenberg]

Also discussed on a brief call about moving the current Multilink into a Property on PressListener (so non-sun-buttons can use the same behavior without the boilerplate).

This part was done in the above commit. The tricky part was getting it to work with ButtonModel and its current ability to have more than one listener. We ended up with using a multilink that observes all of the looksPressedProperty's of all PressListeners that may have been created.

listenersToRemoveOnDispose has commented code related to it that does nothing right now.

The multilink solution mentioned above replaced the listenersToRemoveOnDispose, and is being disposed instead, so this part was also covered.

We discussed the "pressed" appearance of "it looks pressed if any a11y click has happened in the last X time period", but instead the implemented behavior is "X seconds after any a11y click, regardless of other a11y clicks, it becomes unpressed". Can we switch to the described one above?

We should look into this, it would be an improvement. But the weirdness described isn't happening, probably because a second click still can't initiate because of the check in canClick. Right now a user can only click at the interval of fireOnHoldInterval.

[jessegreenberg]

We discussed the "pressed" appearance of "it looks pressed if any a11y click has happened in the last X time period", but instead the implemented behavior is...

Handled in the above commit. Now fireOnHoldInterval is no longer an accurate name. It is purely for the a11yClickingProperty/looksPressedPRoperty now so we should change its name, and not use the option in ButtonModel.

[jessegreenberg]

We added fireOnHoldInterval to ButtonModel for this, and I think it can be removed now.

[jessegreenberg]

I think the above commits finish this. @zepumph could you please review 7df5663, cda0945, and phetsims/sun@38c10e6?

[jessegreenberg]

We addressed the comments in #831 (comment), @zepumph reviewed the above commits. @jonathanolson can you please do another review of the commits since #831 (comment)?

[jonathanolson]

The above commits look great, thanks!

[zepumph]

Thanks!