[Slider] Add keyboard support

[felipethome]

The changes I want to propose are a little bit big, so I will divide it in two. I did a demo so you can check the final result.
I tested it against Chrome 47+, IE 10+, Firefox 43+, Safari 9.0, Android 4.4+

The changes I added in this PR:

1. Support to move the slider handle using the keyboard when it has focus (left arrow, right arrow, page down, page up, home, end)
2. When you click on the track it brings the handle under the cursor on mousedown and you can move it (no need anymore to release the mouse button and click on the handle). This solves: slider input should enter dragging state when mousedown triggered on the track. #1642
3. Support to do the same thing as above using touch events. This solves another issue: when clicking on the track using a touch device, since the corresponding touch event is not mapped the device will simulate a mouse event and then the touchend will not fire leaking the listeners and creating undesired behaviors. To reproduce this issue you just need to tap on the track instead of the handle using a touch device, and after, tap on the handle and move it. You will notice the slider will remain in active state even when you are not performing any action.
4. Add preventDefault() in the right place to avoid scroll and context menu when using touch events. Also I changed the approach of cancelling text selection using the * user-select css property. I did that because, one, the autoPrefix.set() will be removed and currently is not working which means text selection still happens, two, it makes possible to give focus to the handle when the user click directly on the track.

Changes I want to add in the next one:

1. Add a label over the slider handle following the discrete slider specification
2. Add more variables to the slider theme so the user can control all aspects of the label.

Obs: instead of following completely the specs for the label, I did a similar behaviour with the one found in the polymer paper slider as you can see in the demo.

Though I know these PR is big, I think there are valid ideas here. Hope you like.

[newoga]

Looks very cool @felipethome, I'll look at this implementation more in depth soon. 👍

[felipethome]

Thanks @newoga , looking forward for your comments.

[newoga]

I'm not the biggest fan of the current utils/key-code.js utility, but maybe we could reuse that utility for now rather than declaring these constants for now since that utility is being used in other parts of the code base. Either way, these consts could probably be declared at the top of the file rather than in the key down handler.

[felipethome]

We would need to expand a little the key-code.js, but I think it is a nice idea use it, because the key codes are constants that a lot of components may want to use, so it makes sense to me have them as a separate module. Thanks for the tip!

[alitaheri]

Don't touch these. I'm going to open an issue to discuss this further. #3348

[newoga]

@felipethome, Thanks in advance for deciding to break up your contributions in multiple PRs. It's going to make this a lot easier to review. Thanks for the demo too! 😄

I think this PR does a good job extending the functionality without increasing the API surface, so I'm really happy with that. I think there might be opportunities to improve how to define custom key event behavior to components without adding functions to the classes, though I'm not particularly hard pressed to address those problems in this PR.

@oliviertassinari @alitaheri Could you take a look at this? What do you think?

[skratchdot]

+1 on merging this in (when merge conflicts are resolved- and comments are addressed)! Not having onInput() support in the slider is a pretty big deal breaker for certain types of apps, and this PR fixes #1642. It does raise the point that onChange() in the material-ui slider would now behave like onInput() for regular type="range" html inputs (instead of the range input's onChange). I'm okay with that though, because at least it allows "instant" feedback from the slider- which an app I'm working on currently needs.

[felipethome]

@skratchdot nice point about the difference between onChange and onInput, but just to clarify the slider onChange event was already working like the onInput event. Was just when clicking on the track that this behaviour was different.

[newoga]

@felipethome Do you have time to work on this (and more) 😄? The team is slammed getting some refactoring done for 0.15.0, do you mind if I assign to you a few tasks related to this component before we merge the keyboard changes in?

If you're busy that's fine, we can either ping you when we think we'll be ready for you to get the latest or we can try to incorporate your changes in a future PR.

Let me know thanks again!

[skratchdot]

@felipethome - good point about it already working like onInput when you click the slider and drag. I temporarily forgot that while adding my +1 :) I'm just so used to clicking on the track to get the slider to change values. Let me know if you need any help rebasing to fix conflicts, or any help refactoring the refs vs findDOMNode or utils/key-code.js changes.

[felipethome]

Hi @newoga , I don't mind at all. If there is no problem that for big tasks I do it during the weekends then I have time (small ones I can do during normal days). Thanks for the invitation 😄

[newoga]

Hi @newoga , I don't mind at all. If there is no problem that for big tasks I do it during the weekends then I have time (small ones I can do during normal days). Thanks for the invitation 😄

Awesome, welcome! Almost everyone working on this is volunteer so you're not alone 😄

I know you have a lot of improvements lined up for this component and I don't want the other activity on this project to be a blocker, so I'm thinking it might be best to let you take ownership of the changes for Slider for now.

Here are a couple things that are happening:

1. We're in the process of removing mixins, including style-propable that this component uses ([Core] remove style-propable mixin from components #2852). Could you open up a new PR that removes it?

   It's pretty easy, we're replacing this.prepareStyles() with a version of prepareStyles() in muiTheme. and this.mergeStyles() with Object.assign(). In that PR you can probably remove the ReactDOM dependency that we discussed earlier. You can look at pretty much any component in the repo now for how it works such as Avatar.jsx

