Skip to content

Valera/bugfix tabbing video controls #659

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.

Sign up for GitHub

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

Merged
valera-rozuvan merged 7 commits into from
Aug 14, 2013
Merged

Valera/bugfix tabbing video controls #659

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
05539b1
Removed unnecessary tabindex -1 from Video template.
Aug 13, 2013
0a5d261
For YouTube videos tabbing from Speeds to Volume closes Speeds dialog.
Aug 13, 2013
da3e21c
Fixed tabbing backwards.
Aug 13, 2013
efac70e
Adding documentation to the event handlers for volume and speed control.
Aug 13, 2013
3b326c3
Fixing spelling typos in comments.
Aug 14, 2013
99ae637
Removed unnecessary tabindex = 0 from <a /> - they get it by default.
Aug 14, 2013
b583a4e
Optimizing code. Caching an element selector.
Aug 14, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 51 additions & 14 deletions common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,31 +67,68 @@ function () {
state.videoVolumeControl.el.toggleClass('muted', state.videoVolumeControl.currentVolume === 0);
}

// function _bindHandlers(state)
//
// Bind any necessary function callbacks to DOM events (click, mousemove, etc.).
/**
* @desc Bind any necessary function callbacks to DOM events (click,
* mousemove, etc.).
*
* @type {function}
* @access private
*
* @param {object} state The object containg the state of the video player.
* All other modules, their parameters, public variables, etc. are
* available via this object.
*
* @this {object} The global window object.
*
* @returns {undefined}
*/
function _bindHandlers(state) {
state.videoVolumeControl.buttonEl.on('click', state.videoVolumeControl.toggleMute);
state.videoVolumeControl.buttonEl
.on('click', state.videoVolumeControl.toggleMute);

state.videoVolumeControl.el.on('mouseenter', function() {
$(this).addClass('open');
});

state.videoVolumeControl.buttonEl.on('focus', function() {
$(this).parent().addClass('open');
state.videoVolumeControl.el.addClass('open');
});

state.videoVolumeControl.el.on('mouseleave', function() {
$(this).removeClass('open');
state.videoVolumeControl.el.removeClass('open');
});

// Attach a focus event to the volume button.
state.videoVolumeControl.buttonEl.on('blur', function() {
state.videoVolumeControl.volumeSliderEl.find('a').focus();
// If the focus is being trasnfered from the volume slider, then we
// don't do anything except for unsetting the special flag.
if (state.volumeBlur === true) {
state.volumeBlur = false;
}

//If the focus is comming from elsewhere, then we must show the
// volume slider and set focus to it.
else {
state.videoVolumeControl.el.addClass('open');
state.videoVolumeControl.volumeSliderEl.find('a').focus();
}
});

state.videoVolumeControl.volumeSliderEl.find('a').on('blur', function () {
state.videoVolumeControl.el.removeClass('open');
});
// Attach a blur event handler (loss of focus) to the volume slider
// element. More specifically, we are attaching to the handle on
// the slider with which you can change the volume.
state.videoVolumeControl.volumeSliderEl.find('a')
.on('blur', function () {
// Hide the volume slider. This is done so that we can
// contrinue to the next (or previous) element by tabbing.

@polesye polesye Aug 14, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contrinue -> continue

// Otherwise, after next tab we would come back to the volume
// slider because it is the next element sisible element that

@polesye polesye Aug 14, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sisible -> visible

// we can tab to after the volume button.
state.videoVolumeControl.el.removeClass('open');

// Set focus to the volume button.
state.videoVolumeControl.buttonEl.focus();

// We store the fact that previous element that lost focus was
// the volume clontrol.
state.volumeBlur = true;
});
}

// ***************************************************************
Expand Down
96 changes: 81 additions & 15 deletions common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,46 +61,114 @@ function () {
state.videoSpeedControl.setSpeed(state.speed);
}

