ckeditor / ckeditor4

The best enterprise-grade WYSIWYG editor. Fully customizable with countless features and plugins.
https://ckeditor.com/ckeditor-4
Other
5.8k stars 2.48k forks source link

check if media embed should try to handle all URLs or only selected that it supports #2306

Open RyszardB opened 6 years ago

RyszardB commented 6 years ago

Type of report

Feature

Provide detailed reproduction steps (if any)

  1. copy url from the browser,
  2. paste into the editor

Expected result

text as a link

Actual result

untitled421

Other details

wwalc commented 6 years ago

It looks like the Media Embed plugin is too aggressive and tries to handle more URLs that it is able to properly embed. It may be also that iframe.ly integration got broken at some point :/

IIRC it used to work better, for example when a link to Amazon product was placed IIRC the product was nicely embeded (but I may be wrong).

mlewand commented 6 years ago

Makes a perfect sens, though I'll make it a feature as all in all that's an addition to current plugin.

Comandeer commented 6 years ago

Seems like Iframe.ly has new feature: support for OpenGraph Protocol → https://iframely.com/embed/https%3A%2F%2Fwww.comandeer.pl%2F

wwalc commented 6 years ago

Yep, Amazon:

iframe.ly demo:

screen shot 2018-08-10 at 11 34 41

Iframe.ly in CKEditor:

screen shot 2018-08-10 at 11 35 03

mlewand commented 6 years ago

That's much probably due to <script async src="//if-cdn.com/embed.js" charset="utf-8"></script>, that probably finds elements with data-iframely-url.

Yup, I can confirm that ☝️. The problem is that this script inserts an iframe into the content, which is something that we'd like to avoid.

There are some info that allows us to recreate the content in some extend, without throwing an iframe, and relay on 3rd party.

However it is missing some information, like a favicon. But we have title, description, UTL and thumbnail - which is enough to create some meaningful media out of it.

f1ames commented 5 years ago

However it is missing some information, like a favicon. But we have title, description, UTL and thumbnail - which is enough to create some meaningful media out of it.

Agree, with @mlewand, since iframely supports it and returns some info it will be good to try to utilize that instead of just blocking some type of responses.

engineering-this commented 5 years ago

We insert response.html into editor, which contains two things. First is the placeholder - a preview, in some cases it looks very close to what final content would look like, and in some cases it looks completely different. Second part is a script, that replaces placeholder with an iframe. But we are removing the script.

How it looks now

Spotify Screenshot 2019-07-09 at 11 26 22 What you see here is just an img that comes with response.html

https://ckeditor.com/ckeditor-4/demo/ Screenshot 2019-07-09 at 11 31 23 This is also an img from response.html.

How it might look when we ignore returned html, and create content by our own

Spotify Screenshot 2019-07-09 at 11 47 14

https://ckeditor.com/ckeditor-4/demo/ Screenshot 2019-07-09 at 11 47 16

Note: styling is to be improved.

Responses

Spotify

{
    'url': 'https://open.spotify.com/track/2XsTC2dPbmWREu5RQ1mQC0',
    'type': 'rich',
    'version': '1.0',
    'title': 'Comfortably Numb - Live',
    'author': 'Pink Floyd',
    'author_url': 'https://open.spotify.com/artist/0k17h0D3J5VfsdmQ1iZtE9',
    'provider_name': 'Spotify',
    'description': 'Comfortably Numb - Live, a song by Pink Floyd on Spotify',
    'thumbnail_url': 'https://i.scdn.co/image/482e1ac3db809e837be42ee3a9cf451474fa5865',
    'thumbnail_width': 640,
    'thumbnail_height': 640,
    'html': '<div style="max-width: 500px;"><div style="left: 0; width: 100%; height: 0; position: relative; padding-bottom: 100%; padding-top: 80px;"><iframe src="https://open.spotify.com/embed/track/2XsTC2dPbmWREu5RQ1mQC0" style="border: 0; top: 0; left: 0; width: 100%; height: 100%; position: absolute;" allowfullscreen scrolling="no" allow="encrypted-media"></iframe></div></div>',
    'cache_age': 86400,
    'options': {
        '_horizontal': {
            'value': false,
            'label': 'Slimmer horizontal player'
        }
    }
}

https://ckeditor.com/ckeditor-4/demo/

{
    'id': '9ptZtgd',
    'url': 'https://ckeditor.com/ckeditor-4/demo/',
    'type': 'rich',
    'version': '1.0',
    'title': 'CKEditor 4 Demo - The battle-tested WYSIWYG HTML editor',
    'provider_name': 'ckeditor',
    'description': 'Demo of the battle-tested rich text editor, when you need even more features and legacy compatibility.',
    'thumbnail_url': 'https://ckeditor.com/assets/images/og/ogimage-ck4-1ea514a336.png',
    'thumbnail_width': 971,
    'thumbnail_height': 509,
    'cache_age': 86400,
    'html': '<div class="iframely-embed"><div class="iframely-responsive" style="padding-bottom: 52.4202%; padding-top: 120px;"><a href="https://ckeditor.com/ckeditor-4/demo/" data-iframely-url="//if-cdn.com/9ptZtgd">CKEditor 4 Demo - The battle-tested WYSIWYG HTML editor</a></div></div><script async src="//if-cdn.com/embed.js" charset="utf-8"></script>'
}