2. Also, do you mind reviewing the other two PRs relating to Slider: https://github.com/callemall/material-ui/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+slider

   We can try to get a hold of the authors and if they're too busy, maybe you could incorporate some of their ideas in a new or one of your PRs (if you haven't already)

After we get step 1 done, we can also probably update this PR and I'll see if I can get another collaborator to provide feedback.

Feel free to ping me on gitter if you have any questions.

[felipethome]

Ok, got it! Thanks

[alitaheri]

This can continue now 😍

[mbrookes]

Hey @felipethome - no rush, but just checking if you're happy to continue this PR?

[felipethome]

Hi @mbrookes . I have! I had a personal problem these days, but I am ready to continue now

[mbrookes]

@felipethome sorry to hear that. Please don't feel you need to worry about this if you have other things to attend to - you have done the hard work here already! If you would prefer, I can rebase on your behalf.

[felipethome]

Thank you so much! But I know you have a lot of things to do managing the project with the others. I can do it if you give me just a couple of days.

[mbrookes]

Please take your time - it will still be here! :)

[newoga]

But I know you have a lot of things to do managing the project with the others. I can do it if you give me just a couple of days.

Thanks @felipethome for being a team player and helping make this project great! As @mbrookes said, take care of yourself and take your time if you need it. We'll need you at your best for the longhaul 😄. At any time, let us know if there's anything we can do to make it easier for to stay involved (such as keeping PRs up to date)

[felipethome]

Thanks!!! That is why it is great to work in this project 😄

[felipethome]

I rebased and tested. It looks everything ok, but can you please give it a careful check? Because there were a lot of conflicts to solve.

[alitaheri]

how about:

let action = false;
switch (keycode(event)) {
  case 'page down':
  case 'left':
  case 'down':
    action = 'decrease';
    break;
  case 'page up':
  case 'right':
  case 'up':
    action = 'increase';
    break;
  case 'home':
    action = 'home';
    break;
  case 'end':
    action = 'end';
    break;
}

if (action) {

// ...

switch(action) {
// ...
}

This will reduce 10 calls to keycode to 1 and make the code a little more readable.

[felipethome]

Nice, I like it! can I just change let action = false to let action = ''? I like of having variables with the same type always

[newoga]

Or simply let action; should work too I think?

[alitaheri]

yes it would. let action; is a bit more expressive 👍

[felipethome]

Also true! (or in other words falsy 😄)

[alitaheri]

😆 😆

[alitaheri]

Thanks a lot @felipethome for rebasing this, it's a must have feature 👍

[felipethome]

Thank you too, I am incredibly glad of contributing.

[newoga]

@felipethome Thanks so much for going through all the work. This is looking great 😄. I'll take another look when I'm more rested 😴

FYI - I opened #3436 based on our conversations earlier regarding this PR and started moving to callback refs in another PR while working on a separate issue.

[newoga]

Also @felipethome, no pressure for this PR, but we're going to try to increase our test coverage for all of the components.

@nathanmarks did an amazing job improving up testing setup recently and provided an example of unit testing in Avatar.spec.js. We also recently merged a test for Badge in (#3427).

@pradel offered to help add tests for our components, but it's likely going to have to be a group effort since our coverage is only currently 27.4% in master. If you have some time to work on developing a few tests for slider.jsx as you're improving it, that'd super helpful!

[felipethome]

Nice @newoga !! I am certain they will deprecate string refs soon.

Tests will help so much, it will speed up the work a lot because every time we change something we need to worry if everything is ok. I will gladly do it!

[newoga]

Tests will help so much, it will speed up the work a lot because every time we change something we need to worry if everything is ok. I will gladly do it!

Awesome 🎉

This is the coverage report for slider based on this PR. It should help to identify testing areas: https://codecov.io/github/callemall/material-ui/src/slider.jsx?ref=63b3cfccf5e125afba77e2e0161463af8aa37893

[felipethome]

Ok! Do you want the test for this PR or in another one? Is a problem if I start to work in the test on the weekend?

[newoga]

Ok! Do you want the test for this PR or in another one? Is a problem if I start to work in the test on the weekend?

@felipethome No problem! It was more of an FYI rather than an urgent request. Increasing the test coverage for this component is something we can strive for 0.15.0, not necessarily this PR. I'd say as it relates to this PR, you can focus on the other required changes. 👍

Thanks!

[felipethome]

I think this one is done now so I can start to work on the test. What do you think? Should I squash the commits?

[alitaheri]

This looks good to me 😍

@callemall/material-ui Review, but don't merge before the alpha 👍 Thanks 👍

@felipethome The alpha will be released tomorrow, so this might take 1 or 2 to be merged. sorry 😅

[felipethome]

Nice!! Thank you too 👍 👍

[mbrookes]

Felipe, please can you squash?

[felipethome]

Of course, just did it.

[mbrookes]

@felipethome Thanks!

[newoga]

Great work @felipethome! Looking forward to what you have planned next for the component. 😄

Edit: If you're interested in working on anything else, let me know, there's lots of work to do.