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

Fix the video continue player when user drag in timeline #4918

Merged
merged 3 commits into from Feb 12, 2018

Conversation

199911
Copy link
Contributor

@199911 199911 commented Feb 1, 2018

Description

When user seek by dragging the area below the timeline, the video continue playing.
Video should stop when user is seeking
pause-when-seek-before

Specific Changes proposed

Reuse the mouse up and mouse down event handler of seekBar. So the video is paused during seek
pause-when-seek-after-2

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

@gkatsev
Copy link
Member

gkatsev commented Feb 1, 2018

Hey, thanks for making the PR. We haven't done anything to change this behavior, though, I definitely can reproduce this locally with 6.7.0 and cannot reproduce it with 6.6.0. However, I cannot reproduce this with 6.7.1. Were you using 6.7.0? If so, can you retest with 6.7.1? I think I may want to deprecate version 6.7.0 itself and just stick with 6.7.1.

@199911
Copy link
Contributor Author

199911 commented Feb 1, 2018

I can reproduce the problem on 6.7.1
I am using Chrome 63.0.3239.132 in mac OS.
Here is my steps on building a local copy of 6.7.1 player

git checkout master
git pull upstream master
rm -rf dist
cp sandbox/index.html.example sandbox/index.html
npm run build
npm run start
# open http://localhost:9999/sandbox/

PS: the bug only happen when dragging the thin area between the white timeline and control bar, but not the white timeline

@gkatsev
Copy link
Member

gkatsev commented Feb 1, 2018

Oh, I see. The PS is the important part here :)
I can now reproduce even in 6.7.1, I was probably accidentally scrubbing in different areas.

@gkatsev
Copy link
Member

gkatsev commented Feb 1, 2018

Just tried it here (https://deploy-preview-4918--videojs-docs.netlify.com/test-example/) and looks like this is causing an issue when dragging on the seekbar itself now. Seeking in the progress control area will pause and play correctly but now when seeking in the seekbar, it will stay paused when stopped scrubbing.

Use event.stopPropagation() to prevent the mouse down and mouse up events
being handled in both progress control and seek bar.
@199911
Copy link
Contributor Author

199911 commented Feb 2, 2018

@gkatsev
Thanks for your feedback. The bug is should be caused by handleMouseDown() is called twice.

When we drag the seek bar, the handleMouseDown() record the video player is playing.
And then the mouse down event is propagate to the progress control panel, handleMouseDown() is called again and record the video player is paused before drag.
So the player does not resume after drag.

My fix is to stop the event propagate from seek bar to progress control panel, if it is handled by seek bar already.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Awesome, works great now. Thanks!

@gkatsev gkatsev added patch needs: LGTM Needs an additional approval labels Feb 2, 2018
@@ -157,6 +157,9 @@ class ProgressControl extends Component {
*/
handleMouseDown(event) {
const doc = this.el_.ownerDocument;
const seekBar = this.getChild('seekBar');
Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case the seekBar was removed from the player through the player options, you'll want to double check the seekBar component was found before calling seekBar.handleMouseDown()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, I have also checked other function in this component to make sure seekBar exist.

@gkatsev
Copy link
Member

gkatsev commented Feb 10, 2018

definitely doesn't hurt to do the check, though, disabling the seekbar as makes the component useless :)

Copy link
Contributor

@ldayananda ldayananda left a comment

Choose a reason for hiding this comment

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

Great addition, thanks for making the fix!

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

LGTM

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs an additional approval labels Feb 12, 2018
@gkatsev gkatsev merged commit a1cef80 into videojs:master Feb 12, 2018
@199911 199911 deleted the fix/pause-when-seek branch February 13, 2018 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants