Basic slider accessibility #282

byCedric · 2017-06-15T21:15:01Z

As promptly discussed in issue #131, I created some basic accessibility for the Slider. I implemented the keyboard behaviour as my personal best guess of how it should work.

Note, this is a work in progress but I wanted to share my changes already.

benjycui · 2017-06-19T09:22:23Z

Oops, CI failed

paranoidjk · 2017-07-03T12:36:19Z

@byCedric
Thanks for your pr, Wonderful job 👍
Could you please rebase master to solve the conflict and fix the ci, then i will do a code review.

byCedric · 2017-07-03T13:01:51Z

@paranoidjk thanks! Yeah, of course, I have some time late tonight. I think I can have this done tomorrow morning, around 08:00 (GMT) or something like that.

coveralls · 2017-07-04T07:32:20Z

Coverage decreased (-5.8%) to 43.558% when pulling d99adb9 on byCedric:feature/accessibility into 9c0ea63 on react-component:master.

byCedric · 2017-07-04T07:35:11Z

@paranoidjk Ok, so I rebased and added the tabindex to the handlers within the snapshots. Can I help with something for the final merge?

benjycui · 2017-07-05T02:13:07Z

👍

It will be better if you can add unit test for this new feature.

paranoidjk · 2017-07-07T03:41:47Z

src/utils.js

+
+    case keyCode.END: return (value, props) => props.max;
+    case keyCode.HOME: return (value, props) => props.min;
+    case keyCode.PAGE_UP: return (value, props) => value + props.step * 2;


why * 2 ？

as you described in #131 (comment), peg_up should be equal with up

For the up and down behaviours I tend to follow the Polymer approach. Mainly because you are "upping" or "downing" the current value.

What I tried to explain was that I like how Polymer's approach had the same type of behaviour with both up and pageup keys. Both "up" keys increased the current value. OAA's behaviour was the same for up and pagedown, which I didn't like because of mixed "up" and "down" keys.

Later on I tried to reason that OAA has a point about the increased value jump through pageup and pagedown. For small sliders it makes no different, but if you have a big one you would probably want to go faster through the steps.

OAA Accessibility has a point about the page up and page down behaviours. I mean, left/down and up/right only go by 1 step. For small sliders this is fine, but for large ones not. Also if an user actually uses the page up/page down key, they probably expect something more then just a mimick of left and right.

I'm open for both approaches. Either increase the step when using the page keys or just copy the same behaviour from the normal up/down keys. It's up to you guys.

Ok, i understand. But please make sure you update the api doc in readme correctly.

paranoidjk · 2017-07-07T03:48:50Z

src/Slider.jsx

+    if (valueMutator) {
+      utils.pauseEvent(e);
+      const state = this.state;
+      const oldValue = state.value;


no need to define a variable state ?

const { value: oldValue } = this.state;

If it was up to me, I would have definitely used these destructuring assignment. But I didn't want to introduce a new code styling. That's why I sticked as close as possible to the existing one. I think I based this part of the code on this code.

😅 no need to obey current code style, just use what you think is better.

It's just because that we do not have enough time to refactor this component right now, and PR is welcome ~

Haha, good to know! Let's keep this for now unchanged. If I have some time again after this PR, I will try to create one for some refactoring 😄

paranoidjk · 2017-07-07T03:50:51Z

src/common/createSlider.jsx

+      if (utils.isEventFromHandle(e, this.handlesRefs)) {
+        const handlePosition = utils.getHandleCenterPosition(isVertical, e.target);
+
+        this.dragOffset = 0;


Why set dragOffset to zero?

I think I did this to "reset" mouse/touch behaviours when combining keyboard with these. But I'm not sure if this is even necessary. Just tested with and without this and I had the same outcome... I can remove it if you want?

I think we can keep it, since this.dragOffset = 0; is to restore drag status.

paranoidjk · 2017-07-08T12:27:49Z

@byCedric
We almost done. Please update the api doc on readme, and add some test case, you can take a look at the existing testcase in /tests folder.

byCedric · 2017-07-08T17:54:11Z

Yeah, sure! I will try to start on that tonight!

benjycui · 2017-07-25T09:14:55Z

ping~ @byCedric

byCedric · 2017-07-26T13:18:16Z

Hey guys, sorry it took a while to respond. I tried to write some tests for the slider first, but quickly encountered some errors. For example, this is the most simple test I've written.

  it('increments the value when key "up" was pressed', () => {
    const wrapper = mount(<Slider value={50} step={1} />);
    const handler = wrapper.find('.rc-slider-handle');

    handler.simulate('focus');
    handler.simulate('keyDown', { keyCode: keyCode.UP });

    expect(wrapper.state('value')).toBe(51);
  });

I think this should work right? But It gives me this error.

$ npm test

> rc-slider@8.1.5 test /absolute/path/to/slider
> jest

 FAIL  tests/Slider.test.js
  ● Slider › increments the value when up key was pressed

    TypeError: Cannot read property 'toFixed' of undefined
      
      at Object.ensureValuePrecision (src/utils.js:75:2549)
      at ComponentEnhancer.trimAlignValue (src/Slider.jsx:127:20)
      at ComponentEnhancer.calcValueByPos (src/common/createSlider.jsx:224:1870)
      at ComponentEnhancer.onStart (src/Slider.jsx:85:1605)
      at ComponentEnhancer._this.onFocus (src/common/createSlider.jsx:130:45)
      at Object.invokeGuardedCallback [as invokeGuardedCallbackWithCatch] (node_modules/react-dom/lib/ReactErrorUtils.js:26:5)
      at executeDispatch (node_modules/react-dom/lib/EventPluginUtils.js:83:21)
      at Object.executeDispatchesInOrder (node_modules/react-dom/lib/EventPluginUtils.js:108:5)
      at executeDispatchesAndRelease (node_modules/react-dom/lib/EventPluginHub.js:43:22)
      at executeDispatchesAndReleaseSimulated (node_modules/react-dom/lib/EventPluginHub.js:51:10)
      at forEachAccumulated (node_modules/react-dom/lib/forEachAccumulated.js:26:8)
      at Object.processEventQueue (node_modules/react-dom/lib/EventPluginHub.js:252:7)
      at node_modules/react-dom/lib/ReactTestUtils.js:351:22
      at ReactDefaultBatchingStrategyTransaction.perform (node_modules/react-dom/lib/Transaction.js:143:20)
      at Object.batchedUpdates (node_modules/react-dom/lib/ReactDefaultBatchingStrategy.js:62:26)
      at Object.batchedUpdates (node_modules/react-dom/lib/ReactUpdates.js:97:27)
      at node_modules/react-dom/lib/ReactTestUtils.js:349:18
      at ReactWrapper.<anonymous> (node_modules/enzyme/build/ReactWrapper.js:776:11)
      at ReactWrapper.single (node_modules/enzyme/build/ReactWrapper.js:1421:25)
      at ReactWrapper.simulate (node_modules/enzyme/build/ReactWrapper.js:769:14)
      at Object.<anonymous> (tests/Slider.test.js:29:13)
      at handle (node_modules/worker-farm/lib/child/index.js:41:8)
      at process.<anonymous> (node_modules/worker-farm/lib/child/index.js:47:3)
      at emitTwo (events.js:125:13)
      at process.emit (events.js:213:7)
      at emit (internal/child_process.js:768:12)
      at _combinedTickCallback (internal/process/next_tick.js:141:11)
      at process._tickCallback (internal/process/next_tick.js:180:9)

 PASS  tests/common/createSlider.test.js
 PASS  tests/common/marks.test.js
 PASS  tests/Range.test.js

Test Suites: 1 failed, 3 passed, 4 total
Tests:       1 failed, 1 skipped, 22 passed, 24 total
Snapshots:   3 passed, 3 total
Time:        5.482s
Ran all test suites.
npm ERR! Test failed.  See above for more details.

Any idea how to fix this? Or do you need actual code for this? @benjycui

benjycui · 2017-07-27T01:29:55Z

@byCedric can you re-produce this with master branch?

paranoidjk · 2017-07-27T02:25:02Z

@byCedric

Please push your test code to this pr, i'd like to help you figure out the problem.

And please make sure you do have rebased master.

byCedric · 2017-07-27T08:04:40Z

Yeah, will do tonight. I'll start with the tests for the slider. It looks like I'm doing something wrong with trimming the value for keyboard events. But as I said, will rebase and add the slider tests tonight! 👍

byCedric · 2017-07-27T19:22:43Z

Ok guys, I did a final effort in debugging what goes wrong. I found out that when focusing the handler directly, as coded in the example, the slider thinks he is exactly 0px wide. The strange thing is that when I focus the wrapper first, and then fire a KeyboardEvent on the handler, everything works. This looks like it needs some extra caring and love, but for now I'm not going in to deep.

Right now I'm patching up the other tests, I'll commit it soon afterwards!

coveralls · 2017-07-27T19:31:13Z

Coverage increased (+4.05%) to 53.374% when pulling b8155a4 on byCedric:feature/accessibility into 9c0ea63 on react-component:master.

coveralls · 2017-07-27T19:35:23Z

Coverage increased (+4.05%) to 53.374% when pulling 6a448fd on byCedric:feature/accessibility into 9c0ea63 on react-component:master.

byCedric · 2017-07-27T19:35:32Z

So what do you guys think about this, any tips/tricks I should do? I think there could be some improvements within the tests by e.g. grouping them. Or even automate some repeating tests by iterations, instead of hardcoding it. Anyways, I'm still a bit puzzled why I need to use both the wrapper and handler for focus and keyboard event. Maybe you can shine your lights on that one @paranoidjk?

byCedric · 2017-07-27T19:37:15Z

Oh wait, I see I changed a line I shouldn't have.

coveralls · 2017-07-27T19:43:29Z

Coverage increased (+4.05%) to 53.374% when pulling c6fde1d on byCedric:feature/accessibility into 9c0ea63 on react-component:master.

paranoidjk · 2017-07-28T03:37:11Z

Thanks for @byCedric 👍 Released rc-slider@8.3.0

benjycui · 2017-07-28T03:40:21Z

@paranoidjk PR to antd is welcomed.

paranoidjk · 2017-07-28T03:41:58Z

@benjycui What do you mean? antd should be no need to do any change, right?

benjycui · 2017-07-28T05:09:32Z

https://github.com/ant-design/ant-design/blob/master/package.json#L64

antd had locked minor

react-component/slider#282

paranoidjk added the Feature label Jul 3, 2017

paranoidjk self-assigned this Jul 4, 2017

paranoidjk reviewed Jul 7, 2017

View reviewed changes

byCedric added 6 commits July 27, 2017 20:53

Makes the handle a focusable element

f360e88

Adds keyboard value mutators for all supported key behaviours

a70d808

Adds the keyboard event handlers to the createSlider method

e5d3cb2

Adds the onKeyboard event handler in both slider and range

4f4d6c6

Updates the snapshot to allow tabindex on the handler

20270fa

Updates the Range snapshot to allow tabindex in tests

ba9af74

Adds tests for all keyboard events for the slider

6a448fd

Reverts defaultValue property back to value within the slider tests

c6fde1d

paranoidjk merged commit 59a5b15 into react-component:master Jul 28, 2017

paranoidjk added a commit to ant-design/ant-design that referenced this pull request Jul 28, 2017

feat: upgrade rc-slider, support keyboard accessibility

ef339a3

react-component/slider#282

paranoidjk mentioned this pull request Jul 28, 2017

Not accessible #131

Closed

yesmeck pushed a commit to ant-design/ant-design that referenced this pull request Jul 30, 2017

feat: upgrade rc-slider, support keyboard accessibility

6b7757e

react-component/slider#282

byCedric deleted the feature/accessibility branch July 30, 2017 12:10

byCedric mentioned this pull request Aug 4, 2017

Wrong handle is being focused, when crossing #298

Open

Robin-front mentioned this pull request Oct 19, 2017

Cannot control slider via Keyboard #335

Closed

paranoidjk mentioned this pull request Nov 3, 2017

Can't set tabIndex on handle #353

Closed

renovate bot mentioned this pull request Mar 21, 2018

Update rc-slider to ^8.0.0-abandoned sbardian/DebtTrackerAPI#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic slider accessibility #282

Basic slider accessibility #282

byCedric commented Jun 15, 2017

benjycui commented Jun 19, 2017

paranoidjk commented Jul 3, 2017

byCedric commented Jul 3, 2017

coveralls commented Jul 4, 2017

byCedric commented Jul 4, 2017

benjycui commented Jul 5, 2017

paranoidjk Jul 7, 2017

byCedric Jul 7, 2017

paranoidjk Jul 8, 2017

paranoidjk Jul 7, 2017

byCedric Jul 7, 2017

paranoidjk Jul 8, 2017

byCedric Jul 8, 2017

paranoidjk Jul 7, 2017

byCedric Jul 7, 2017

paranoidjk Jul 8, 2017

paranoidjk commented Jul 8, 2017

byCedric commented Jul 8, 2017

benjycui commented Jul 25, 2017

byCedric commented Jul 26, 2017 •

edited

Loading

benjycui commented Jul 27, 2017

paranoidjk commented Jul 27, 2017

byCedric commented Jul 27, 2017

byCedric commented Jul 27, 2017

coveralls commented Jul 27, 2017

coveralls commented Jul 27, 2017

byCedric commented Jul 27, 2017 •

edited

Loading

byCedric commented Jul 27, 2017

coveralls commented Jul 27, 2017

paranoidjk commented Jul 28, 2017

benjycui commented Jul 28, 2017

paranoidjk commented Jul 28, 2017

benjycui commented Jul 28, 2017

Basic slider accessibility #282

Basic slider accessibility #282

Conversation

byCedric commented Jun 15, 2017

benjycui commented Jun 19, 2017

paranoidjk commented Jul 3, 2017

byCedric commented Jul 3, 2017

coveralls commented Jul 4, 2017

byCedric commented Jul 4, 2017

benjycui commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paranoidjk commented Jul 8, 2017

byCedric commented Jul 8, 2017

benjycui commented Jul 25, 2017

byCedric commented Jul 26, 2017 • edited Loading

benjycui commented Jul 27, 2017

paranoidjk commented Jul 27, 2017

byCedric commented Jul 27, 2017

byCedric commented Jul 27, 2017

coveralls commented Jul 27, 2017

coveralls commented Jul 27, 2017

byCedric commented Jul 27, 2017 • edited Loading

byCedric commented Jul 27, 2017

coveralls commented Jul 27, 2017

paranoidjk commented Jul 28, 2017

benjycui commented Jul 28, 2017

paranoidjk commented Jul 28, 2017

benjycui commented Jul 28, 2017

byCedric commented Jul 26, 2017 •

edited

Loading

byCedric commented Jul 27, 2017 •

edited

Loading