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

Enable youtube and vimeo media players #5314

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

peteruithoven
Copy link

@peteruithoven peteruithoven commented May 21, 2022

Currently inserted media is sanitized away.
When inserting media using the visual editor and html sanitation is turned off only an oembed tag is added.

Based on an existing workaround I'm letting the html-mediaplayers module replace the oembed with an youtube or vimeo ready iframe when it's a youtube or vimeo video.
I've also added a few common iframe attributes to the html-security module, like allowfullscreen.

This change will still require people to enable "Allow iframes", which might be a bit confusing.

This is probably far from the perfect solution but let's get the ball rolling, please let me know what to improve.

Compliments for the easy dev setup, it was super easy to get started.

Update The PR contains a very limited crude approach, in the mean time I've done a bit more research, see the notes below, I'm open for any approach.

@auto-assign auto-assign bot requested a review from NGPixel May 21, 2022 12:04
@peteruithoven
Copy link
Author

peteruithoven commented May 21, 2022

Reading https://oembed.com/ should we do a request to for example youtube to get the right html to inject or info to generate the right embed?

For example:
https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=m9QU69958JU&format=json
Gives me:

{
  "title": "Blade Runner 2049 | After Dark - Mr.Kitty",
  "author_name": "Yudong Wang",
  "author_url": "https://www.youtube.com/channel/UCRHMVllLrevpEEKVNZmXHgw",
  "type": "video",
  "height": 113,
  "width": 200,
  "version": "1.0",
  "provider_name": "YouTube",
  "provider_url": "https://www.youtube.com/",
  "thumbnail_height": 360,
  "thumbnail_width": 480,
  "thumbnail_url": "https://i.ytimg.com/vi/m9QU69958JU/hqdefault.jpg",
  "html": "\u003ciframe width=\u0022200\u0022 height=\u0022113\u0022 src=\u0022https://www.youtube.com/embed/m9QU69958JU?feature=oembed\u0022 frameborder=\u00220\u0022 allow=\u0022accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture\u0022 allowfullscreen\u003e\u003c/iframe\u003e"
}

This does mean we'd need to trust youtube's html or use the info to build our own html.

This also means doing a request while rendering, which is async, which might not be supported as I don't see a mechanism to let the wiki know the module is done with rendering? which is possible since the init is awaited.

We could integrate a library that knows which service and how to query for urls, that would add support for many services.
Comparison: https://moiva.io/?npm=iframely+oembed-parser+oembed-spec
A library like oembed-spec has a local list of possible providers.

What that would look like using oembed-spec:

const oEmbed = require('oembed-spec')

module.exports = {
  async init($, config) {
    for (const elm of $('oembed')) {
      const url = $(elm).attr('url')
      const info = await oEmbed(url)
      if (info && info.html) {
        $(elm).parent().replaceWith(info.html)
      }
    }
  }
}

This would require a bit of additional css to make the players responsive.

If possible we might want to cache the results of these queries for a short while, to speed up the rendering.

@peteruithoven
Copy link
Author

peteruithoven commented May 21, 2022

Actually, I realize that the CKEditor already goes through this process, inside the editor we get a preview. Would be ideal if we could reuse that logic? This would make sure the rendered result matches with the preview. It also prevents issues where support is out of sync between the 2 parts.
One clear downside is that this approach is specific for the CKEditor, so it wouldn't work for the markdown editor for example.

This clarifies the current CKeditor's oembed element output:
https://ckeditor.com/docs/ckeditor5/latest/features/media-embed.html#data-output-format

Looking at the vega branch we'll continue using CKEditor, so doing this for v2 is hopefully also applicable to v3.

This documentation seems interesting:
https://ckeditor.com/docs/ckeditor5/latest/features/media-embed.html#displaying-embedded-media-on-your-website

Following Including previews in data I added the following config to editor-ckeditor.vue:

      mediaEmbed: {
        previewsInData: true
      }, 

This renders players, just like in the editor, for:

  • dailymotion
  • spotify
  • youtube
  • vimeo

It's a small list, but also a very small change.

If we're going with a purely front-end approach this would require that the oembed is no longer filtered out by the html-security module. But that probably requires using a commercial service like embed.ly or iframely.com, which would add complexity for the users of the wiki, like getting an API key. This also requires an additional client side request per embed.

@peteruithoven
Copy link
Author

peteruithoven commented May 21, 2022

For markdown, where we use markdown-it, we'd also need a solution. There for example is markdown-it-oembed, but that also includes the list of providers, meaning another part to keep in sync. The library's last release was more than a year ago, which isn't a good sign.

If we do the oembed > preview ourselves using something like oembed-spec in the html-mediaplayers module, people could add something like this to their markdown:

<oembed url="https://www.youtube.com/watch?v=m9QU69958JU" />

Ideally we add some simple markdown it plugin that just converts something like:

![Getting Started: Sktch.io](https://www.youtube.com/watch?v=CMfBj0U221k)

Into an oembed html element.

And ideally we have some kind of insert media button so people don't have to remember the syntax.

@peteruithoven peteruithoven changed the title Enable youtube and vimeo media players Enable youtube and vimeo media players (oembed) May 22, 2022
@peteruithoven peteruithoven changed the title Enable youtube and vimeo media players (oembed) Enable youtube and vimeo media players May 22, 2022
@peteruithoven
Copy link
Author

peteruithoven commented May 22, 2022

I'd love some input on the general approach. I think we'd want the solution to at least potentially work for all the editors?
This means we sadly can't simply go with CKEditor's previewsInData feature or markdown-it-oembed.
Update: I understand the eventual goal is to switch from CKEditor to Tiptap, which would be another reason not to go that route.

What do we think about adding a library like oembed-spec (alternatives)? This externalizes the actual oembed to iframe conversion logic to a library and using a widely used providers.json file from oembed.com.
But do we even want to support all of those? Seeing we need a bit of CSS to make them responsive, there are a lot of edge cases.

People would still need to enable iframes in the rendering > security config, which isn't exactly intuitive. It's also not ideal that we always show the Insert Media button in the visual editor since it will never work when allow iframes is disabled. People might not understand that embedding video's requires iframes? What if we add some kind of Enable embedding switch that will show/hide the insert media buttons and controls whether iframes can be added? A description could then explains that this enables iframes and some of the security risks.

@peteruithoven
Copy link
Author

peteruithoven commented May 23, 2022

Possibly interesting is how Wordpress handles this.

General docs: https://wordpress.org/support/article/embeds/

They seem to have a hardcoded list of regular expressions with a matching oembed url:
https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp-oembed.php#L53-L107

How they fetch the oembed data:
https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp-oembed.php#L525-L549

Some exceptions of how they handle the result:
https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp-oembed.php#L685-L713
(For video they just take the html for example)

They seem to cache the results?
https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp-embed.php#L231-L234

They have added an API that enables people to add or remove providers.

@RHeynsZa
Copy link

Can we please merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants