Skip to content
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

Jump to bottom

Hotfix/native controls2 #1811

Closed
wants to merge 3 commits into from
Closed

Hotfix/native controls2 #1811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd4cbe7
Don't always add controls to the video element
dmlap Oct 3, 2014
3b3e4d5
Fix controls attribute test on iOS
dmlap Jan 19, 2015
8d0619f
Update nativeControlsForTouch default
dmlap Jan 20, 2015
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
10 changes: 9 additions & 1 deletion src/js/media/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ vjs.Html5.prototype.createEl = function(){
var player = this.player_,
// If possible, reuse original tag for HTML5 playback technology element
el = player.tag,
attributes,
newEl,
clone;

Expand All @@ -74,8 +75,15 @@ vjs.Html5.prototype.createEl = function(){
player.tag = null;
} else {
el = vjs.createEl('video');

// determine if native controls should be used
attributes = videojs.util.mergeOptions({}, player.tagAttributes);
if (!vjs.TOUCH_ENABLED || player.options()['nativeControlsForTouch'] !== true) {
delete attributes.controls;
}

vjs.setElementAttributes(el,
vjs.obj.merge(player.tagAttributes || {}, {
vjs.obj.merge(attributes, {
id:player.id() + '_html5_api',
'class':'vjs-tech'
})
Expand Down
15 changes: 14 additions & 1 deletion test/unit/media.html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ module('HTML5', {
id: function(){ return 'id'; },
el: function(){ return el; },
options_: {},
options: function(){ return {}; },
options: function(){ return this.options_; },
bufferedPercent: function() { return 0; },
controls: function(){ return false; },
usingNativeControls: function(){ return false; },
Expand Down Expand Up @@ -71,6 +71,19 @@ test('test playbackRate', function() {
strictEqual(tech.playbackRate(), 0.75);
});

test('should remove the controls attribute when recreating the element', function() {
Copy link
Member

Choose a reason for hiding this comment

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

@dmlap this test is failing on iOS. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

The first test fails because controls are always true on the iPhone (or at least the two I tested). I can add an if (vjs.IS_IPHONE == false), but I'd like to know if there's any bigger implications of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure why the test is failing on iPhones. Are you testing with 8.1? I feel like this passed on earlier versions but I'll have to go dig up a device to check.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I believe we only tested on 8.x. I don't know if we have a 7.x device test, but either way, it's how it works now. I did end up adding and IS_IPHONE check, but that's just a bandaid and I don't know how that really applies to what we're trying to test there. If anything else relies on that to be false it could be an issue.

var el;
player.tagAttributes = {
controls: true
};
// force custom controls so the test environment is equivalent on iOS
player.options_['nativeControlsForTouch'] = false;
el = tech.createEl();

ok(!el.controls, 'controls attribute is absent');
ok(player.tagAttributes.controls, 'tag attribute is still present');
});

test('patchCanPlayType patches canplaytype with our function, conditionally', function() {
// the patch runs automatically so we need to first unpatch
vjs.Html5.unpatchCanPlayType();
Expand Down
4 changes: 2 additions & 2 deletions test/unit/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ test('should restore attributes from the original video tag when creating a new
// simulate attributes stored from the original tag
player.tagAttributes = {
'preload': 'auto',
'controls': true,
'autoplay': true,
'webkit-playsinline': true
};

Expand All @@ -545,7 +545,7 @@ test('should restore attributes from the original video tag when creating a new
el = vjs.Html5.prototype.createEl.call(html5Mock);

equal(el.getAttribute('preload'), 'none', 'attribute was successful overridden by an option');
equal(el.getAttribute('controls'), '', 'controls attribute was set properly');
equal(el.getAttribute('autoplay'), '', 'autoplay attribute was set properly');
equal(el.getAttribute('webkit-playsinline'), '', 'webkit-playsinline attribute was set properly');
});

Expand Down