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

Audio Described displayed next the AD icon #7556

Closed
amtins opened this issue Dec 6, 2021 · 3 comments · Fixed by #7559
Closed

Audio Described displayed next the AD icon #7556

amtins opened this issue Dec 6, 2021 · 3 comments · Fixed by #7559

Comments

@amtins
Copy link
Contributor

amtins commented Dec 6, 2021

Description

ad-bug

Steps to reproduce

  1. Open https://videojs-http-streaming.netlify.app
  2. Start the playback
  3. Click on the audio track icon

Results

Expected

The Audio Described should not be displayed.

Actual

The Audio Described is displayed next the AD icon.

Additional Information

Maybe something wrong here v7.14.3...v7.15.0

versions

videojs

Since version 7.15.0. Version 7.14.3 it works like a charm.
@video-archivist-bot
Copy link

Hey! We've detected some video files in a comment on this issue. If you'd like to permanently archive these videos and tie them to this project, a maintainer of the project can reply to this issue with the following commands:

@gkatsev
Copy link
Member

gkatsev commented Dec 6, 2021

Thanks for the issue. It's likely related to #7337. I thought I went through and verified everything, but I guess I missed a use case.

gkatsev added a commit that referenced this issue Dec 7, 2021
@gkatsev
fix: regression with AD audio track menu items
4c53a7d 
In #7337, a lot of code was updated to no longer user innerHTML, but we
accidentally caused an issue with Audio Description (AD) tracks where
the track title was included twice. Once before and once after the AD
icon.

This is because we were calling `super.createEl()` but MenuItem created
a specific element and didn't just pass things the arguments along.
Instead, we should use `Dom.createEl()` directly.

Fixes #7556
@gkatsev gkatsev mentioned this issue Dec 7, 2021
Merged
@gkatsev
Copy link
Member

gkatsev commented Dec 7, 2021

Have a fix here #7559

gkatsev added a commit that referenced this issue Dec 8, 2021
@gkatsev
fix: regression with AD audio track menu items (#7559)
1d4bad8 
In #7337, a lot of code was updated to no longer user innerHTML, but we
accidentally caused an issue with Audio Description (AD) tracks where
the track title was included twice. Once before and once after the AD
icon.

This is because we were calling `super.createEl()` but MenuItem created
a specific element and didn't just pass things the arguments along.
Instead, we should use `Dom.createEl()` directly.

Fixes #7556
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
edirub pushed a commit to edirub/video.js that referenced this issue Jun 8, 2023
@gkatsev @edirub
fix: regression with AD audio track menu items (videojs#7559)
4986b9f 
In videojs#7337, a lot of code was updated to no longer user innerHTML, but we
accidentally caused an issue with Audio Description (AD) tracks where
the track title was included twice. Once before and once after the AD
icon.

This is because we were calling `super.createEl()` but MenuItem created
a specific element and didn't just pass things the arguments along.
Instead, we should use `Dom.createEl()` directly.

Fixes videojs#7556
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: regression with AD audio track menu items videojs/video.js
3 participants
@gkatsev @amtins @video-archivist-bot