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

NumberSpinner should behave like NumberPicker for keyboard navigation #318

Closed
jessegreenberg opened this issue Sep 26, 2017 · 7 comments
Closed
Assignees

Comments

@jessegreenberg
Copy link
Contributor

In NumberPicker, the focus highlight goes around the buttons and the arrow keys can be used to increment/decrement the values. NumberSpinner should behave the same way. The focus highlight should go around the arrow buttons and not include the readout.

@zepumph
Copy link
Member

zepumph commented Oct 24, 2017

I worked on this, basically copying the implementation from NumberPicker. There the component is an input of type "number." I think that this works pretty well. A few comments:

  • The original assignment was to have the focus highlight surround just the two arrows, but why not the whole component? That is idiomatic to other implementations, see the example here:
    image
    To me that looks pretty nice

  • Another argument against surround just the arrows, their layout can be more custom than just the RPAL implementation, see expression exchange here:
    image

Surrounding the whole component makes much more sense here.

  • Final thought. The current implementation only supports the vertical arrow keys to adjust the value, but in expression exchange the arrows are horizontal, this is awkward. I suggest that we support both vertical and horizontal always, although that may be custom, and unnormal interaction for AT users, I think it will help visual keyboard users.

@terracoda @emily-phet @jessegreenberg what do you think?

@terracoda
Copy link
Contributor

@zepumph, typically with a input type number, the actual box is editable, so the Left and Right Arrows are reserved to move within the number to edit it. The Up and Down Arrows are for increasing and decreasing.

Since we don't have an editable box, and in some cases the arrows display horizontally, it would make sense in our case to enable all the arrow keys for for increasing and decreasing changing the value of the number. I think this would be a custom implementation based on what is in the ARIA 1.1 Specification. It seems silly, however, that it is not a default option in the case that the actual number box is not editable.

It works nicely in RPAL.

@zepumph
Copy link
Member

zepumph commented Oct 26, 2017

I can't speak to phet's relationship to editable components, but I feel like we just don't use them, not sure why. Perhaps @ariel-phet could speak to that.

As for the "custom" interaction of adapting the left/right arrows to do the same as the up/down ones. I think that would be a good improvement.

@zepumph
Copy link
Member

zepumph commented Nov 2, 2017

@jessegreenberg please review the below commit. I had to use a different event (keydown) since the input event isn't called with left/right with a number input. How does that look?

@jessegreenberg
Copy link
Contributor Author

@zepumph, @mbarlow12 and I decided to try the mixin pattern we used in HSlider to add accessibility to NumberPicker and NumberSpinner with the same implementation. I had forgotten about this issue, but would you mind reviewing AccessibleNumberTweaker since you took the lead on this issue? The highlight surrounds the whole input as you suggest in #318 (comment).

@zepumph
Copy link
Member

zepumph commented Mar 6, 2018

A few thoughts:

  • Why are the options timerDelay and timerInterval not prefixed with "a11y"? They seem keyboard specific, and specific options to this mixin.
  • incrementDownEmitter and decrementDownEmitter are a11y specific, but they aren't named as such, which makes
    this.incrementDownEmitter.addListener( function( isDown ) { upStateProperty.value = ( isDown ? 'down' : 'up' ); } );`
    in NumberPicker:394 a bit confusing since really it is just passing the up state from the keyboard to the component.

This is really nice! It makes the usage so streamlined and simple. Well done @jessegreenberg and @mbarlow12.

@jessegreenberg
Copy link
Contributor Author

I renamed the options and Emitters as recommended in #318 (comment). Closing.

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

No branches or pull requests

7 participants