Keyboard events and ARIA markup added to speed control #2381

jmclaus · 2014-01-30T20:50:47Z

Addresses BLD-402.

jmclaus · 2014-01-31T16:38:06Z

@polesye @valera-rozuvan DO NOT MERGE. Please review the new functionality of the speed menu button, I made it adhere to the following standards: http://www.w3.org/TR/wai-aria-practices/#menubutton. I will then fix the tests that are failing.

jmclaus · 2014-02-03T07:54:54Z

@polesye @valera-rozuvan By the way, I did not add aria-haspopup="true" to the speed button on purpose, it doesn't work well with Voice Over. If added, it will also announce the position of the video and its total length. Will investigate why. For the moment, it is not an issue as the volume control doesn't have that ARIA markup either.

polesye · 2014-02-03T16:41:45Z

common/lib/xmodule/xmodule/js/spec/video/video_speed_control_spec.js

+                    speedControl = $('div.speeds'),
+                    speedEntries = $('div.speeds>a'),
+                    firstSpeedEntry = speedEntries.first(),
+                    secondSpeedEntry = $(speedEntries.eq(1)),


$(speedEntries.eq(1)) => speedEntries.eq(1)

jmclaus · 2014-02-03T18:21:07Z

@polesye @valera-rozuvan Fixed tests. Will add a couple of missing cases (ENTER & ESC on speed entries) tomorrow.

valera-rozuvan · 2014-02-04T07:26:12Z

common/lib/xmodule/xmodule/js/spec/video/video_speed_control_spec.js

+
+                // Get previous element in array or cyles back to the last if it
+                // is the first.
+                previousSpeed = function(index) {


@jmclaus Why is this function declared globally? Use the var keyword...

@valera-rozuvan Done. (Why is this function declared globally? Use the var keyword)

polesye · 2014-02-04T08:03:34Z

My thoughts: let's avoid navigation via TAB through the menu items.

When the container or its active descendant has focus, the user may navigate through the container by pressing additional keys, such as the arrow keys, to change the currently active descendant. Any additional press of the main navigation key (generally the TAB key) will move out of the container to the next widget. See w3.org

What do you think about following behaviors:

If focus is on the hint button:

Enter: Open or close hint popup. Select last focused hint item if opening
Space: Open or close hint popup. Select last focused hint item if opening
Esc: close hint popup
Tab: focus goes to the next hint button (Volume button)
Shift + Tab: focus goes to the previous hint button (Play/Pause button)

If focus is on a hint item:

Left arrow: Select previous hint item
Up arrow: Select previous hint item
Right arrow: Select next hint item
Down arrow: Select next hint item
Esc: close hint popup
Tab: focus goes to the next hint button (Volume button)
Shift + Tab: focus goes to the previous hint button (Play/Pause button)

jmclaus · 2014-02-04T08:36:02Z

@polesye I was modeling the TAB behavior on menu items after this:

http://adobe-accessibility.github.io/Accessible-Mega-Menu/

But I agree, it should rather adhere to the W3 standards. It's easy to change in the case of the speed control. And we should include this behavior in the upcoming plugin.

I'm not sure of the behavior of:

the UP and DOWN arrows, the focus should stay on menu control:
(in http://www.w3.org/TR/wai-aria-practices/#menubutton)
Space or Enter - With focus on the button pressing Space or Enter will toggle the display of the drop-down menu. Focus remains on the button.
the LEFT and RIGHT arrows: I think they should rather behave as the SHIFT + TAB and TAB ie:
LEFT arrow: focus goes to the previous hint button (Play/Pause button)
RIGHT arrow: focus goes to the next hint button (Volume button)

This is also the standard behavior in native menus.

polesye · 2014-02-04T09:05:22Z

I think we need to implement [Menu Button(http://www.w3.org/TR/wai-aria-practices/#menubutton) behavior in this PR and no need to support LEFT/RIGHT arrows like in Menu.

I have updated behavior:
If focus is on the menu button:

Enter: Open or close menu. Select the first menu item if opening
Space: Open or close menu. Select the first menu item if opening
Up arrow: Open menu and move focus onto the first menu item.
Esc: Close menu
Tab: Move focus to the next menu button (Volume button)
Shift + Tab: Move focus to the previous menu button (Play/Pause button)

If focus is on a menu item:

Up arrow: Select previous menu item
Down arrow: Select next menu item
Esc: Close menu popup
Tab: Move focus to the next menu button (Volume button)
Shift + Tab: Move focus to the previous menu button (Play/Pause button)

jmclaus · 2014-02-04T09:28:15Z

@anton Agreed, lets not pack this PR with too many new features. I just noticed this (under 'Menu or Menu bar' not 'Menu button'):

If a menu bar item has focus and the menu is not open, then:
Enter, Space, and the up down arrow keys opens the menu and places focus on the first menu item in the opened menu or child menu bar.

So I agree on behavior of Space/Enter as it is not an isolated menu button but part of a menu bar.

I've added a few things. Here's the updated behavior:

If focus is on the menu button:

Enter/Space/Up arrow: Open menu and move focus onto the first menu item.
Esc: Close menu. Return focus to menu button.
Tab: Move focus to the next menu button (Volume button).
Shift + Tab: Move focus to the previous menu button (Play/Pause button).

If focus is on a menu item:

Up arrow: Select previous menu item.
Down arrow: Select next menu item.
Esc: Close menu popup. Return focus to menu button.
Enter: Select menu item value. Return focus to menu button.
Tab: Close menu. Move focus to the next menu button (Volume button).
Shift + Tab: Close menu. Move focus to the previous menu button (Play/Pause button).

polesye · 2014-02-04T09:33:14Z

common/lib/xmodule/xmodule/js/spec/video/video_speed_control_spec.js

                beforeEach(function () {
                    state = jasmine.initializePlayer();
+                    speedControl = $('div.speeds');
+                    speedEntries = $('div.speeds>a');


speedEntries = speedControl.children('a')

@polesye Done (speedEntries = speedControl.children('a')).

jmclaus · 2014-02-04T15:21:38Z

@polesye @valera-rozuvan Addressed your comments, please continue review. Two specs were added, ENTER and ESCAPE behavior on menu items (I can't figure how to test the speed change on the ENTER keypress and left a TODO note. Any idea?). Also, using 'toBeFocused()' now makes the tests fail...

jmclaus · 2014-02-04T15:25:34Z

@polesye @valera-rozuvan New behavior left to implement after our recent discussion:

Pressing Space or Enter when Speed button has focus should open the menu and give focus to the last item in list (not the Speed button).
Pressing TAB or SHIFT + TAB should give focus to Play/Pause button and Volume button respectively.

jmclaus · 2014-02-04T18:48:03Z

@polesye @valera-rozuvan Implemented the following:

'Pressing Space or Enter when Speed button has focus should open the menu and give focus to the last item in list (not the Speed button).'

and changed the tests accordingly. Also reverted to not using 'toBeFocused()'.

jmclaus · 2014-02-05T10:07:32Z

@polesye @valera-rozuvan Please continue review. I just implemented the following:

Pressing TAB or SHIFT + TAB should give focus to Play/Pause button and Volume button respectively.

and added related tests. In these, there are four things I cannot get to work (tests fail, therefore I omitted them):

On ENTER, TAB, TAB+SHIFT, verify that menu closes.
On ENTER, verify that speed has changed.

I welcome any suggestion.

polesye · 2014-02-06T06:27:44Z

Open the menu with the keyboard when you mouse click away from the menu speed it should be closed.
Open the menu with the keyboard -> focus in on last item -> mouse over speed menu -> mouse leave speed menu -> speed menu is closed and focus is lost. Probably, menu button should catch this focus on menu closing.
Choose new speed in the menu via mouse -> menu is closed - > menu button catches focus. I think when we're working via mouse, menu button should not be outlined. What do you think?

jmclaus · 2014-02-06T07:11:20Z

@polesye

I agree.
I think that when the menu was opened with keyboard interaction, hovering the mouse on it then away from should do absolutely nothing. See http://adobe-accessibility.github.io/Accessible-Mega-Menu for example.
I agree, it is BLD-804.

polesye · 2014-02-06T07:19:53Z

I think that when the menu was opened with keyboard interaction, hovering the mouse on it then away from should do absolutely nothing. See http://adobe-accessibility.github.io/Accessible-Mega-Menu for example.

Agree.

polesye · 2014-02-06T07:21:35Z

Anyway it looks great! I believe we'll add the same behavior for the captions in future.

polesye · 2014-02-06T14:53:55Z

common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js

+                        $(nextSpeedLink).focus();
+                        break;
+                    // Close menu.
+                    case KEY.TAB:


I believe that we should not do that explicitly, it should works via native tab ordering.

@polesye If I remove the focus() call, things break. I'll leave it as is for the moment, let me wrap up the rest beforehand. Thanks.

valera-rozuvan · 2014-02-06T15:01:39Z

@jmclaus Wow! How much work went into this PR! Crisp and clean. I like it = )

👍 @polesye Great job with the perfectionistic code review! You basically leave me no work = )

polesye · 2014-02-06T15:12:33Z

common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js

+        // entries.
+        speedLinks.on('click', _clickHandler.bind(state));
+
+        speedLinks.each(function(index, speedLink) {


To improve performance and memory usage:

var speedButtonAnchor = state.videoSpeedControl.el.children('a'); state.videoSpeedControl.videoSpeedsEl.on('keydown', 'a.speed_link', function (event) { event.preventDefault(); event.stopImmediatePropagation(); var el = state.videoSpeedControl.el, index = $(event.currentTarget).parent().index(); switch (event.keyCode) { // Scroll up menu, wrapping at the top. Keep menu open. case KEY.UP: _previousSpeedLink(speedLinks, index).focus(); break; // Scroll down menu, wrapping at the bottom. Keep menu // open. case KEY.DOWN: _nextSpeedLink(speedLinks, index).focus(); break; // Close menu. case KEY.TAB: el.removeClass('open'); break; // Close menu, give focus to speed control and change // speed. case KEY.ENTER: case KEY.SPACE: el.removeClass('open'); speedButtonAnchor.focus(); state.videoSpeedControl.changeVideoSpeed(event); break; // Close menu and give focus to speed control. case KEY.ESCAPE: el.removeClass('open'); speedButtonAnchor.focus(); break; } });

@polesye Yes, delegated events, elegant. Done.

jmclaus · 2014-02-07T01:01:53Z

@polesye @valera-rozuvan The following has been done:

Open the menu with the keyboard when you mouse click away from the menu speed it should be closed.
I think that when the menu was opened with keyboard interaction, hovering the mouse on it then away from should do absolutely nothing. See http://adobe-accessibility.github.io/Accessible-Mega-Menu for example.

And addressed all comments, except the one regarding TAB (or Tab + SHIFT) keypress on the speed entries. Did a lot of refactoring and got rid of the last anonymous eventHandler (key press on speed entries).

Left to do: correct the failing tests. And add a few more of them to test new functionality. It's starting to look good! Please review again when you have time. Thanks for all your help and comments!

polesye · 2014-02-07T15:39:27Z

👍 once all tests will be written.

valera-rozuvan · 2014-02-07T15:43:09Z

👍

jmclaus · 2014-02-07T15:45:55Z

@valera-rozuvan @polesye OK, will let you know when they are done. Thanks.

jmclaus · 2014-02-10T09:35:44Z

@polesye @valera-rozuvan I added the missing tests. Please review. What should we do with the 3 disabled tests? 2 of them are not relevant since the refactoring. Should I just remove them? And the other one, I cannot get it to function. Anton suggested that we open a new ticket for that issue. Should I do that and remove the test in video_speed_control_spec.js?

polesye · 2014-02-10T10:54:25Z

@jmclaus I spent some time to figure out why this test doesn't work and found the problem:
Use here

spyOn($.fn, 'focus').andCallThrough();

instead

spyOn($.fn, 'focus');

And test do not close the speed menu on mouseleave if... pass.

2 of them are not relevant since the refactoring.

Yes, remove them.

jmclaus · 2014-02-10T11:42:07Z

@polesye @valera-rozuvan Removed 2 irrelevant tests. Fixed last test that wasn't working. Rebased and squashed commits. Good to merge? (when it passes Jenkins)

polesye · 2014-02-10T11:48:42Z

common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js

+
+    function _closeMenu(state) {
+        // Remove the previously added clickHandler from window element.
+        $(window).off('click', _clickHandler.bind(state));


Is this handler really removed after this? I'm not sure, because of bind.

jmclaus · 2014-02-10T11:51:29Z

@polesye You are right. Let me remove the bind, it is not necessary here.

polesye · 2014-02-10T11:54:15Z

common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js

+                focused = true
+            }
+        });
+        return focused;


Description: Check the current matched set of elements against a selector, element, or jQuery object and return true if at least one of these elements matches the given arguments. (See jquery is)

I think it can be rewritten so:

function _speedLinksFocused(state) { var speedLinks = state.videoSpeedControl.videoSpeedsEl .find('a.speed_link'); return speedLinks.is(':focus');

You right, done.

polesye · 2014-02-10T11:58:22Z

Good job! 👍 once Jenkins pass.

jmclaus · 2014-02-10T13:05:36Z

@polesye @valera-rozuvan Addressed latest comments. Rebased & squashed last commit. Checked new functionality -- all behaves correctly. All tests all pass locally.

valera-rozuvan · 2014-02-10T13:47:30Z

@jmclaus "Test Result (no failures)" means you should restart the Jenkins tests manually. Go to https://jenkins.testeng.edx.org/ , log in, select "edx-all-tests-manual-pr", click on "Build with Parameters", enter the PR number. After the second pass, Jenkins usually reports OK.

polesye · 2014-02-10T13:49:10Z

@valera-rozuvan it doesn't help. I have the same issue in my PR.

valera-rozuvan · 2014-02-10T13:49:39Z

@jmclaus 👍

polesye · 2014-02-10T13:50:11Z

👍

…ymous event handlers by named functions. Menu stays open on mouseleave when a speed entry has focus. In that case, the menu can be closed by clicking anywhere outside of it. [BLD-402, BLD-363]

…_improved_a11y Keyboard events and ARIA markup added to speed control

polesye reviewed Feb 3, 2014
View reviewed changes

valera-rozuvan reviewed Feb 4, 2014
View reviewed changes

polesye reviewed Feb 4, 2014
View reviewed changes

polesye reviewed Feb 6, 2014
View reviewed changes

polesye reviewed Feb 10, 2014
View reviewed changes

Keyboard events and ARIA markup added to speed control. Replaced anon…

31ffce4

…ymous event handlers by named functions. Menu stays open on mouseleave when a speed entry has focus. In that case, the menu can be closed by clicking anywhere outside of it. [BLD-402, BLD-363]

jmclaus pushed a commit that referenced this pull request Feb 10, 2014

Merge pull request #2381 from edx/jmclaus/feature_video_speed_control…

72ea40d

…_improved_a11y Keyboard events and ARIA markup added to speed control

jmclaus merged commit 72ea40d into master Feb 10, 2014

jmclaus deleted the jmclaus/feature_video_speed_control_improved_a11y branch February 10, 2014 21:07

snyk-bot mentioned this pull request Oct 4, 2021

[Snyk] Upgrade sinon from 2.3.5 to 11.1.2 baby636/edx-platform#5

Open

Keyboard events and ARIA markup added to speed control #2381

Keyboard events and ARIA markup added to speed control #2381

Conversation

jmclaus commented Jan 30, 2014

jmclaus commented Jan 31, 2014

jmclaus commented Feb 3, 2014

Choose a reason for hiding this comment

jmclaus commented Feb 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polesye commented Feb 4, 2014

jmclaus commented Feb 4, 2014

polesye commented Feb 4, 2014

jmclaus commented Feb 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmclaus commented Feb 4, 2014

jmclaus commented Feb 4, 2014

jmclaus commented Feb 4, 2014

jmclaus commented Feb 5, 2014

polesye commented Feb 6, 2014

jmclaus commented Feb 6, 2014

polesye commented Feb 6, 2014

polesye commented Feb 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valera-rozuvan commented Feb 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmclaus commented Feb 7, 2014

polesye commented Feb 7, 2014

valera-rozuvan commented Feb 7, 2014

jmclaus commented Feb 7, 2014

jmclaus commented Feb 10, 2014

polesye commented Feb 10, 2014

jmclaus commented Feb 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmclaus commented Feb 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polesye commented Feb 10, 2014

jmclaus commented Feb 10, 2014

valera-rozuvan commented Feb 10, 2014

polesye commented Feb 10, 2014

valera-rozuvan commented Feb 10, 2014

polesye commented Feb 10, 2014