What we can do

None is optimal, so I'd like have some opinions here.

Optionally we could add a button (It would fit perfectly in balloon toolbar) to convert widgets into links and vice versa.

f1ames commented 5 years ago

First response (Spotify) and its response.html doesn't have a script tag:

<div style="max-width: 500px;">
  <div style="left: 0; width: 100%; height: 0; position: relative; padding-bottom: 100%; padding-top: 80px;">
    <iframe src="https://open.spotify.com/embed/track/2XsTC2dPbmWREu5RQ1mQC0" style="border: 0; top: 0; left: 0; width: 100%; height: 100%; position: absolute;" allowfullscreen scrolling="no" allow="encrypted-media"></iframe>
  </div>
</div>

which means the final response shape will not be altered by script.

The second one (ckeditor.com) have a script tag in response.html:

<div class="iframely-embed">
  <div class="iframely-responsive" style="padding-bottom: 52.4202%; padding-top: 120px;">
    <a href="https://ckeditor.com/ckeditor-4/demo/" data-iframely-url="//if-cdn.com/9ptZtgd">CKEditor 4 Demo - The battle-tested WYSIWYG HTML editor</a>
  </div>
</div>
<script async src="//if-cdn.com/embed.js" charset="utf-8"></script>

which means it should be changed by an attached script.

Can we use the response.html to determine if the response should be used directly (no script tag) or if we should compose our custom HTML (script tag)?


One more thing, there are some responses containing different script tags. For example, Twitter links have response.html like:

<blockquote class="twitter-tweet" data-dnt="true" align="center" data-conversation="none">
  <p lang="en" dir="ltr">thx! This library is very nice! I will use it to make better products!🤩</p>
  &mdash; Haruki Tazoe🏍 (@jdkfx)
  <a href="https://twitter.com/jdkfx/status/1146678886643617792?ref_src=twsrc%5Etfw">July 4, 2019</a>
</blockquote>
<script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>

so they both have "proper" HTML and script tag (which is removed making displayed response not as rich as it could be).

So probably we should determine handling the response based on if response.html contains iframely custom script - <script async src="//if-cdn.com/embed.js" charset="utf-8"></script> (could be problematic if the script URL changes)? WDYT @engineering-this? Does it seem doable and reasonable? cc @wwalc

engineering-this commented 5 years ago

Initially I didn't investigate it carefully enough and took some assumptions.

Img which I thought that is a preview of content is actually just our overlay which prevents interactivity of content in editor.

Urls which response.html contain script, but not iframe: twitter, instagram, imgur

Urls which response.html doesn't contain iframe, but does contain iframe: placekitten, youtube, spotify

We might assume, that we need to create our own content only when script src is "//if-cdn.com/embed.js`.

f1ames commented 5 years ago

This issue is a little bit tricky as there are few different approaches how to solve this. We did some additional brainstorming with @engineering-this and there are few options. However, lets first put some context how omitting scripts for embeds affects content in CKEditor itself vs content created via CKEditor displayed on the frontend layer. It seems to be known behaviour but also sometimes considered as an issue (e.g. #2264).

Since embed scripts are not executed inside CKEditor the editable content looks different than CKEditor output displayed on the frontend (because scripts are executed there):

In editor Output
image image
image image
image image

This inconsistency might be problematic because content layout will change, however we haven't seen many issues reported relates to this I think 🤔 Let's get to possible solutions then:

Use omit_script=1 iframely option

Starting from omit_script=1 as I will reference to it later. The omit_script option prevents iframely from attaching script to response.html. Their docs says:

In that case, Iframely will wrap e.g. Twitter, Instagram, Facebook etc into our <iframe ... > code. For providers that already give an iFrame, Iframely will simply ignore that parameter and will follow your other preferences.

and

When you request &omit_script=1, our own script will not be included with HTML code at all and you need to add it to your site yourself. See below.

But from what I tested, embedding Twitter (no iframe and script present) or Spotify works the same, however, custom URLS like https://ckeditor.com works different (better?):

Before: image

After:

image

Pros

Cons

Compose our own content only if response.html contains //if-cdn.com/embed.js script

This was the initial idea proposed in https://github.com/ckeditor/ckeditor-dev/issues/2306#issuecomment-412032049. So we use what iframely gives us - title, description, image and then compose nice embed similar to iframley native one.

Pros

Cons

Attach entire response.html inside sandbox and copy final HTML as embed

This approach somehow appeared during F2F talks and seemed like a neat idea which will allow having the same content in editor and output data. @engineering-this already tested this approach both with iframe and shadow DOM sandboxing (both inside and outside of the editor). However, there are few problems we are unable to overcome (problem with detecting final dimension and moment in time when embed script has finished modifying the content).

Pros

Cons


To sum up the reasonable and low-cost approach is to go with 2nd solution (Compose our own content only if response.html contains //if-cdn.com/embed.js script), which allows to solve this issue rather swiftly. However, I'm for reporting an issue with embed format inconsistency as a follow-up so we may get back to it and maybe came up with some clever solution.

Related #2264, #3132.