-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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.
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
Control bar updates #672
Control bar updates #672
Conversation
…eo-js into feature/control-bar-fixes
oh man, huge update. :D |
haha, yeah... controls touch a lot of things when you get into it. Let me know if you want to talk through anything live. |
One of the first thing I noticed is that there is a function |
Yeah, the naming is something I went back and forth on, and definitely still open to other ideas. I actually started with userActive, then switched everything over to userState for the reasons you mentioned, then switched back to userActive because I really liked working with a booleans over strings. It felt simpler and more readable. I dug into it and the HTML5 video spec seems to prefer booleans for states with two possible values, e.g. That still leaves
|
Hm... yeah, |
Cool. Are there any code examples that would show how the string would be more obvious? I feel like when you're reading the value, the code is more obvious with the boolean. var state = userState(); // The value could be anything
var active = userActive(); // Reads like a boolean Granted, If |
Yeah, API design is hard 😁 setUserState('active');
var isUserActive = vjs.isUserActive(); Or something like that. Though, I guess it isn't that great that the setter takes a string of 'active'/'inactive' or 'active'/'passive'. Perhaps an enum would be good? var userState = function() {};
userState.ACTIVE = true;
userState.INACTIVE = false;
userState(userState.ACTIVE);
var isActive = userState();
if (isActive) { }
if (isActive === userState.ACTIVE) { } |
The english language is hard too. :) I could see using an enum internally, but in the exposed API it starts to break away from the existing conventions. Seems like 'idle' is a word worth considering. |
Do we need an API for var userActive = !!document.querySelector('vjs-user-active'); I prefer not having "aliases" for state if the canonical source is easily accessible. |
this.trigger('tap'); | ||
// It may be good to copy the touchend event object and change the | ||
// type to tap, if the other event properties aren't exact after | ||
// vjs.fixEvent runs (e.g. event.target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea to me
That's exactly one of the reasons why booleans are iffy for this kind of API. |
player.off('controlsenabled', controlsOn); | ||
player.off('controlsdisabled', controlsOff); | ||
}; | ||
tech.on('dispose', cleanUp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we should go backbone's route with their new listenTo event methods which will auto-remove event handlers when an object gets disposed off.
Also, wrt |
The mobile tap example demonstrates pretty clearly that there's a lot of uses for allowing plugins and external applications to interact with this state. Any application that uses the playback api to interact with the player would probably benefit. It also makes it possible to keep the controls enabled. player.on('userpassive', function(){
player.userActive(true);
}); Or to trigger activity when the user interacts with a related part of the page. |
Yeah, that's ok, but I think developers will lean towards events while designers will lean towards classes. That's a massive generalization. :-P
That's a good point. Plugins could build controls outside of the player div that should inform user activity. Though the reportUserActivity method would more likely be used in that case. But a few people have pointed out that the main concern is the class names and the event names, so I'm probably over-thinking the method name. It's needed internally, and maybe by advanced tech developers, to change the state and trigger the events, but otherwise it will rarely be used. I still like the change from |
…ouse Also cleaned up some of the user activity watching code, adding support for tracking when the mouse continues to be down without any movement
|
||
// When the touch ends, measure how long it took and trigger the appropriate | ||
// event | ||
this.on('touchend', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me with this implementation as long as only one finger is involved in the touching. If multiple fingers are involved, you could have a situation where you have a false positive—a different finger triggers touchend
from the one that issued the touchstart
.
Probably an edge case not worth worrying about at this time. If we get a couple of complaints about touch interaction, might be worth looking into. Sometimes people touch the device with more than one finger accidentally, for example by resting their hand against one edge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you could store the touch ids from the touchstart, the problem is keeping them around and not have them be overridden and all that other fun stuff.
Touch rejection is hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. This rabbit hole just gets deeper. I'm with you @bcjwilson on waiting for complaints on the multi touch issue.
Ok, I'm going to pull this in. Native controls are still the default on touch devices, so some I'm going to create issues for these ongoing discussions, to be addressed in a future pass to get controls solid on mobile. On that note, let me know if there's any code in the BC player fork that could still be pushed back. |
Fixed a number of issues around the control bar
#556 disabled controls are displayed if movie is paused via api
#500 Fullscreen Hiding
#374 Fullscreen hiding
#403 Fix for Fullscreen hiding
#441 Control interaction, don't hide controls. fading out under user
#193 Hide in fullscren
#602 mouseout on control bar triggering fadeout
#561 Setting controls when switching techs to html5 video
#281 Enable/Disable controls after player is created
Made some architectural changes to support these updates. I have blog post draft of those changes here:
Toggling Video Player Controls
Last week I decided to tackle a number of outstanding issues around the control bar, and then proceeded to fall down a rabbit hole of related player updates. I've thankfully resurfaced now, and figured I'd write about a few of the updates that came from it.
One of the expected behaviors of the player's control bar is that it will fade out after a couple of seconds when the user is inactive while watching a video. Previously, the way we achieved this with video.js was through a bit of a CSS trick. When the user's mouse would move out of the video player area, the control bar would be given the classname
vjs-fade-out
. This class had a visibility transition with an added 2 second delay.When the user's mouse moved back over the player, the class would be removed, canceling any delayed fade-out. This provided a similar experience to how you might expect the controls fading to work, and only took a few lines of javascript to add/remove the class.
There's a few drawbacks though that have made it necessary to move away from this approach.
In addition to these issues, we want it to be possible for any player component/plugin to hook into the same trigger that hides the contorls. Components like social sharing icons should fade out in the same way that the controls do.
User State
One of the first things that is being added is a
userActive
property on the player, that can be eithertrue
orfalse
. What this does is abstract the controls hiding out to what it is we're actually concerned with, that is, whether the user is currently interacting with the player or just passively watching the video. This also decouples the control bar from tracking the user activity itself, and allows other components to more easily behave the same way as the control bar, through a player-level state.That actual property is
player.userActive()
and returns eithertrue
orfalse
. When this value is changed, it triggers an event on the player.A CSS classname of either
vjs-user-active
orvjs-user-passive
is also added to the player element. The classname is what's actually used now to hide and show the control bar.The 2 second delay has been removed from the CSS, and instead will be built into the process of setting the userActive state to false through a javascript timeout. Anytime a mouse event occurs on the player, this timeout will reset. e.g.
The mousemove event is called very rapidly while the mouse is moving, and we want to bog down the player process as little as possible during this action, so we're using a technique written about by John Resig. http://ejohn.org/blog/learning-from-twitter/
Intead of resetting the timeout for every mousemove, the mousemove event will instead set a variable that can be picked up by a javascipt interval that's running at a slower pace.
That may be a lot to follow, and it's a bit of a simplification of what's actually in the player now, but essentially it allows us to take some of the processing weight off of the browser while the mouse is moving.
Hiding controls in fullscreen
Thanks to the new userActive state and the javascript timeout for the delay, the controls no longer require the mouse to move outside of the player area in order to hide and can now hide in fullscreen mode the same way they do when the player is in the page. This also means we can now hide the mouse cursor in the same way we do the controls, so that it doesn't sit over the player while watching in fullscreen.
Hiding controls on touch devices
The expected behavior on touch devices is a little different than in desktop browsers. There is no mousemove event to help determine if the user is active or passive, so typically a longer delay is added before the controls are faded out. Also, while a click on the video itself in desktop browsers will typically toggle playback, a tap on the video on mobile devices will toggle the controls.
Luckily the framework we've set up around userActive has made this last part easy enough to set up.
Manaully toggling userActive between true and false will apply the appropriate classnames and trigger the events needed to show and hide the controls as you'd expect on a mobile device.
The
tap
event is actually a custom event, similar to the tap event you'll find in jQuery mobile and other mobile libraries. A tap event occurs whenever atouchstart
event is fired with the associatedtouchend
event firing within 250 milliseconds. If thetouchend
event takes longer to fire, or if atouchmove
event happens between the two, it is not considered atap
.Conclusion
This wasn't meant to be a full how-to guide on video player controls, but hopefully it'd given some insight into how that piece of the controls operate, and how you can mimic the same activity if you're building your own additions to video.js.