Add keyboard support

[ValentinH]

As discussed in #157.
Demo: http://jsfiddle.net/24gryyu5/

Should this be added by default or only if a keyboard option is set?

ARIA reference for sliders: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_slider_role .

[ValentinH]

For now, it handles both simple and range sliders. More keys needs to be supported. When a pointer is focused and a user start to update it with the mouse, then keyboard control doesn't work anymore.

[ValentinH]

I just added more keys such as page-up/down, home and end. Also, now, when a handle is clicked then it is automatically focused so we can update it after a slide using the keyboard.
I'm not sure of the best way to handle the tracking state of the slider (therz-active class that was applied during slide).

[ValentinH]

Focused handle is wrong for range slider when switching min/max with mouse.

[gbmeow]

Its working great in the fiddle - let me review the commits and see if I have any suggestions. Awesome work

[ValentinH]

Thanks! There is at least one bug remaining for range slider (my last comment). Also, it lacks comments and code cleaning.

[gbmeow]

Got it. I setup the test suite - mocha etc. on your branch [keyboard-support]
https://github.com/georgebatalinski/angularjs-slider.git
Tomorrow I will write some tests, and mock up some examples.

[ValentinH]

Great! I wanted to add tests for a long time but never got the time/motivation...
Once we'll start to have some tests, it would be nice to set up coverage reporting and continuous integration. :) The hardest part was to initiate that process, thank you ;)

[gbmeow]

I setup up coverage, as requested :) [keyboard-support]
https://github.com/georgebatalinski/angularjs-slider.git angularjs-slider/tests/coverage/

[gbmeow]

I am trying to figure out the testing strategy, events are difficult to handle in the unit tests, and aria does not seem to be updating in the tests, we may not be able to rely as heavily on unit tests.

1. Focused handle is wrong for range slider when switching min/max with mouse. - this works fine, I did not see this problem. (+ this is not required, since if the user is going to be using keyboard, they are not going to be using a mouse at the same time)
2. Is there a reason we are supporting HOME and END - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_slider_role
3. Shift and Shift-Tab is working well on the multiple handles - and that is what we want :)

Let me know if you have any suggestions on the tests, and if you have anything you would like to test specifically - plus checkout the coverage report. https://github.com/georgebatalinski/angularjs-slider/blob/keyboard-support/tests/spec/rz-slider-service-test.js

[gbmeow]

1. May you provide more detail on this one?
   I'm not sure of the best way to handle the tracking state of the slider (therz-active class that was applied during slide).

[ValentinH]

1. For me the best behaviour would be that you could start moving the slider with mouse and adjust it a little bit using keyboard. That's why I force the focus (browser state) when a handle is clicked. However, for range sliders, if you switch min/max using the mouse, the inner logic of the library is fine but the focused element (the browser state) is wrong.

On this example, I first switch min/max using keyboard: OK then with mouse: not OK.

[Edit]: This bug is fixed by last commit.

2. I found about HOME and END here: http://oaa-accessibility.org/example/32/ . Also I think they are often use in UI where keyboard are used.

Regarding the tracking state, everything is handled internally using the this.tracking property, rz-active is only for styling purpose.

[ValentinH]

Also, it would be better if you could avoid using JQuery specific function because JQuery is not required for this library. Here's a nice example about using mocked Event without JQuery: http://stackoverflow.com/a/28924708/1713469

[gbmeow]

Got it. I removed the Jquery and tested using your technique.
https://github.com/georgebatalinski/angularjs-slider.git

1. Nice work on the switching focus with mouse and keyboard :) You are right about producing the same experience for both mouse and keyboard. https://developer.mozilla.org/en-US/docs/Web/Accessibility/Keyboard-navigable_JavaScript_widgets

2. We should add tabindex="0" on the role="slider" - as per https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_slider_role
   Flow should be -> Tab to slider first and then we able to tab into the pointer

3. Quick Feedback

this.maxH[0].focus();

We should make this on function that does the focus()
e.g.

focusElement(element) {
     element.focus()
}

this.maxH[0]

0 is not descriptive - why is it 0 - is there a significance - what else is in that array?

this.tracking

I have no idea what tracking is. Should it be model? since it looks like its always - 'rzSliderModel'

if (this.options.keyboardSupport)

Since we are supporting focus - both keyboard and mouse - wondering if we need that flag. Not sure if it is worth it to disable it, since we have accessibility folks should just have it.
It is also more like FocusSupport unless there is a reason .

Here for the future - if we ever have more attributes

var attributes = ['aria-valuenow'.....]
forEach(addAttribute)

Everything else is awesome

Let me know if you want me to look at anything else

[ValentinH]

Thanks for this very complete feedback. ;)

About the tabindex="0" on the role="slider", I do it for each handle and I don't agree about the fact that we should first focus the slider and then the pointer. Only editable elements should be focusable so there is no point focusing the whole slider. Quoted from the Mozilla reference: "The slider role is assigned to the "thumb," the control that is adjusted to change the value." Here "thumb" is our pointers.

I agree that we could extract the focus logic to its own function. About the [0] syntax, it's because focus is a native DOM method so we need to access the DOM element whereas minH is the JQuery/JqLite object (https://learn.jquery.com/using-jquery-core/faq/how-do-i-pull-a-native-dom-element-from-a-jquery-object/).

For the this.tracking property, it can either be 'rzSliderModel' or 'rzSliderHigh' depending of the moving pointer. (See this line for example: https://github.com/rzajac/angularjs-slider/blob/keyboard-support/dist/rzslider.js#L1205).

For the if (this.options.keyboardSupport) condition, my plan is to enable the keyboard support by default to encourage user to provide accessible content but a user should be able to disable it if he really need.

Lastly, about the aria-attributes, I don't understand what you want to do because each attribute gets a different value...

[gbmeow]

You are fast :) The following are really small improvements.

1. In respect to the ARIA attributes, ngAria does a good job here - https://github.com/angular/angular.js/blob/master/src/ngAria/aria.js
   Given that all of them have different values - we could save some lines - like this

this.minH.attr({'aria-valuenow': 10, 'aria-valuemin': 20})

1. Just for readability then, having something like this - source: Clean Code (book Robert Cecil Martin)

var DOM_ELEMENT = 0; (constant or whatever)
this.maxH[DOM_ELEMENT]

I will create a pull request, for the test suite.

[ValentinH]

Damn the list of commits is quite weird now. A PR on a PR was maybe not a good idea! ^^ But I don't know how to do it differently (several people working on the same PR).

I'll try to fix the commits list with a big rebase.

[ValentinH]

Rebase down!

I'll modify my code with your last remarks and we should be good to go.

I don't want to use ngAria because I prefer to avoid to use any dependencies...

[gbmeow]

cool - awesome work :)

[ValentinH]

OK do you think we are good to merge?

[gbmeow]

Yes