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: Generate chapters menu only when needed, and don't create orphaned event listeners #7604

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

mister-ben
Copy link
Contributor

Description

The chapters button can create its menu multiple times if several tracks of any kind are present, and each menu item adds an event listener on the track which is not removed when it is replaced.

Specific Changes proposed

Reduces the instances the menu will be created.
Switches to a single event listener on the menu button to manage its menu's selected item.
Fixes #7595

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 (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #7604 (6b1e428) into main (eeda26f) will increase coverage by 0.06%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7604      +/-   ##
==========================================
+ Coverage   80.24%   80.31%   +0.06%     
==========================================
  Files         116      116              
  Lines        7325     7329       +4     
  Branches     1771     1772       +1     
==========================================
+ Hits         5878     5886       +8     
+ Misses       1447     1443       -4     
Impacted Files Coverage Δ
...ar/text-track-controls/chapters-track-menu-item.js 80.00% <ø> (+18.09%) ⬆️
...control-bar/text-track-controls/chapters-button.js 89.06% <92.30%> (-1.68%) ⬇️
src/js/tracks/text-track.js 92.46% <0.00%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eeda26f...6b1e428. Read the comment docs.

@mister-ben mister-ben changed the title Fix chapters fix: Chapters menu shouldn't generate more than needed Jan 11, 2022
@misteroneill misteroneill added the needs: LGTM Needs one or more additional approvals label Feb 14, 2022
@mister-ben mister-ben changed the title fix: Chapters menu shouldn't generate more than needed fix: Generate chapters menu only when needed, and don't create orphaned event listeners Mar 8, 2022
@misteroneill
Copy link
Member

@mister-ben Any notes on a good way to test this manually?

@mister-ben
Copy link
Contributor Author

Just add a chapters track, then another type of any kind, and observe console errors during playback.

7.18.1: https://jsbin.com/refevoseko/edit?html,css,output
This PR: https://jsbin.com/covowukade/edit?html,css,output

@misteroneill misteroneill added confirmed and removed needs: LGTM Needs one or more additional approvals labels Mar 21, 2022
@misteroneill misteroneill merged commit 5af81ca into videojs:main Mar 21, 2022
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
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.

Multiple this.player_ is null errors after switching player source when Chapters file is present
3 participants