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 all 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
6 changes: 3 additions & 3 deletions src/css/components/_control-bar.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
@include transition($trans);
}

.video-js.vjs-controls-disabled .vjs-control-bar,
.video-js.vjs-using-native-controls .vjs-control-bar,
.video-js.vjs-error .vjs-control-bar {
.video-js.video-js.video-js.vjs-controls-disabled .vjs-control-bar,
.video-js.video-js.video-js.vjs-using-native-controls .vjs-control-bar,
.video-js.video-js.video-js.vjs-error .vjs-control-bar {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is necessarily because otherwise, the .vjs-no-flex selector below ends up being more specific than this and sets the control bar's display to table. This avoids using !important or really strong css selectors.

Copy link
Member

Choose a reason for hiding this comment

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

I get the limitation here, but I don't think we should say this is an ok thing to do in the stylesheet. It's gonna result in uglier and uglier selectors, and I think we can do better.

  • I'd prefer important here over a triple class name. It is after all important that the controls be hidden when the controls are disabled.
  • We could try lowering some of the selector strengths and using strategic ordering over increasing class names.
  • We could hide the controls in additional ways to display, so you have to do more than just display:table to override it.

Copy link
Member

Choose a reason for hiding this comment

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

Another option to fix this specific issue would be to add a .vjs-flex class. Since that's the default behavior it could basically be a noop with no specific styles associated with it (because they're the default), but it would solve specificity problems in situations like this.

display: none;
}

Expand Down
16 changes: 5 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,6 +1939,7 @@ 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');

/**
Expand All @@ -1959,6 +1952,7 @@ class Player extends Component {
*/
this.trigger('usingnativecontrols');
} else {
this.addTechControlsListeners();
this.removeClass('vjs-using-native-controls');

/**
Expand Down
6 changes: 4 additions & 2 deletions src/js/tech/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@ class Html5 extends Tech {
// Our goal should be to get the custom controls on mobile solid everywhere
// 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');
if (browser.TOUCH_ENABLED && options.nativeControlsForTouch === true ||
browser.IS_IPHONE ||
browser.IS_NATIVE_ANDROID) {
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
3 changes: 3 additions & 0 deletions src/js/utils/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import document from 'global/document';
import window from 'global/window';

const USER_AGENT = window.navigator.userAgent;
const webkitVersionMap = (/AppleWebKit\/([\d.]+)/i).exec(USER_AGENT);
const appleWebkitVersion = webkitVersionMap ? parseFloat(webkitVersionMap.pop()) : null;

/*
* Device is an iPhone
Expand Down Expand Up @@ -48,6 +50,7 @@ export const ANDROID_VERSION = (function() {
})();
// Old Android is defined as Version older than 2.3, and requiring a webkit version of the android browser
export const IS_OLD_ANDROID = IS_ANDROID && (/webkit/i).test(USER_AGENT) && ANDROID_VERSION < 2.3;
export const IS_NATIVE_ANDROID = IS_ANDROID && ANDROID_VERSION < 5 && appleWebkitVersion < 537;

export const IS_FIREFOX = (/Firefox/i).test(USER_AGENT);
export const IS_CHROME = (/Chrome/i).test(USER_AGENT);
Expand Down
1 change: 1 addition & 0 deletions test/unit/tech/tech-faker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class TechFaker extends Tech {
duration() { return {}; }
networkState() { return 0; }
readyState() { return 0; }
controls() { return false; }

// Support everything except for "video/unsupported-format"
static isSupported() { return true; }
Expand Down