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

Implementing Keyboard Nav #2

Open
emily-phet opened this issue Feb 9, 2018 · 25 comments
Open

Implementing Keyboard Nav #2

emily-phet opened this issue Feb 9, 2018 · 25 comments
Assignees

Comments

@emily-phet
Copy link

@ariel-phet Can you provide some guidance on how to arrange this?

In last Tuesday's keyboard navigation meeting, we discussed adding keyboard navigation to this sim (and Area Model: Decimals), ideally prior to final deployment. See list below.

It seemed like the four-way moveable object and the partition sliders would be the most challenging features for implementation, and @jessegreenberg had estimated these to be pretty straightforward to do. Is it possible @mbarlow12 and @jonathanolson could make some progress on this while @jessegreenberg is away? I'm just hoping to get this into the sims prior to deployment, no other rush from an a11y perspective.

Navigation order and element type:
Multiply Screen
Grid

  • Grid pull knob, four-way moveable object
  • Show numbers in Grid, checkbox

Panels

  • Factors box, button
  • Factor, number spinners (or pickers - whichever is correct)
  • Product box, button
  • Erase, Button
  • Grid size, radio buttons
  • Reset All, button

Partition Screen
Grid

  • Pull knob, four-way moveable object
  • Horizontal Partition, slider with changing input range to match grid size
  • (Vertical Partition, slider with changing input range to match grid size)
  • Vertical or Horizontal partition options, Radio buttons
  • Grid lines Checkbox

Panels

  • Factors box, button
  • Factor, number spinners (or pickers - whichever is correct)
  • Product box, button
  • Partial product radio buttons
  • Erase, Button
  • Grid size radio buttons
  • Reset All, button
@ariel-phet
Copy link

@emily-phet, unfortunately, @jonathanolson's time is fairly tied up at the moment working on some common code to benefit all sims.

I think it would be OK for @mbarlow12 to take a stab at things while @jessegreenberg is away. I will chat with @jonathanolson on Monday.

@mbarlow12
Copy link

@emily-phet I agree with @ariel-phet and will start looking into this tomorrow. How does this compare in priority to phetsims/john-travoltage#267?

@mbarlow12 mbarlow12 self-assigned this Feb 12, 2018
@ariel-phet
Copy link

@mbarlow12, @jonathanolson is sick today. Let's try to touch base with him when he is better just to see if he has any concerns about keyboard nav (I am pretty sure that will no affect his progress and such).

@emily-phet
Copy link
Author

@ariel-phet Just to be clear - part of the goal here is for @jonathanolson to learn how to do this for sims he works on (these and future sims), not just to have these features implemented any way possible. Since there is very little custom implementation needed - it sounded like from Jesse's estimation much could be done with common code or patterns created in previous sims - this would likely take minimal time for @jonathanolson. I just assumed he would need a knowledgeable developer to partner with to ensure progress was as efficient as possible, and to notice quickly if any major hiccups came up. Does this change any comments you have above?

@mbarlow12 Supporting implementation of keyboard navigation for Area Model: Intro and Area Model: Decimals is higher priority than phetsims/john-travoltage#267. We'll just need to sort out what that support should be.

@ariel-phet
Copy link

@emily-phet I need to speak with @jonathanolson about it...unfortunately he is also working on several common code projects that of high priority to sim production overall. Hopefully he is feeling better today and I can chat with him.

@ariel-phet
Copy link

@jonathanolson is going to collaborate with @mbarlow12 on this and check back in with me after 8 hours of work or so to judge how big of a time commitment is needed (might be done by then).

@mbarlow12 please get in touch with @jonathanolson to arrange some collaboration times.

@mbarlow12
Copy link

Will do @ariel-phet.

@mbarlow12
Copy link

mbarlow12 commented Feb 21, 2018

@jonathanolson worked through the keyboard navigation, and everything's looking pretty solid. Order is also correct.

We should review in the next a11y meeting:

  • pixel polishing for new focus highlights
  • for partition screen, up/down arrows are inverted in their manipulation of the vertical partition (blue) line
  • the calculation display wasn't included in the ordering in Implementing Keyboard Nav #2 (comment), so we added it after the partial product radio buttons

We can also discuss any other issues that pop up on Tues. JO will be joining as well.

@jonathanolson
Copy link
Contributor

@amanda-phet
Copy link
Contributor

Looking good!

I would tweak the order a bit. We moved the partition radio buttons to the panel with partial products to intentionally change the order someone might interact with the sim, so the tab navigation order should follow the progression you visually see on the screen a bit more closely.

So, I think the order should be:

green handle, partition line, factors, product, partial products, partition line, calculation, erase.