// function _bindHandlers(state)
//
// Bind any necessary function callbacks to DOM events (click,
// mousemove, etc.).
/**
* @desc Bind any necessary function callbacks to DOM events (click,
* mousemove, etc.).
*
* @type {function}
* @access private
*
* @param {object} state The object containg the state of the video player.
* All other modules, their parameters, public variables, etc. are
* available via this object.
*
* @this {object} The global window object.
*
* @returns {undefined}
*/
function _bindHandlers(state) {
state.videoSpeedControl.videoSpeedsEl.find('a')
.on('click', state.videoSpeedControl.changeVideoSpeed);

if (onTouchBasedDevice()) {
state.videoSpeedControl.el.on('click', function(event) {
// So that you can't highlight this control via a drag
// operation, we disable the default browser actions on a
// click event.
event.preventDefault();
$(this).toggleClass('open');

state.videoSpeedControl.el.toggleClass('open');
});
} else {
state.videoSpeedControl.el
.on('mouseenter', function () {
$(this).addClass('open');
state.videoSpeedControl.el.addClass('open');
})
.on('mouseleave', function () {
$(this).removeClass('open');
state.videoSpeedControl.el.removeClass('open');
})
.on('click', function (event) {
// So that you can't highlight this control via a drag
// operation, we disable the default browser actions on a
// click event.
event.preventDefault();
$(this).removeClass('open');

state.videoSpeedControl.el.removeClass('open');
});

// ******************************
// Attach 'focus', and 'blur' events to the speed button which
// either brings up the speed dialog with individual speed entries,
// or closes it.
state.videoSpeedControl.el.children('a')
.on('focus', function () {
$(this).parent().addClass('open');
// If the focus is comming from the first speed entry, this
// means we are tabbing backwards. In this case we have to
// hide the speed entries which will allow us to change the
// focus further backwards.
if (state.firstSpeedBlur === true) {
state.videoSpeedControl.el.removeClass('open');

state.firstSpeedBlur = false;
}

// If the focus is comming from some other element, show
// the drop down with the speed entries.
else {
state.videoSpeedControl.el.addClass('open');
}
})
.on('blur', function () {
// When the focus leaves this element, if the speed entries
// dialog is shown (tabbing forwards), then we will set
// focus to the first speed entry.
//
// If the selector does not select anything, then this
// means that the speed entries dialog is closed, and we
// are tabbing backwads. The browser will select the
// previous element to tab to by itself.
state.videoSpeedControl.videoSpeedsEl
.find('a.speed_link:first')
.focus();
});


// ******************************
// Attach 'focus', and 'blur' events to elements which represent
// individual speed entries.
state.videoSpeedControl.videoSpeedsEl.find('a.speed_link:last')

@polesye polesye Aug 14, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache your selection, for instance :

var speedLinks = state.videoSpeedControl.videoSpeedsEl.find('a.speed_link');
speedLinks.last().on('blur', ...);
speedLinks.first().on('blur', ...);
speedLinks.on('focus', ...);

.on('blur', function () {
// If we have reached the last speed enrty, and the focus

@polesye polesye Aug 14, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enrty -> entry

// changes to the next element, we need to hide the speeds
// control drop-down.
state.videoSpeedControl.el.removeClass('open');
});
state.videoSpeedControl.videoSpeedsEl.find('a.speed_link:first')
.on('blur', function () {
// This flag will indicate that the focus to the next
// element that will receive it is comming from the first
// speed entry.
//
// This flag will be used to correctly handle scenario of
// tabbing backwards.
state.firstSpeedBlur = true;
});
state.videoSpeedControl.videoSpeedsEl.find('a.speed_link')
.on('focus', function () {
// Clear the flag which is only set when we are un-focusing
// (the blur event) from the first speed entry.
state.firstSpeedBlur = false;
});
}
}

Expand Down Expand Up @@ -151,7 +219,7 @@ function () {
$.each(this.videoSpeedControl.speeds, function(index, speed) {
var link, listItem;

link = '<a href="#">' + speed + 'x</a>';
link = '<a class="speed_link" href="#">' + speed + 'x</a>';

listItem = $('<li data-speed="' + speed + '">' + link + '</li>');

Expand All @@ -162,11 +230,9 @@ function () {
_this.videoSpeedControl.videoSpeedsEl.prepend(listItem);
});

this.videoSpeedControl.videoSpeedsEl.find('a')
.on('click', this.videoSpeedControl.changeVideoSpeed);

// TODO: After the control was re-rendered, we should attach 'focus'
// and 'blur' events once more.
// Re-attach all events with their appropriate callbacks to the
// newly generated elements.
_bindHandlers(this);
}

});
Expand Down
10 changes: 5 additions & 5 deletions lms/templates/video.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ <h2>${display_name}</h2>
<div class="secondary-controls">
<div class="speeds">
<a href="#" tabindex="0" title="Speeds">

@polesye polesye Aug 14, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this tabindex, because anchor will be focused by default.

<h3 tabindex="-1">${_('Speed')}</h3>
<p tabindex="-1" class="active"></p>
<h3>${_('Speed')}</h3>
<p class="active"></p>
</a>
<ol tabindex="-1" class="video_speeds"></ol>
<ol class="video_speeds"></ol>
</div>
<div class="volume">
<a href="#" tabindex="0" title="Volume"></a>
<div tabindex="-1" class="volume-slider-container">
<div tabindex="-1" class="volume-slider"></div>
<div class="volume-slider-container">
<div class="volume-slider"></div>
</div>
</div>
<a href="#" class="add-fullscreen" tabindex="0" title="${_('Fill browser')}">${_('Fill browser')}</a>
Expand Down