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

fix: update the progress-bar correctly when pausing the video and cli… #6234

Closed
wants to merge 1 commit into from
Closed

fix: update the progress-bar correctly when pausing the video and cli… #6234

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 4 additions & 5 deletions src/js/control-bar/progress-control/seek-bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ class SeekBar extends Slider {
if (document.hidden) {
this.disableInterval_(e);
} else {
this.enableInterval_();
if (!this.player_.paused() && !this.player_.hasClass('vjs-waiting')) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that we should end up paused with vjs-waiting.

Copy link
Member

Choose a reason for hiding this comment

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

Also, seems like this is unrelated to the actual issue, just a nice improvement?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's a improvement.It's no need to call update if the player is paused or waiting.

this.enableInterval_();
}

// we just switched back to the page and someone may be looking, so, update ASAP
this.update();
Expand Down Expand Up @@ -133,9 +135,8 @@ class SeekBar extends Slider {
* The current percent at a number from 0-1
*/
update(event) {
const percent = super.update();

this.requestAnimationFrame(() => {
const percent = super.update();
const currentTime = this.player_.ended() ?
this.player_.duration() : this.getCurrentTime_();
const liveTracker = this.player_.liveTracker;
Expand Down Expand Up @@ -167,8 +168,6 @@ class SeekBar extends Slider {
this.duration_ = duration;
}
});

return percent;
Copy link
Member

Choose a reason for hiding this comment

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

we should still be returning this value as it may be a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

Because the update function is throttled, so even you trigger a timeupdate event manually when finish the seek on mouseup, the update function only called once at the same tick.

Copy link
Author

Choose a reason for hiding this comment

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

The return value of update function seems not used.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah, the throttling is a good point.

While we may not be using it, if someone who uses Video.js is extending the SeekBar themselve and expecting update to return a value, they will break. We should avoid changes that have a potential breaking change in them.

Copy link
Author

Choose a reason for hiding this comment

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

Since the percent depends on the Slider's update function. The return of undefined does not make sense.
https://github.com/videojs/video.js/blob/master/src/js/slider/slider.js#L238-L248

Copy link
Author

Choose a reason for hiding this comment

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

I tried to remove the throttling of the update function, it works, but may cause a little perf issue. So there's a trade-off.

Copy link
Author

Choose a reason for hiding this comment

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

@gkatsev, If you think removing the throttling of the update function is more reasonable, I will send another pr.

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't think we should unthrottle it. I'll have to think about it a bit.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, hope for a better solution.

}

/**
Expand Down