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

Native controls updates #2499

Closed
wants to merge 8 commits into from
Closed

Native controls updates #2499

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5d31bdf
Use native controls on iphone.
gkatsev Jul 16, 2015
1af6089
Update nativeControls functionality
gkatsev Aug 21, 2015
80c9ca9
Add an IS_NATIVE_ANDROID to browser object
gkatsev Aug 21, 2015
28509f4
Default to native controls for touch on iphones and native android de…
gkatsev Aug 21, 2015
ebf758f
Add tech controls listners when disabling native controls usage
gkatsev Aug 21, 2015
92a637a
increase css specificity
gkatsev Aug 21, 2015
693ae2e
remove techCalls from usingNativeControls method
gkatsev Aug 21, 2015
d9c0263
Add controls method to TechFaker
gkatsev Aug 21, 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
17 changes: 6 additions & 11 deletions src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,7 @@ class Player extends Component {

// Grab tech-specific options from player options and add source and parent element to use.
var techOptions = assign({
'nativeControlsForTouch': this.options_.nativeControlsForTouch,
Copy link
Member Author

Choose a reason for hiding this comment

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

pass the option to the tech

'source': source,
'playerId': this.id(),
'techId': `${this.id()}_${techName}_api`,
Expand Down Expand Up @@ -534,7 +535,6 @@ class Player extends Component {
textTrackConverter.jsonToTextTracks(this.textTracksJson_ || [], this.tech);

this.on(this.tech, 'ready', this.handleTechReady);
Copy link
Member Author

Choose a reason for hiding this comment

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

no longer needed because we assume if techGet('controls') is true, we are using native controls.

this.on(this.tech, 'usenativecontrols', this.handleTechUseNativeControls);

// Listen to every HTML5 events and trigger them back on the player for the plugins
this.on(this.tech, 'loadstart', this.handleTechLoadStart);
Expand Down Expand Up @@ -564,6 +564,8 @@ class Player extends Component {
this.on(this.tech, 'texttrackchange', this.onTextTrackChange);
this.on(this.tech, 'loadedmetadata', this.updateStyleEl_);

this.usingNativeControls(this.techGet('controls'));

if (this.controls() && !this.usingNativeControls()) {
this.addTechControlsListeners();
}
Expand Down Expand Up @@ -665,16 +667,6 @@ class Player extends Component {
}
}

/**
* Fired when the native controls are used
*
* @private
* @method handleTechUseNativeControls
*/
handleTechUseNativeControls() {
this.usingNativeControls(true);
}

/**
* Fired when the user agent begins looking for media data
*
Expand Down Expand Up @@ -1947,7 +1939,9 @@ class Player extends Component {
if (this.usingNativeControls_ !== bool) {
this.usingNativeControls_ = bool;
if (bool) {
this.removeTechControlsListeners();
Copy link
Member Author

Choose a reason for hiding this comment

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

make sure to remove these listeners if they got added, otherwise, it will mess things up.
Our custom controls will show up unnecessarily, and also do weird stuff like prevent the video from playing correctly (#2277)

Copy link
Member

Choose a reason for hiding this comment

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

This wouldn't hurt as a code comment if you feel like adding it.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to re-add the listeners if we swap back to non-native controls?

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 guess we should also re-add them below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

this.addClass('vjs-using-native-controls');
this.techCall('setControls', true);
Copy link
Member Author

Choose a reason for hiding this comment

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

Make sure we have controls


/**
* player is using the native device controls
Expand All @@ -1960,6 +1954,7 @@ class Player extends Component {
this.trigger('usingnativecontrols');
} else {
this.removeClass('vjs-using-native-controls');
this.techCall('setControls', false);
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove controls if they are no longer needed


/**
* player is using the custom HTML controls
Expand Down
2 changes: 1 addition & 1 deletion src/js/tech/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class Html5 extends Tech {
// so we can remove this all together. Right now this will block custom
// controls on touch enabled laptops like the Chrome Pixel
if (browser.TOUCH_ENABLED && options.nativeControlsForTouch === true) {
this.trigger('usenativecontrols');
this.setControls(true);
Copy link
Member Author

Choose a reason for hiding this comment

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

make sure controls are set.

}

this.triggerReady();
Expand Down