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

Add keyboard navigation to NumberControl #341

Closed
zepumph opened this issue Oct 24, 2017 · 29 comments
Closed

Add keyboard navigation to NumberControl #341

zepumph opened this issue Oct 24, 2017 · 29 comments

Comments

@zepumph
Copy link
Member

zepumph commented Oct 24, 2017

In the keyboard navigation meeting today, we designed a prototype mockup for how the NumberControl component should look. We used Rutherford Scattering, Plinko Probability, and Pendulum Lab to think about what is best for the interaction. @amanda-phet could you please add the mockup to this issue so that @jessegreenberg or I can implement?

@amanda-phet
Copy link

Ruthorford scattering would need extra space to make room for the thin outer box.
screen shot 2017-10-24 at 12 10 03 pm
screen shot 2017-10-24 at 12 10 15 pm
screen shot 2017-10-24 at 12 10 23 pm

@zepumph
Copy link
Member Author

zepumph commented Oct 24, 2017

It has been added now to master.

zepumph added a commit to phetsims/scenery that referenced this issue Oct 24, 2017
zepumph added a commit to phetsims/sun that referenced this issue Oct 24, 2017
@amanda-phet
Copy link

Looks great to me!

My one concern is if the thin outer box is slightly transparent that it won't work well on sims with a black background, or colored panel (which is often beige or gray in addition to white or black). So it would be good to see it on a few more sims. If it doesn't work well in Ruthorford Scattering, maybe we opt for a thinner but fully opaque stroke instead?

@arouinfar
Copy link

Looks good @zepumph!

So it would be good to see it on a few more sims. If it doesn't work well in Ruthorford Scattering, maybe we opt for a thinner but fully opaque stroke instead?

Great suggestion @amanda-phet. I'd also like to see how this look in other sims, and possibly use a fully opaque stroke if we notice any contrast issues.

@arouinfar arouinfar removed their assignment Oct 25, 2017
@zepumph
Copy link
Member Author

zepumph commented Oct 25, 2017

I'm currently converting Rutherford to use this component for keyboard nav, here is what it will look like:
image

Perhaps not enough contrast. Once it is fully implemented you will be able to take it for a test.

zepumph added a commit to phetsims/sun that referenced this issue Oct 25, 2017
@zepumph
Copy link
Member Author

zepumph commented Oct 25, 2017

Our workaround to get this focusHighlight working yesterday isn't a good one. By making HSlider focusHighlightLayerable, we changed the bounds of all HSliders (thanks @phet-steele and @pixelzoom) for catching that). So @jessegreenberg and I will need to figure out another way to access the HSlider's "visibility" from the parent NumberControl. Marking for meeting to discuss.

@zepumph
Copy link
Member Author

zepumph commented Oct 25, 2017

This was also a problem for that node added to NumberControl. I think we can fix that though by creating a shape that stays inside the bounds of the Component.

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 25, 2017

Looks like you're doing something here related to highlighting the thumb. Keep in mind that HSlider supports a custom thumb via options.thumbNode. So whatever you do needs to handle a thumb with arbitrary shape and size.

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 25, 2017

Slack discussion, since there is no record of design decisions in this issue.

@pixelzoom Outside my wheelhouse, buy a11y question related to NumberControl. In the mockups for #341, I don’t see any examples where the arrow buttons have focus. Is that not supported, or just not shown in the mockups?

@jessegreenberg
The design decision was to not include the arrow buttons in focus order because they are redundant for keyboard interaction, the NumberControl as a whole is the element that receives keyboard focus.

@zepumph The granularity gained by the arrows for mouse/touch is already achieved in keyboard interaction with the slider

@pixelzoom ah… so you’re making the thumb move in the same steps as the buttons, not in pixels.

@jessegreenberg That is correct, thumb moves in discrete step sizes

@jessegreenberg
Copy link
Contributor

@zepumph I think we are back on for this, refactoring is mostly completed in phetsims/sun#326, waiting for code review. If you want to discuss changes let me know, or if you would rather wait until code review is complete in that issue that is understandable too. Reassigning back to you.

@zepumph
Copy link
Member Author

zepumph commented Dec 8, 2017

I got the depressed arrow look working, but I don't think it is good for master.

  1. I added and increasedEmitter and decreasedEmitter to AccessibleSlider.
  2. Then in NumberControl (which mixes AccessibleSlider) I add listeners to them like so:
    this.increasedEmitter.addListener( function() {
      rightArrowButton.buttonModel.downProperty.value = true;
      rightArrowButton.buttonModel.overProperty.value = true;

      setTimeout( function() {
        rightArrowButton.buttonModel.downProperty.value = false;
        rightArrowButton.buttonModel.overProperty.value = false;
      }, 100 );
    } );
    this.decreasedEmitter.addListener( function() {

      leftArrowButton.buttonModel.downProperty.value = true;
      leftArrowButton.buttonModel.overProperty.value = true;

      setTimeout( function() {
        leftArrowButton.buttonModel.downProperty.value = false;
        leftArrowButton.buttonModel.overProperty.value = false;
      }, 100 );

    } );

This is a major hack to me, as buttonModel is read/listen only. Also setTimeout is no good. But it worked. We could use the Joist based setTimeout, but I'm not sure about this buttonModel stuff. Honestly I'm surprised that we have never had to look into/set/listen to the visual elements of a button view before.
@jessegreenberg do you have any other recommendations about how to proceed?

@jessegreenberg
Copy link
Contributor

This feels very related to phetsims/sun#323. I can't think of a great solution off the top of my head. For the a11y reasons listed in that issue, I have a feeling we may be stuck with something like Timer.setTimeout.

One thing we could do is add something like togglePressedState() to RectangularButtonView, so the code in NumberControl would look like

    this.increasedEmitter.addListener( function() {
      arrowButton.togglePressedState();
    } );

and RectangularButtonView handles the timeout and the buttonModel. I believe that togglePressedState could be used for phetsims/sun#323.

@jessegreenberg
Copy link
Contributor

The above commit does something very similar to #341 (comment), but using phetsims/sun#323. @zepumph would you mind reviewing this commit when you return?

@zepumph
Copy link
Member Author

zepumph commented Mar 6, 2018

Looks very nice! As with phetsims/sun#318 (comment) I wonder if we don't want to make increasedEmitter more like increasedFromA11yEmitter. What do you think?

@mbarlow12
Copy link
Contributor

There may be a new bug found in phetsims/plinko-probability#104 where mouse interaction with the thumb of the HSlider doesn't shift focus, and an extra tab is required to regain focus. As this doesn't happen in RIAW or Ohm's Law (but does for ISLC sims), I think it's related to the number control implementation. @jessegreenberg should I assign myself to investigate and update?

@pixelzoom
Copy link
Contributor

There has been significant work done here, but no closure, and no progress for 31 months. So raising priority to high.

Status of this issue:

@zepumph had a question for @jessegreenberg in #341 (comment).

@mbarlow12 identified a potential related bug manifesting in plinko-probability in phetsims/plinko-probability#104, and asked @jessegreenberg how to proceed.

@terracoda
Copy link

We have an accessible number control in Gravity Force Lab regular and accessible number spinners in Gravity Force Lab: Basics. I don't think the design patterns have been documented in "Binder", but the PhET objects are accessible with all inputs.

@terracoda
Copy link

@pixelzoom, I think this issue stale and phetsims/plinko-probability#104 is also likely stale since we didn't move forward with publishing that sim with Alternative Input.

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 23, 2021

Thanks @terracoda - you're probably correct, it looks stale to me too. The subjects of @zepumph's question in #341 (comment), increasedEmitter and increasedFromA11yEmitter, don't even exist anymore. But since I'm not familiar with the work that was being done here, I'd prefer to have @jessegreenberg to wrap this up.

@jessegreenberg
Copy link
Contributor

This is stale and can now be closed. The emitters referenced in #341 (comment) do not exist anymore, and the initial a11y code discussed in this issue has been removed or factored out in #452.

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

8 participants