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

ustbhuangyi · 2019-09-17T06:27:45Z

Description

First, when handle mouse down event of the seek-bar, we set player's scrubbing_ to true, and at the same tick, the update function called and we call super.update(). However, it returns undefined because the progress equals to the previous progress, the root cause is that we get cached currentTime. So we call super.update() in the next tick to make sure we can get the correct progress.

Second, when toggle visibility of the document, we should not call this.enableInterval_() when the player is paused or waiting.

Fixed #6232

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chome, Firefox, IE)
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

…ck the seek-bar. First,when handle mosue down event of the seek-bar, we set player's scrubbing_ to true, and at the same tick, the update function called and we call super.update(). however, it returns undefined because the progress equals to the previous progress, the root cause is that we get cached currentTime. So we call super.update() in the next tick to make sure we can get the correct progress.Second, when toggle visibility of the document,we should not call this.enableInterval_() when the player is paused or waiting. Fixed videojs#6232

welcome · 2019-09-17T06:27:47Z

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

Run npm run lint -- --errors locally to catch formatting errors earlier.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

gkatsev

So, the way that the progress bar is updated is either with the mousemove event or with a timeupdate event. mousedown does call our mousemove handler, and that's potentially what you're seeing with the value not being set properly. However, we also trigger a timeupdate when we finish the seek on mouseup, which the seekbar should be listening to to update itself. I'd at least expect the timeupdate handler to update things properly.

I guess it broke as part of #5879.

gkatsev · 2019-09-17T06:33:22Z

src/js/control-bar/progress-control/seek-bar.js

@@ -167,8 +168,6 @@ class SeekBar extends Slider {
        this.duration_ = duration;
      }
    });
-
-    return percent;


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

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.

The return value of update function seems not used.

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.

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

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.

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

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

Ok, hope for a better solution.

gkatsev · 2019-09-17T06:34:38Z

src/js/control-bar/progress-control/seek-bar.js

@@ -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')) {


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

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

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

stale · 2019-11-17T03:00:30Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

…is set Updating cache_.currentTime as soon as the currentTime is set avoids having to wait for the timeupdate event, which results in: - making cache_.currentTime more reliable - updating the progress bar on mouse up after dragging when the media is paused. See also: videojs#6232, videojs#6234, videojs#6370, videojs#6372

…is set (#8285) Updating cache_.currentTime as soon as the currentTime is set avoids having to wait for the timeupdate event, which results in: - making cache_.currentTime more reliable - updating the progress bar on mouse up after dragging when the media is paused. See also: #6232, #6234, #6370, #6372

…is set Updating cache_.currentTime as soon as the currentTime is set avoids having to wait for the timeupdate event, which results in: - making cache_.currentTime more reliable - updating the progress bar on mouse up after dragging when the media is paused. See also: videojs#6232, videojs#6234, videojs#6370, videojs#6372

…is set (videojs#8285) Updating cache_.currentTime as soon as the currentTime is set avoids having to wait for the timeupdate event, which results in: - making cache_.currentTime more reliable - updating the progress bar on mouse up after dragging when the media is paused. See also: videojs#6232, videojs#6234, videojs#6370, videojs#6372

…is set Updating cache_.currentTime as soon as the currentTime is set avoids having to wait for the timeupdate event, which results in: - making cache_.currentTime more reliable - updating the progress bar on mouse up after dragging when the media is paused. See also: videojs#6232, videojs#6234, videojs#6370, videojs#6372

* refactor(player): decrease the indentation level in the currentTime method * fix(player): cache_.currentTime is not updated when the current time is set Updating cache_.currentTime as soon as the currentTime is set avoids having to wait for the timeupdate event, which results in: - making cache_.currentTime more reliable - updating the progress bar on mouse up after dragging when the media is paused. See also: #6232, #6234, #6370, #6372 * feat: add an option to handle smooth seeking Adds a player option called enableSmoothSeeking, which is false by default, to provide a smoother seeking experience on mobile and desktop devices. Usage: ```javascript // Enables the smooth seeking const player = videojs('player', {enableSmoothSeeking: true}); // Disable the smooth seeking player.options({enableSmoothSeeking: false}); ``` - **player.js** add an `option` called `enableSmoothSeeking` - **time-display.js** add a listener to the `seeking` event if `enableSmoothSeeking` is `true` allowing to update the `CurrentTimeDisplay` and `RemainingTimeDisplay` in real time - **seek-bar.js** `update` the seek bar on `mousemove` event if `enableSmoothSeeking` is `true` - add test cases

gkatsev reviewed Sep 17, 2019

View reviewed changes

stale bot added the outdated Things closed automatically by stalebot label Nov 17, 2019

gkatsev added confirmed and removed outdated Things closed automatically by stalebot labels Nov 22, 2019

gkatsev self-assigned this Nov 22, 2019

gkatsev added pinned Things that stalebot shouldn't close automatically and removed confirmed labels Dec 2, 2019

This was referenced Dec 23, 2019

Progress control UI is not updating correctly on seek of paused video #6370

Closed

fix: updates seekbar position after mouse up event is triggered. #6372

Merged

gkatsev closed this in cd4076a Dec 24, 2019

This was referenced Mar 19, 2020

[Snyk] Upgrade video.js from 7.4.1 to 7.7.5 videojs/videojs-playlist#173

Closed

[Snyk] Upgrade video.js from 7.6.6 to 7.7.5 videojs/videojs-errors#192

Closed

amtins mentioned this pull request May 20, 2023

fix(player): cache_.currentTime is not updated when the current time is set #8285

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ustbhuangyi commented Sep 17, 2019

welcome bot commented Sep 17, 2019

gkatsev left a comment

gkatsev Sep 17, 2019

ustbhuangyi Sep 17, 2019

ustbhuangyi Sep 17, 2019

gkatsev Sep 17, 2019

ustbhuangyi Sep 17, 2019

ustbhuangyi Sep 17, 2019

ustbhuangyi Sep 18, 2019

gkatsev Sep 18, 2019

ustbhuangyi Sep 18, 2019

gkatsev Sep 17, 2019

gkatsev Sep 17, 2019

ustbhuangyi Sep 17, 2019

stale bot commented Nov 17, 2019

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

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

Conversation

ustbhuangyi commented Sep 17, 2019

Description

Requirements Checklist

welcome bot commented Sep 17, 2019

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Nov 17, 2019