Hotfix/native controls2 #1811
Hotfix/native controls2 #1811
Conversation
Fix for videojs#1561. If the HTML tech is being constructed without a video element to work off of, make sure that the controls attribute is only added under the same circumstances it would be at player init. Before this fix, if you loaded the Flash tech and then switched to the HTML tech, you would see the native controls underneath the video.js controls.
iOS uses native controls by default and so was failing the assertions that native controls weren't used. Force custom controls for this test case to make it work like everywhere else.
|
|
||
| // determine if native controls should be used | ||
| attributes = videojs.util.mergeOptions({}, player.tagAttributes); | ||
| if (!vjs.TOUCH_ENABLED || player.options()['nativeControlsForTouch'] === false) { |
There was a problem hiding this comment.
The default here changed. See line 33. (an example of why it'd be good to have things in the same place if we can figure it out some how)
|
Other than the comment on the default, this looks good to me. We should try to get it out as a patch. |
The default value changed so fix the predicate that tested for whether it was in use.
|
@heff updated to properly test for the new default setting |
| @@ -71,6 +71,19 @@ test('test playbackRate', function() { | |||
| strictEqual(tech.playbackRate(), 0.75); | |||
| }); | |||
|
|
|||
| test('should remove the controls attribute when recreating the element', function() { | |||
There was a problem hiding this comment.
@dmlap this test is failing on iOS. Any ideas?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Rebased #1564 with a test fix.