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.

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

Mouse time display #2569

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
27 changes: 27 additions & 0 deletions src/css/components/_progress.scss
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
.video-js .vjs-progress-control:hover .vjs-progress-holder { font-size: 1.666666666666666666em }

/* Also show the current time tooltip */
.video-js .vjs-progress-control:hover .vjs-mouse-display:after,
.video-js .vjs-progress-control:hover .vjs-play-progress:after {
display: block;

Expand All @@ -53,6 +54,13 @@
top: 0;
}

.video-js .vjs-mouse-display {
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. This makes the two displays the same size. I can move it down in the file, if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

Ah! I see. Cool. Here's good.

@extend .vjs-icon-circle;

&:before {
display: none;
}
}
.video-js .vjs-play-progress {
background-color: $primary-text;
@extend .vjs-icon-circle;
Expand All @@ -65,10 +73,12 @@
font-size: 0.9em;
height: 1em;
line-height: 1em;
z-index: 1;
}
}

// Current Time "tooltip"
.video-js .vjs-mouse-display:after,
.video-js .vjs-play-progress:after {
/* By default this is hidden and only shown when hovering over the progress control */
display: none;
Expand All @@ -81,6 +91,7 @@
padding: 0.2em 0.5em;
@include background-color-with-alpha($primary-text, 0.8);
@include border-radius(0.3em);
z-index: 1;
}

.video-js .vjs-load-progress {
Expand All @@ -97,3 +108,19 @@ specific time ranges that have been buffered */
.video-js.vjs-no-flex .vjs-progress-control {
width: auto;
}

.video-js .vjs-progress-control .vjs-mouse-display {
display: none;
position: absolute;
width: 1px;
height: 100%;
background-color: $primary-bg;
opacity: 0.9;
}
.video-js .vjs-progress-control:hover .vjs-mouse-display {
display: block;
}
.video-js .vjs-progress-control .vjs-mouse-display:after {
color: $primary-text;
@include background-color-with-alpha($primary-bg, 0.8);
}
70 changes: 70 additions & 0 deletions src/js/control-bar/progress-control/mouse-time-display.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* @file mouse-time-display.js
*/
import Component from '../../component.js';
import * as Dom from '../../utils/dom.js';
import * as Fn from '../../utils/fn.js';
import formatTime from '../../utils/format-time.js';
import throttle from 'lodash-compat/function/throttle';

/**
* The Mouse Time Display component shows the time you will seek to
* when hovering over the progress bar
*
* @param {Player|Object} player
* @param {Object=} options
* @extends Component
* @class MouseTimeDisplay
*/
class MouseTimeDisplay extends Component {

constructor(player, options) {
super(player, options);
this.update();
player.on('ready', () => {
this.on(player.controlBar.progressControl.el(), 'mousemove', throttle(Fn.bind(this, this.handleMouseMove), 50));
Copy link
Member Author

Choose a reason for hiding this comment

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

we want to listen to mousemove on the progress control because otherwise we won't get any events since the bar will be hidden.
Also, we throttle it to 50ms so that it doesn't fire too often.

Copy link
Member

Choose a reason for hiding this comment

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

Could we just create a single component for the mouse time display and add it as child of SeekBar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly, for this PR I went with the "least work and use built-in components as much as possible" approach.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, but we end up with a lot of extra components and html. I think it'd be worth trying to make it one if we can.

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 have an idea how to fix this up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you also wanted less html elements. I'll see if it's easy to do.

Copy link
Member

Choose a reason for hiding this comment

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

Threw together an example. It's not great but it's an example.
http://jsbin.com/sipamakoba/edit?html,output

Copy link
Member

Choose a reason for hiding this comment

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

Let me know if you see any limitations in that approach. I can already see that the listener would need to be progress-control, not just the seekbar. The seekbar doesn't have enough vertical space.

Copy link
Member Author

Choose a reason for hiding this comment

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

One of the main issues is that we're duplicating the "calculate distance"/"calculate time" code. However, I'll work on merging the two. Maybe the slider's calculate distance method should be moved out into a shared function.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that'd be cool. There's dom.findElPosition which does some of it, but I didn't use it because I don't think it's exported. But something more like a getRelativePosition dom function would be useful.

Copy link
Member

Choose a reason for hiding this comment

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

It feels a little jumpy to me at 20fps. It's nit-picky but I think we could bump it up to at least 33ms without impacting performance.

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 can drop it down to 25 or 33 or something.

});
}

/**
* Create the component's DOM element
*
* @return {Element}
* @method createEl
*/
createEl() {
return super.createEl('div', {
className: 'vjs-mouse-display'
});
}

getPercent() {
return this.newTime / this.player_.duration();
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of using currentTime, use the newTime that we calculated in handleMouseMove.

Copy link
Member

Choose a reason for hiding this comment

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

Since this isn't inheriting from slider anymore I don't think getPercent is being used anywhere.

}

handleMouseMove(event) {
Copy link
Member

Choose a reason for hiding this comment

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

handleMouseMove, calculateDistance, update, and updateDataAttr could probably be consolidated. It depends on your preference for granularity, but they're all basically just used once now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, looks like it's not quite necessary to have them all as separate functions.

this.newTime = this.calculateDistance(event) * this.player_.duration();

this.position = event.pageX - Dom.findElPosition(this.el().parentNode).left;

this.update();
}

calculateDistance(event) {
return Dom.getPointerPosition(this.el().parentNode, event).x;
}

update() {
Copy link
Member Author

Choose a reason for hiding this comment

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

update the bar's data attribute. Then update the slider so it's set up correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to just add this as a comment for clarity?

Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, obviously, but it seems if it's worth pointing out to us, it's worth just having in there for some future person.

this.updateDataAttr();

this.el().style.left = this.position + 'px';
}

updateDataAttr() {
let time = formatTime(this.newTime, this.player_.duration());
this.el().setAttribute('data-current-time', time);
}
}

Component.registerComponent('MouseTimeDisplay', MouseTimeDisplay);
export default MouseTimeDisplay;
1 change: 1 addition & 0 deletions src/js/control-bar/progress-control/progress-control.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import Component from '../../component.js';
import SeekBar from './seek-bar.js';
import MouseTimeDisplay from './mouse-time-display.js';

/**
* The Progress Control component contains the seek bar, load progress,
Expand Down
13 changes: 8 additions & 5 deletions src/js/control-bar/progress-control/seek-bar.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import LoadProgressBar from './load-progress-bar.js';
import PlayProgressBar from './play-progress-bar.js';
import * as Fn from '../../utils/fn.js';
import formatTime from '../../utils/format-time.js';
import assign from 'object.assign';

/**
* Seek Bar and holder for the progress bars
Expand All @@ -30,11 +31,12 @@ class SeekBar extends Slider {
* @return {Element}
* @method createEl
*/
createEl() {
return super.createEl('div', {
className: 'vjs-progress-holder',
createEl(type='div', props={}) {
Copy link
Member Author

Choose a reason for hiding this comment

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

going to revert this since I'm no longer using it.

props = assign({
'aria-label': 'video progress bar'
});
}, props);
props.className = 'vjs-progress-holder' + (props.className ? ' ' + props.className : '');
return super.createEl(type, props);
}

/**
Expand Down Expand Up @@ -126,7 +128,8 @@ class SeekBar extends Slider {
SeekBar.prototype.options_ = {
children: {
'loadProgressBar': {},
'playProgressBar': {}
'playProgressBar': {},
'mouseTimeDisplay': {}
},
'barName': 'playProgressBar'
};
Expand Down
32 changes: 3 additions & 29 deletions src/js/slider/slider.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,37 +145,11 @@ class Slider extends Component {
* @method calculateDistance
*/
calculateDistance(event){
let el = this.el_;
let box = Dom.findElPosition(el);
let boxW = el.offsetWidth;
let boxH = el.offsetHeight;

let position = Dom.getPointerPosition(this.el_, event);
if (this.vertical()) {
let boxY = box.top;

let pageY;
if (event.changedTouches) {
pageY = event.changedTouches[0].pageY;
} else {
pageY = event.pageY;
}

// Percent that the click is through the adjusted area
return Math.max(0, Math.min(1, ((boxY - pageY) + boxH) / boxH));

} else {
let boxX = box.left;

let pageX;
if (event.changedTouches) {
pageX = event.changedTouches[0].pageX;
} else {
pageX = event.pageX;
}

// Percent that the click is through the adjusted area
return Math.max(0, Math.min(1, (pageX - boxX) / boxW));
return position.y;
}
return position.x;
}

/**
Expand Down
32 changes: 32 additions & 0 deletions src/js/utils/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -332,3 +332,35 @@ export function findElPosition(el) {
top: Math.round(top)
};
}

/**
* Get pointer position in element
* Returns an object with x and y coordinates.
* The base on the coordinates are the bottom left of the element.
*
* @param {Element} el Element on which to get the pointer position on
* @param {Event} event Event object
* @return {Object=} position This object will have x and y coordinates corresponding to the mouse position
* @metho getPointerPosition
*/
export function getPointerPosition(el, event) {
let position = {};
let box = findElPosition(el);
let boxW = el.offsetWidth;
let boxH = el.offsetHeight;

let boxY = box.top;
let boxX = box.left;
let pageY = event.pageY
let pageX = event.pageX;

if (event.changedTouches) {
pageX = event.changedTouches[0].pageX;
pageY = event.changedTouches[0].pageY;
}

position.y = Math.max(0, Math.min(1, ((boxY - pageY) + boxH) / boxH));
position.x = Math.max(0, Math.min(1, (pageX - boxX) / boxW));

return position;
}