Can't remove accessibleInputListeners?

[jonathanolson]

It's happening in sim code (not yet committed) where I need to add/remove KeyboardDragListeners, errors any time you try to add one, since the value put in _accessibleInputListeners is actually an object filled in from the listener, not the "listener" itself.

Also note that when this is fixed, it should ideally be fixed in a way that doesn't modify the listener itself (i.e. it should be supported to add the same listener to two different nodes, right?)

As a side note, it's really weird having to create an entirely new KeyboardDragListener every time you want to change one of the deltas or its transform. I'll probably create an issue in scenery-phet for this. Also I see no way to interrupt one of those listeners.

[jessegreenberg]

addAccessibleInputListener currently returns the wrapped listener to be removed with removeAccessibleInputLisitener. So it is possible to remove the listener. The reason it wraps a listener right now is because I thought it would be convenient to automatically handle certain kinds of changes. At the moment, if a listener updates the DOMElement value attribute, it automatically updates the thisNode.inputValue. But this has caused lots of headache, I don't think we are leveraging this anywhere, and I think the wrapping should be totally removed.

it's really weird having to create an entirely new KeyboardDragListener every time you want to change one of the deltas or its transform.

This is a different issue, Ill make an issue in scenery-phet, it should be a straight forward thing to add.

[jessegreenberg]

@zepumph if you agree with #764 (comment) I can work on this.

[jessegreenberg]

@zepumph and I discussed this. We are concerned that we are trading one "gotcha" for another. If we remove the wrapping, we will have to remember set inputValue everywhere. On the other hand, the current pattern for adding/removing accessible input listeners is different from other Scenery and axon listeners.

We are not sure which is the greater evil.

[jessegreenberg]

inputValue might not have to be set very often. We discussed that for how often we have to manually set inputValue, it isn't worth having a strategy for adding/removing accessibleInput listeners that is different from what is generally used by the project.

For this reason we are leaning toward removing the wrapping function as suggested by @jonathanolson originally.

[zepumph]

Confirmed with @jonathanolson, we are ready for implementation. @jessegreenberg whoever get's to this first.

[jonathanolson]

Also I added this to RichText (I had to use removeAccessibleInputListener):

var tmp = this.addAccessibleInputListener( this.accessibleInputListener );

// "safe?" workaround for https://github.com/phetsims/scenery/issues/764
if ( this.getAccessibleInputListeners().contains( tmp ) ) {
  this.accessibleInputListener = tmp;
}

Which should remove the wrapped object and be kinda future-compatible. Probably should remove it when this is fixed.

[phet-steele]

@jonathanolson is work in this issue the cause of phetsims/scenery-phet#369?

[jonathanolson]

Yes, I should have used includes instead of contains. There's still an open question to @zepumph in phetsims/scenery-phet#369 though.

[jonathanolson]

I also have a workaround in place in ProportionalDragHandle.js (area-model-common). Both that and the RichText workaround should be removed when this is fixed.

[jessegreenberg]

Searching for listeners added to input and change events, I see usages in john-travoltage/AppendageNode sun/RadioButtonGroupMember, sun/CheckBox, sun/AquaRadioButton, and sun/AccessibleSlider. Before changing this it would be good to review to see if any of those are assuming value attribute is getting set by wrapping the listener.

[jessegreenberg]

Not set by check boxes or radio buttons, but those shouldn't need it:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox

A DOMString representing the value of the checkbox. This is never seen on the client-side, but on the server this is the value given to the data submitted with the checkbox's name.

Removing the setting in Accessibility.js and NVDA is still indicating which button is selected.

AccessibleSlider and AppendageNode are already setting inputValue.

[zepumph]

Let me know if I can be of any more assistance.

[jessegreenberg]

Thanks @zepumph, change made above so that addAccessibleInputListener is no longer wrapping the listeners in the passed in object.

It is still returning the accessibleInput, so next step will be to remove the return and update usages across the project.

[jessegreenberg]

Here is the hits for addAccessibleInputListener that need to be reviewed:

• area-model-common\js\proportional\view\ProportionalDragHandle.js
• balloons-and-static-electricity\js\balloons-and-static-electricity\view\BalloonNode.js
• balloons-and-static-electricity\js\balloons-and-static-electricity\view\TwoSceneSelectionNode.js
• circuit-construction-kit-common\js\view\CircuitElementNode.js
• circuit-construction-kit-common\js\view\VertexNode.js
• faradays-law\js\faradays-law\view\MagnetNodeWithField.js
• forces-and-motion-basics\js\motion\view\ItemNode.js
• forces-and-motion-basics\js\motion\view\ItemToolboxNode.js
• forces-and-motion-basics\js\netforce\view\KnotFocusRegion.js
• forces-and-motion-basics\js\netforce\view\PullerNode.js
• forces-and-motion-basics\js\netforce\view\PullerToolboxNode.js
• friction\js\friction\view\book\BookNode.js
• friction\js\friction\view\magnifier\MagnifierNode.js
• inverse-square-law-common\js\view\ISLCRulerNode.js
• john-travoltage\js\john-travoltage\view\AppendageNode.js
• joist\js\HomeButton.js
• joist\js\HomeScreenView.js
• joist\js\JoistButton.js
• joist\js\KeyboardHelpButton.js
• joist\js\KeyboardHelpDialog.js
• joist\js\NavigationBarScreenButton.js
• joist\js\PhetButton.js
• joist\js\PhetMenu.js
• joist\js\ScreenButton.js
• molecules-and-light\js\moleculesandlight\view\MoleculeSelectionPanel.js
• molecules-and-light\js\photon-absorption\view\EmissionRateControlSliderNode.js
• scenery\js\accessibility\Accessibility.js
• scenery\js\accessibility\AccessibilityTests.js
• scenery\js\nodes\RichText.js
• scenery-phet\js\accessibility\listeners\KeyboardDragListener.js
• scenery-phet\js\buttons\SoundToggleButton.js
• shred\js\view\ElectronShellView.js
• sun\js\AccordionBox.js
• sun\js\AquaRadioButton.js
• sun\js\Checkbox.js
• sun\js\Dialog.js
• sun\js\MenuItem.js
• sun\js\accessibility\AccessibleNumberTweaker.js
• sun\js\accessibility\AccessibleSlider.js
• sun\js\buttons\RadioButtonGroupMember.js
• sun\js\buttons\RectangularPushButton.js
• sun\js\buttons\RoundPushButton.js
• sun\js\buttons\RoundStickyToggleButton.js
• sun\js\buttons\RoundToggleButton.js
• [ ] joist\js\Dialog.js

[jessegreenberg]

The above commits get everything except ProportionalDragHandle.js and RichText.js.

[jessegreenberg]

@jonathanolson in those remaining cases, you added a check like

        if ( this.getAccessibleInputListeners().includes( tmp ) ) {
          this.accessibleInputListener = tmp;
        }

I believe those can be removed now, but don't fully understand why they were necessary as a workaround. Can you please update those as you see fit?

Once that is done, we can replace the return statement in Node.addAccessibleInputListener with

return this;

for chaining (to match Node.addInputListener).

[jonathanolson]

I believe those can be removed now, but don't fully understand why they were necessary as a workaround. Can you please update those as you see fit?

I made them compatible with the old implementation AND the new implementation that would return this. I'll remove them then assign to you.

[jonathanolson]

Removed the workarounds.

[jessegreenberg]

Excellent, thanks @jonathanolson. @zepumph would you mind reviewing changes for this issue? In particular, this commit implemented the largest change: 2e6e7bc

The rest are updating usages of addAccessibleInputListener across the project.

[zepumph]

I think this is nice, I like that you set it up so that you didn't have to change the usages in the dispose method. I like it when things work out like that. I think this is ready to close, @jessegreenberg please reopen if there are any other considerations.