Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

why does Checkbox need a separate listener for keyboard input? #487

Closed
pixelzoom opened this issue Mar 28, 2019 · 4 comments
Closed

why does Checkbox need a separate listener for keyboard input? #487

pixelzoom opened this issue Mar 28, 2019 · 4 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 28, 2019

Noted while working on #488.

Checkbox has this code related to "firing" of its state change:

    // interactivity
    var fire = function() {
      if ( self.enabledProperty.value ) {
        var newValue = !property.value;
        toggledEmitter.emit( newValue );
      }
    };

    var checkboxButtonListener = new ButtonListener( {
      fire: fire
    } );
    this.addInputListener( checkboxButtonListener );

    // a11y - fire the listener when checkbox is clicked with keyboard or assistive technology
    var changeListener = {
      change: fire
    };
    this.addInputListener( changeListener );

Why do we need to add another listener for a11y that calls the exact same function as the ButtonListener that was already added? This should be handled by ButtonListener; it should be able to handle all types of button input, including keyboard/AT, not just pointer.

@pixelzoom
Copy link
Contributor Author

Transferring this issue from scenery-phet to sun, since Checkbox lives in sun.

@pixelzoom pixelzoom transferred this issue from phetsims/scenery-phet Mar 28, 2019
@jessegreenberg
Copy link
Contributor

This listener was added before phetsims/scenery#888, but since that is done there is no need to have a separate listener for a11y.

Response to a11y input should probably be moved to ButtonListener, but I am going to review usages of ButtonListener to make sure.

@jessegreenberg
Copy link
Contributor

ButtonListener was instrumented for a11y in #959, and this a11y listener was removed from Checkbox.

@jessegreenberg
Copy link
Contributor

Closing since the listener was removed from Checkbox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants