Remove vjs-ad-loading class from player if vast response is empty

videojs / videojs-contrib-ads

A Tool for Building Video.js Ad Plugins

http://videojs.github.io/videojs-contrib-ads/

Other

384 stars 257 forks source link

Remove vjs-ad-loading class from player if vast response is empty #449

Closed mimse closed 5 years ago

mimse commented 5 years ago

If the vast request was empty the vjs-ad-loading class is not removed causing the video to be hidden behind an overlay

incompl commented 5 years ago

vjs-ad-loading is removed automatically when leaving the Preroll state:

https://github.com/videojs/videojs-contrib-ads/pull/449/files#diff-08cd03552ff917b922e42a25e311561eR250

There must be more going on in your empty vast response case. Is it not changing state? If so, the complete solution would be to make sure the state is changed so the full cleanup logic can execute.

incompl commented 5 years ago

Do you have an example I could look at to help identify the issue?

mimse commented 5 years ago

Hi, @incompl

I enabled debug mode and this is the state changes that are happening.

Will try to make an example for you to test with.

mimse commented 5 years ago

Hi again @incompl

As you can see on this screenshot the vjs-ad-loading class is never removed.

To reproduce the issue download the src code from this link

https://drive.google.com/file/d/11EWqbimNlzLCEpJj4LtBZQhzXHJgLKTs/view?usp=sharing

Clicking play and reload the page until you get an empty VAST response.

incompl commented 5 years ago

Ah, it does appear that the plugin is not aware that it transitioned beyond prerolls and that is why the preroll cleanup never happened. In searching your example for the root cause, I found that the cause is autoplay. The autoplay value "true" does not work with this plugin (basically because vjs middleware can't handle play events from the video element). There are two alternatives to address this: 1) call play manually instead of using the autoplay attribute or 2) use the new string values for the autoplay option.

mimse commented 5 years ago

I´ll try that tomorrow. Thank you for looking into this.

mimse commented 5 years ago

After changing how the autoplay option was set. I could not reproduce the issue.

Thank you for helping with this, I´ll close the PR.

incompl commented 5 years ago

Glad I could help!

On Fri, Nov 16, 2018 at 3:36 AM Morten Vestergaard Hansen < notifications@github.com> wrote:

Closed #449 https://github.com/videojs/videojs-contrib-ads/pull/449.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/videojs/videojs-contrib-ads/pull/449#event-1970659789, or mute the thread https://github.com/notifications/unsubscribe-auth/AAM4Z5TGPmClfN7voRmuOSyRgXoGHOf3ks5uvnjxgaJpZM4Yd2H2 .