search

prebid / videojs-mailonline-plugin

MIT License
11 stars 13 forks source link

videojs 7.x compatibility #44

Open MikeSperone opened 5 years ago

MikeSperone commented 5 years ago

Are there any plans to update this to be compatible with videojs 7? I would love that, as moving from videojs 5 => 7 fixes an iOS issue I have. If there's no work started on this, I would start it and create a PR.

vpitts commented 5 years ago

@MikeSperone We currently have no immediate plans to update this plugin to be compatible with videojs 7. If you would like to start this work, can you first give us a summary of the important differences between videojs 5 and 7. What is the specific iOS issue that 7 is solving?

You can submit PRs to this repository for our review. Before we decide to deploy the upgrade, I will probably have my Test Engineers do some validation of the new version.

Thanks for the contribution. I would also be curious to learn about your prebid video implementation.

MikeSperone commented 5 years ago

The work to update this turned out to be super easy - I only had to update the BlackPoster component slightly. I will create a pull request in the next couple of days.

I forget what the original iOS issue I saw was. I just remember that at the time, the 3rd party player I was using (which was based on videojs v5) had an iOS bug - and I found a commit in videojs v7.1 (I think - maybe even 6.something) that fixed it. At the time, writing a new player based on the latest video.js version was the best option for us.

With this new version of the video player that I'm working on, VPAID ad compatibility was the main focus. So I forgot about that issue until after I implemented this plugin with videojs5. The bug I saw this time was: when the video scrolls into view, the video autoplays (muted) - iOS safari did not detect a user action triggering playback and blocked it. I didn't dig too much deeper as I was sure that updating to videojs7 would fix my iOS issues. And it did!

vpitts commented 5 years ago

Thanks @MikeSperone for the information - glad to hear that the upgrade fixed your issue. We will be looking for your pull request.

Just wanted to make one note: When you load the plugins into the Brightcove Player via the Brightcove Studio, the Brightcove Player always uses its own version of videojs, most likely the latest version of videojs. As a result, the mailonline plugin should be running on player's version in that case. On the other hand, if you are loading the plugin on the page and NOT through the Studio, then the mailonline pluginis running on videojs version had been loaded on the page by script tag: ’ See the Description in theReadme.md file in the mailonline plugin repository

This may or may not impact your particular situation - I just wanted to share that. Please submit your pull request when it is ready. We will be glad to review it.

MikeSperone commented 5 years ago

After comparing my change, I realized it was basically the same as what's already there (but slightly refactored), except with these lines removed from src/scripts/plugin/components/block-poster_5.js:

if (parent && window !== parent && utilities.scriptLoadedInIframe()) {
  BlackPoster.constructor = parent.Object.prototype.constructor;
}

I think I'm not doing either of the cases mentioned in the comments above these lines of code. But for some reason removing this allows this plugin to work with videojs 7 (in my case I'm using videojs 7.4.1). Since these lines are required to fix other issues, I won't make a pull request at this time.

vpitts commented 5 years ago

Ok .. thanks for the update .. let us know if you change your mind. Appreciate your interest in this plugin

MikeSperone commented 5 years ago

No problem! I just didn't want to remove the lines that were put in to fix another issue - especially since it doesn't appear to be an issue that affects my use case (according to the comments) and I don't think I could test to ensure I didn't break that part.