The triangular focus highlight looks fine to me, but I was surprised it was a triangle. Then I noticed the pickers were rounded a bit, so I guess that's the direction we're going. I think it would be fine to only use rectangles and circles for focus highlights, personally.

jonathanolson added a commit to phetsims/area-model-common that referenced this issue Feb 23, 2018
@jonathanolson
Copy link
Contributor

The triangular focus highlight looks fine to me, but I was surprised it was a triangle. Then I noticed the pickers were rounded a bit, so I guess that's the direction we're going. I think it would be fine to only use rectangles and circles for focus highlights, personally.

I don't know what the highlights are supposed to look like. Let me know if it should change, but I assumed that the highlights would typically be the "offset" shape.

@jonathanolson
Copy link
Contributor

I would tweak the order a bit.

Implemented in master, can you verify?

@amanda-phet
Copy link
Contributor

Thanks, that seems like an improvement. We can discuss more at tomorrow's keyboard nav meeting!

@amanda-phet
Copy link
Contributor

Discussion at 2/27/18 meeting:

Modified the order slightly so the erase button came just after interacting with the board/model, and the checkboxes to show numbers and gridlines would come after all other controls. @jonathanolson made this change during the meeting and everyone was happy with it.

Final work to be done seems just to adjust the horizontal partition line so that up and down move it accordingly.

@jonathanolson
Copy link
Contributor

Final work to be done seems just to adjust the horizontal partition line so that up and down move it accordingly.

Presumably this would be something that would be available for every "vertical" slider also? Would the left-right buttons work as they do now, or would those also be swapped?

@mbarlow12
Copy link

mbarlow12 commented Mar 2, 2018

@jonathanolson I think the vertical interaction will only apply to this situation; however, the larger issue seems to be that the origin is at the top/left. Thus 'increasing' the value of the slider's <input> element moves the partition line away from its zero position.

Inverting this behavior (i.e. 'up' corresponds to decreasing the slider's value) could be confusing to users that rely on a screen reader or anyone accustomed to the typically features of an <input type="range"> element. @terracoda and @jessegreenberg might have more insight, but I think it's something we may want to consider.

@jessegreenberg
Copy link
Contributor

Thus 'increasing' the value of the slider's element moves the partition line away from its zero position.

I agree this is the cause. Could an intermediary value Property for the other partition be used in this case so that gets incremented/decremented with up/down arrow keys respectively?

Would the left-right buttons work as they do now, or would those also be swapped?

I think those should also be swapped. For it to behave like an accessible slider, the left and down arrow keys should decrement while the up and right arrow keys increment.

@mbarlow12
Copy link

From today's a11y meeting:

  • set AccessibleSlider's range to min: -[maxValue], max: 0
  • map input/change events to the absolute value for model updates

@jessegreenberg do you think an intermediate will still be necessary or just some custom functionality in the AccessibleSlider?

@emily-phet emily-phet removed their assignment Mar 12, 2018
@jessegreenberg
Copy link
Contributor

@mbarlow12 I would still recommend an intermediate Property, I don't think inverting values is generally a feature that should be added to AccessibleSlider.js.

@jonathanolson
Copy link
Contributor

@mbarlow12 I would still recommend an intermediate Property, I don't think inverting values is generally a feature that should be added to AccessibleSlider.js.

So it will be fine in the future if a screen reader reports negative values?

@jessegreenberg
Copy link
Contributor

Yes that is OK. The reported value can also be completely modified with text, which I assume will be used in this case. The readout might be something like

"Partition changed: 3 rows on top, 2 rows on bottom"

@jonathanolson jonathanolson self-assigned this Mar 13, 2018
@jonathanolson
Copy link
Contributor

Assigning myself, I'll apply the change.

jonathanolson added a commit to phetsims/area-model-common that referenced this issue Mar 14, 2018
@jonathanolson
Copy link
Contributor

Implemented.

Should I put accessibility: true (or !platform.mobileSafari like some others) in the sim options, or should this stay behind a query parameter?

Should I mark the sim's package.json as "accessible", and/or should an a11y view be created?

@jessegreenberg
Copy link
Contributor

Should I put accessibility: true (or !platform.mobileSafari like some others)

Sorry, I need to double check where we are on this.

Should I mark the sim's package.json as "accessible", and/or should an a11y view be created?

Both would be great @jonathanolson, thanks! Let me know if you have concerns.

jonathanolson added a commit that referenced this issue Mar 15, 2018
jonathanolson added a commit that referenced this issue Mar 15, 2018
@jonathanolson
Copy link
Contributor

Added view and flag.

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