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

Don't force sliders to get evaluated on load #1122

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dmlap
Member

dmlap commented Mar 31, 2014

Rebased version of #1079

Since the load and play progress sliders are guaranteed to start from zero, set that through CSS. Calling Slider.prototype.update forces a re-flow because element dimensions are queried and style rules changed. That reflow consistently took around 60ms on my laptop which would mean dropped frames and "jerkiness" on initialization. Updated to handle scenarios where the video.js stylesheet is loaded after the ready event has already fired.

dmlap added some commits Mar 12, 2014

Don't force sliders to get evaluated on load
Since the load and play progress sliders are guaranteed to start from zero, set that through CSS. Calling Slider.prototype.update forces a re-flow because element dimensions are queried and style rules changed. That reflow consistently took around 60ms on my laptop which would mean dropped frames and "jerkiness" on initialization.
Peg the volume control to 1.0 on init
Setup styles so that the volume control initially renders at full volume. This matches browser behavior where volume is available and saves Javascript from having to manually update the volume control on init. After the initial draw, the volume control is updated dynamically the same way it was before.
/* Assumes volume starts at 1.0. If you change the size of the
handle relative to the volume bar, you'll need to update this value
too. */
left: 90%;

This comment has been minimized.

@heff

heff Apr 1, 2014

Member

Would it be possible and make sense to use ems here to make the units consistent?
e.g. the clearest I think would be right: -0.5em;, but I guess left is how js updates the position, yeah? So maybe left: 4.5em?

@heff

heff Apr 1, 2014

Member

Would it be possible and make sense to use ems here to make the units consistent?
e.g. the clearest I think would be right: -0.5em;, but I guess left is how js updates the position, yeah? So maybe left: 4.5em?

This comment has been minimized.

@dmlap

dmlap Apr 1, 2014

Member

The JS ends up using percent units and I didn't want to risk introducing a placement issue when fonts are scaled up or something. Something like left: 4.5em can be made to work, so if you would prefer the units to be consistent with a slight danger of breakage, I can make that change.

@dmlap

dmlap Apr 1, 2014

Member

The JS ends up using percent units and I didn't want to risk introducing a placement issue when fonts are scaled up or something. Something like left: 4.5em can be made to work, so if you would prefer the units to be consistent with a slight danger of breakage, I can make that change.

This comment has been minimized.

@heff

heff Apr 1, 2014

Member

The volume bar is 5em, so 4.5em/5.0em = 0.9, so I don't think there should be any difference in placement? On one side it feels a little weird that JS would then switch to percentages, but the percentage doesn't directly map to the volume percentage anyway, since we have to account for the handle width. Using ems makes the mental math easier and would make it possible to do some auto-calculations with .less vars. So that's the way I'd lean if you're cool with it.

@heff

heff Apr 1, 2014

Member

The volume bar is 5em, so 4.5em/5.0em = 0.9, so I don't think there should be any difference in placement? On one side it feels a little weird that JS would then switch to percentages, but the percentage doesn't directly map to the volume percentage anyway, since we have to account for the handle width. Using ems makes the mental math easier and would make it possible to do some auto-calculations with .less vars. So that's the way I'd lean if you're cool with it.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 1, 2014

Member

Very nice, I like this approach. So it's possible that volume is not 100% when the player loads. I don't see anything left in the slider class that reads the current value of whatever it measures when it's first initialized. I think that could be a problem, but would that undo the benefit of what you're doing here?

Member

heff commented Apr 1, 2014

Very nice, I like this approach. So it's possible that volume is not 100% when the player loads. I don't see anything left in the slider class that reads the current value of whatever it measures when it's first initialized. I think that could be a problem, but would that undo the benefit of what you're doing here?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 1, 2014

Member

To expand on that slightly. The potential use case is a default volume or volume preference. For example if you want to remember that the user selected a certain volume, and apply it to the next video they watch also. That used to be possible with defaultVolume, but I think that was killed. Though still a valid use case. For that I guess we could wait for the tech to be ready and then set the volume to the defaultVolume, which would then trigger a volumechange event and update the slider.

Then there's the issue of if video.js is initialized mid-playback in a video. That's probably a strange use case, but I've tried to keep it so video.js can be initialized at any point, specifically so we get the best loading performance (i.e. we don't restart the video or source loading if it's already going). That could be an HTML5-only issue, and if it becomes an issue we could check the volume state when the html5 tech is initialized and fire a volumechange if necessary.

Member

heff commented Apr 1, 2014

To expand on that slightly. The potential use case is a default volume or volume preference. For example if you want to remember that the user selected a certain volume, and apply it to the next video they watch also. That used to be possible with defaultVolume, but I think that was killed. Though still a valid use case. For that I guess we could wait for the tech to be ready and then set the volume to the defaultVolume, which would then trigger a volumechange event and update the slider.

Then there's the issue of if video.js is initialized mid-playback in a video. That's probably a strange use case, but I've tried to keep it so video.js can be initialized at any point, specifically so we get the best loading performance (i.e. we don't restart the video or source loading if it's already going). That could be an HTML5-only issue, and if it becomes an issue we could check the volume state when the html5 tech is initialized and fire a volumechange if necessary.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 1, 2014

Member

Are there any tests we could add for this? It looks like mostly CSS and removing things, so maybe not, but did you see anything around this that could use a test?

Member

heff commented Apr 1, 2014

Are there any tests we could add for this? It looks like mostly CSS and removing things, so maybe not, but did you see anything around this that could use a test?

@dmlap

This comment has been minimized.

Show comment
Hide comment
@dmlap

dmlap Apr 1, 2014

Member

As the patch stands, you would need to do something like trigger an artificial volumechange to update the slider if you were initializing video.js after the video element and had modified the volume level. We could check the player volume when the slider was initializing and fire an update if the volume wasn't 1. That would be pretty close to a "best of both worlds" scenario at the cost of extra logic inside the volume slider. I'd be happy to update the PR with that if you think it's important.

The fact this PR didn't break any tests does indicate that their probably should have been some tests to start with :) I could add those, too. This PR could get pretty big depending on how deep we want to go with that.

Member

dmlap commented Apr 1, 2014

As the patch stands, you would need to do something like trigger an artificial volumechange to update the slider if you were initializing video.js after the video element and had modified the volume level. We could check the player volume when the slider was initializing and fire an update if the volume wasn't 1. That would be pretty close to a "best of both worlds" scenario at the cost of extra logic inside the volume slider. I'd be happy to update the PR with that if you think it's important.

The fact this PR didn't break any tests does indicate that their probably should have been some tests to start with :) I could add those, too. This PR could get pretty big depending on how deep we want to go with that.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 1, 2014

Member

I think I'm ok with waiting to see if the late initialization is an issue. So after changing the css to ems I think this is ready to go.

I'm not sure what tests could be added exactly but if you see some that would be valuable feel free to add them.

Member

heff commented Apr 1, 2014

I think I'm ok with waiting to see if the late initialization is an issue. So after changing the css to ems I think this is ready to go.

I'm not sure what tests could be added exactly but if you see some that would be valuable feel free to add them.

@paruls

This comment has been minimized.

Show comment
Hide comment
@paruls

paruls Apr 2, 2014

Tested in all browsers. It looks good! In ie8, the icons(play, volume, fullscreen) are missing, but the volume progress bar looks right. I don't see an issue filed for this in vjs.

paruls commented Apr 2, 2014

Tested in all browsers. It looks good! In ie8, the icons(play, volume, fullscreen) are missing, but the volume progress bar looks right. I don't see an issue filed for this in vjs.

@heff heff closed this in aa8d50b Apr 3, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Apr 3, 2014

Member

Fixed the ems in a later commit and pulled this in. @paruls, IE8 is finicky with fonts. If you can consistently get no fonts in IE8, please open another issue with the steps to reproduce and details of the system you're using.

Member

heff commented Apr 3, 2014

Fixed the ems in a later commit and pulled this in. @paruls, IE8 is finicky with fonts. If you can consistently get no fonts in IE8, please open another issue with the steps to reproduce and details of the system you're using.

@dmlap dmlap deleted the dmlap:feature/delayed-progress-ie8 branch Apr 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment