Enable listening for touch events above the player #992
Conversation
Make components listen to touch events themselves. Components can have a "listenToTouchMove" property that would report user activity on touch moves. Currently, the only problem left is that the MediaTechController emits tap events to show/hide the controlbar but that causes the control bar to not be hidden via a tap.
Make media tech controller only hide control bar.
disableUserActivity on MediaTechController and Player. MediaTechController does it manually.
|
|
||
| // Turn on component tap events | ||
| this.emitTapEvents(); | ||
| //this.emitTapEvents(); |
There was a problem hiding this comment.
We don't need this anymore. Forgot to just delete it.
Do we also want to keep emitTapEvents around since it's not used anywhere else?
There was a problem hiding this comment.
Well, it looks like you moved the tap logic to the new touchend event at line 100, but tap works a little differently than just checking for any movement, it also checks if the touch took longer than 250ms (touch and hold). I think we might still want to use the tap event for manually toggling controls specifically.
There was a problem hiding this comment.
So basically not bother with listening to a touchend above and let the tap events stuff handle toggling of the controls?
There was a problem hiding this comment.
There's basically two parts to the media tech touch listeners:
- Reporting user activity when the controls are showing, so they keep showing
- Toggling the controls on/off when a tap happens
I think we can handle 2 with the tap event, but we still need 1.
There was a problem hiding this comment.
Yeah, I think tap event will cover 2 and reporting activity on touchmove above would cover 1.
| @@ -15,6 +15,7 @@ vjs.MediaTechController = vjs.Component.extend({ | |||
| vjs.Component.call(this, player, options, ready); | |||
|
|
|||
| this.initControlsListeners(); | |||
| this.disableUserActivity(); | |||
There was a problem hiding this comment.
Instead of enabling then immediately disabling, we could potentially pass an option value through to the component init that tells it to not enable in the first place
There was a problem hiding this comment.
That makes sense.
Ideas for the option name?
And would I just add it to the options object that Component is called with?
There was a problem hiding this comment.
Maybe 'reportUserActivity':false ?
Yeah, add it to the same options.
There was a problem hiding this comment.
makes sense. Doing it now.
|
Updated the tap events and touch move user activity. |
| this.on('touchcancel', preventBubble); | ||
| this.on('touchend', preventBubble); | ||
| this.on('touchmove', function(event) { | ||
| this.player().reportUserActivity(); |
There was a problem hiding this comment.
oops, forgot to make this only report activity if userWasActive.
|
Updated the options. |
| @@ -65,6 +65,10 @@ vjs.Component = vjs.CoreObject.extend({ | |||
| this.ready(ready); | |||
| // Don't want to trigger ready here or it will before init is actually | |||
| // finished for all children that run this constructor | |||
|
|
|||
| if (options.reportUserActivity) { | |||
There was a problem hiding this comment.
If we want enableUserActivity to be the default for components, I think we want this to be if (options.reportUserActivity !== false)
This fixes #980.
This makes each component report user activity. The
MediaTechControllerandPlayerneed to disable this.MediaTechControllerreports user activity manually as it also handles hiding and showing of the control bar.The tap test should only focus on taps.