Details
- Reviewers
kpatenio niklas - Commits
- rMOZILLACENTRALcc6fe766b0cb: Bug 1891599 - Create PiP JWPlayer Wrapper r=niklas
- Bugzilla Bug ID
- 1891599
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
I resolved this issue by creating a new nbcnews.js wrapper instead of creating an AOL wrapper.
The JWPlayer embedded on AOL websites operates within an iframe. Consequently, the only way to access JWPlayer captions is by retrieving the 'src' property passed from the iframe (in this case: https://www.nbcnews.com). Note: This iframe issue was identified by Katherine in the previous Yahoo bug - Bug 1890807
This approach now displays Picture-in-Picture (PiP) captions for NBC News videos embedded within AOL websites (e.g. https://www.aol.com/o-j-simpson-former-nfl-145545988.html). Additionally, it has the added benefit of displaying PiP captions for all videos on the main NBC News website.
In the end, AOL videos played using the Video.js player will use the videoJSWrapper and NBC News videos played using the JWPlayer will use the nbcnews wrapper (whether embedded or not).
Because the JW player isn't specific to NBC we should rename the nbcnews.js file to jwplayer.js or something similar
Thanks for the patch @joe.scott.webster, great work! I agree with @niklas that we should rename the wrapper, for it seems that the script is specific to JW Player rather than nbcnews. That way, there won't be any confusion about why a "nbcnews" wrapper would work with another site like AOL. This is actually exciting news because that means we can add support for more sites using the same video player! 😄 I happen to have a list of other sites using JW Player that I'd love to see added in, but we can do that as follow-up work and file another ticket for it.
Some additional remarks:
- Could you please rename your patch, since we're no longer adding an AOL specific wrapper?
- ^ I also suggest renaming the Bugzilla ticket
- I left some inline comments about the wrapper
AOL Captions (JWPlayer playing NBC News video)
NBC News Captions
Just FYI, we don't see the attached screenshots on Phabricator. You might have to change the visibility settings for each image in Referenced Files on the right side.
| browser/extensions/pictureinpicture/data/picture_in_picture_overrides.js | ||
|---|---|---|
| 159 | I also suggest renaming this key in the overrides to jwplayer or similar. The property "https://*.nbcnews.com/*" already implies that this wrapper is compatible for all videos loaded within that site. | |
| browser/extensions/pictureinpicture/video-wrappers/nbcnews.js | ||
| 14 ↗ | (On Diff #850409) | If you disable captions on the original nbc news video while the PiP window is open, PiP captions will be stuck. We should ensure that they're hidden when disabled on the original video.
As a workaround, you might have to modify which element is our container and which is our text in setCaptionContainerObserver, especially if container ends up getting removed from the DOM whenever captions are disabled |
I just changed nbcnews to jwplayer. I'd be happy to change it back if that's what you decide.
This looks good to me! Thanks for working on this!
: We don't have a way to test site specific wrappers
Hey @joe.scott.webster, sorry the delay from my end! I just filed a new ticket https://bugzilla.mozilla.org/show_bug.cgi?id=1893061 for adding more sites under the jwplayer wrapper that you just made. Would you be interested in picking that up?