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

Mute toggle stops working #3036

Closed
Lumbendil opened this issue Jan 25, 2016 · 16 comments
Closed

Mute toggle stops working #3036

Lumbendil opened this issue Jan 25, 2016 · 16 comments

Comments

@Lumbendil
Copy link

If I press the mute toggle multiple times, it arrives to a point to which it stopps working.

Tested on Ubuntu with both Chrome and Firefox.

Additional info: It seems it gets stuck into such a position because volume gets set to 0, thus making the toggle unoperative. I've found this because when muted without the bug, calling player.volume() returns the volume if it was unmuted, while if it gets bugged, player.volume() reports a 0, thus the toggle does nothing.

@gkatsev
Copy link
Member

gkatsev commented Jan 26, 2016

That's really weird. The mute toggle should not ever be setting the volume to 0. What version of videojs are you using? Also, would you happen to have a reduced test case?

@Lumbendil
Copy link
Author

I'm using the latest tagged version. I don't have much of a reduced test
case, other than spamming the mute toggle really fast until it happens. My
guess is that doing that the tech receives a "mute" signal when the player
is being muted, thus messing it up.

2016-01-26 17:45 GMT+01:00 Gary Katsevman notifications@github.com:

That's really weird. The mute toggle should not ever be setting the volume
to 0. What version of videojs are you using? Also, would you happen to have
a reduced test case?


Reply to this email directly or view it on GitHub
#3036 (comment).

@gkatsev
Copy link
Member

gkatsev commented Jan 26, 2016

I just ran the following test locally in the sandbox and it worked just fine.

var i = 1000;
function loop() {
  player.controlBar.volumeMenuButton.el_.click(); 
  if (i--) {
    setTimeout(loop, 1); 
    console.log(i); 
}}
loop();

Unfortunately, unless you'll be able to get exact steps to reproduce or a working testcase there's nothing we can do.

@gkatsev gkatsev closed this as completed Jan 26, 2016
@FDiskas
Copy link

FDiskas commented Jan 27, 2016

Same for me. videojs version 4.12.15
try something like this

var player = videojs('video-player');
player.volume(0.5);
player.muted(true);
player.muted(false);
alert(player.volume());

And result will be 0 but must be 0.5
Same problem on 5.6.0 version

@gkatsev
Copy link
Member

gkatsev commented Jan 27, 2016

Just tried it with 5.6.0 and did not see this issue:

player.volume()
// 1
player.volume(0.5)
player.volume()
// 0.5
player.muted()
// false
player.muted(true)
player.volume()
// 0.5
player.muted(false)
player.volume()
// 0.5

@FDiskas
Copy link

FDiskas commented Jan 28, 2016

and flash version to?

@gkatsev
Copy link
Member

gkatsev commented Feb 17, 2016

So, I think we've finally been able to track down the issue, thanks to @imbcmdth. If when you're clicking on the mute toggle, you move the mouse slightly, it'll end up changing the slider. This is because the slider is a child of the mute button in the normal control and we want users to be able to use more than just the 4 pixels of the volume slider itself for sliding the volume up and down. We just have to be stricter about not allowing the volume to change when the mouse is being moved on top of the mute toggle itself. May be a bit tricky but should definitely be doable.

@gkatsev gkatsev reopened this Feb 17, 2016
seescode pushed a commit to seescode/video.js that referenced this issue Apr 2, 2016
@gkatsev gkatsev closed this as completed in 4e45d21 Apr 4, 2016
@gkatsev
Copy link
Member

gkatsev commented Apr 4, 2016

@Lumbendil @FDiskas this should be fixed in the 5.8.8 release. Please try it out.

@FDiskas
Copy link

FDiskas commented Apr 4, 2016

Confirmed

@gkatsev
Copy link
Member

gkatsev commented Apr 4, 2016

Awesome, thanks @FDiskas!

jgubman added a commit to jgubman/video.js that referenced this issue Apr 26, 2016
* upstream/stable: (77 commits)
  v5.9.2
  @gkatsev grouped text track errors in the console, if we can. closes videojs#3259
  v5.9.1
  @gkatsev fixed text track tests for older IEs. closes videojs#3269
  revert 75116d4 adding chrome to travis (videojs#3254)
  @forbesjo added back the background color to the poster. closes videojs#3267
  @gkatsev fixed removeRemoteTextTracks not working with return value from addRemoteTextTracks. closes videojs#3253
  @gkatsev made the first emulated text track enabled by default. closes videojs#3248
  @mister-ben blacklisted Chrome for Android for playback rate support. closes videojs#3246
  @benjipott updated IS_CHROME to not be true on MS Edge. closes videojs#3232
  v5.9.0
  @andyearnshaw updated document event handlers to use el.ownerDocument. closes videojs#3230
  @chrisauclair added ARIA region and label to player element. closes videojs#3227
  @MCGallaspy added vttjs to the self-hosting guide. closes videojs#3229
  @forbesjo added chrome for PR tests. closes videojs#3235
  @OwenEdwards improved handling of deprecated use of Button component. closes videojs#3236
  v5.8.8
  @seescode fixed dragging on mute toggle changing the volume. Fixes videojs#3036. Closes videojs#3228
  @seescode fixed css failing on IE8 due to incorrect ie8 hack. Fixes videojs#3140. Closes videojs#3226.
  @vtytar fixed auto-setup failing if taking too long to load. Fixes videojs#2386. Closes videojs#3233.
  ...
@mizmaar3
Copy link

I still can reproduce it on videojs v.5.20.4 @gkatsev according to you its fixed in 5.8.8 !

here: https://googleads.github.io/videojs-ima/examples/simple/
Wait for preroll ad, after clicking on Volume Bar the mute toggle does not work.

@gkatsev
Copy link
Member

gkatsev commented Jan 22, 2018

Can you elaborate on where you're clicking and what you mean by "does not work"?
For me, trying to do what this bug called for no longer occurs. So, if I click and drag the mute button it will not change the volume level but just mute/unmute the player. Clicking on the volume level does not prevent the mute toggle from toggling whether the player is muted or not. Also, in the latest version, a volume level of 0 is separate from a muted state so it's easier to tell that something weird is going on.
In Video.js version 6, we even combined muted state and volume 0 because from a UI and user perspective, it's better to have them be equivalent. So, dragging the volume to zero mutes the player as the same as clicking on the mute toggle. Unmuting puts you back to where you previously had the volume at.

@mizmaar3
Copy link

Im using videojs-ima (google plugin to play vast/vpaid) and unfortunately they still support videojs 5.20.x so cant blame videojs really but if its fixed in 5.8.x I dont expect the issue in 5.20.x or may be its different that above mentioned issue.

I can reproduce that easily (im using chrome btw) its not about dragging mute button, instead if you drag or click on Volume Level Bar once and then click on mute icon the mute icon does change Volume Level to 0 but video does not mute neither the unmute icon change to mute icon. If you manually click on far left to the Volume Level Bar to make it 0 then video does mute.

Hope I explained it correctly now :)

@gkatsev
Copy link
Member

gkatsev commented Jan 23, 2018

that is a different bug then. also, is this during the ad? if so, in the example you've given the ad has its own controls so it's probably a bug with the ad or videojs-ima.

@mizmaar3
Copy link

yes its during the ad so most probably I shud report it to videojs-ima.
Thanks for clarification @gkatsev 👍

@gkatsev
Copy link
Member

gkatsev commented Jan 23, 2018

you're welcome.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants