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: Remove extra timeupdate event when progress controls disabled #7142

Conversation

alex-barstow
Copy link
Contributor

@alex-barstow alex-barstow commented Mar 23, 2021

Description

SeekBar.handleMouseUp() is a side-effect of disabling progress controls via ProgressControls.disable(). This is an issue because SeekBar.handleMouseUp() manually triggers a 'timeupdate' event (because it assumes a seek has just taken place). If someone attempts to disable the progress controls before playback starts, a 'timeupdate' will fire potentially before we even have 'loadedmetadata', which can cause problems.

Specific Additions

  • Move the relevant mouse and touch event listener removal that is needed by the disable() method from handleMouseUp() into its own function.
  • Preserve the behavior where ProgressControls.disable() will resume a normal playback state if it is invoked while the player is scrubbing.

@alex-barstow alex-barstow force-pushed the fix-rogue-timeupdate-when-progress-controls-disabled branch from 286db9e to 42cad3a Compare March 23, 2021 00:27
@gkatsev
Copy link
Member

gkatsev commented Mar 23, 2021

a 'timeupdate' will fire potentially before we even have 'loadedmetadata', which can cause problems.

Should we make sure that we have started before we try to trigger timeupdate?

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.

Kind of surprised we don't have any tests for this but I'm not going to make sure write them.

@gkatsev gkatsev merged commit b2336aa into videojs:main Mar 23, 2021
@alex-barstow alex-barstow deleted the fix-rogue-timeupdate-when-progress-controls-disabled branch March 23, 2021 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants