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

Inconsistent media embeds in editor content and its output #3264

Open f1ames opened 5 years ago

f1ames commented 5 years ago

Type of report

Bug

Provide detailed reproduction steps (if any)

Extracted from https://github.com/ckeditor/ckeditor-dev/issues/2306#issuecomment-510122599.

The main provider we are using (IFramely), but also regular embeds use scripts to enrich the embedded content. Since scripts are not executed inside CKEditor, the embedded content looks quite different in CKEditor itself and its output (where scripts are usually executed):

In editor Output
image image
image image
image image

One reason for that is not executing scripts as mentioned above and the other is that CKEditor outputs original iframely response instead of what we have displayed in CKEditor. So there are two ways to make embeds consistent:

Output should use HTML which is displayed inside CKEditor

That will make content consistent and would be easy to implement, however then we lost most of the rich media content (which is now visible at last on the frontend layer) so that would be a step backward IMHO.

CKEditor should be able to show rich embeds

Showing rich embeds in CKEditor itself is another alternative. It would make embeds consistent and always looking up to date (if embedding script changes it will be automatically handled).

The idea is to create some kind of a sandbox (using iframe or shadow DOM) where entire response HTML can be attached, then script will load and enrich the content and then rich content will be moved/shown inside CKEditor. @engineering-this did some research on this:

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).

and this can be rather tricky to implement, however I think it could be doable - using a combination of a different event listeners and some simple heuristic maybe ๐Ÿค”


Related issues #2264, #3132.

engineering-this commented 5 years ago

Few words from me.

When trying to execute script in sandbox biggest issue was how to detect when script fully executes. Waiting for script onload, and adding timeout wasn't sufficient, as some scripts took even longer to execute, they might be fetching some more data. There are so many media embed providers, that we can't possibly check them all script by script, to provide solution for each one of them. I've tried to detect when iframe resizes, and use it as a sign of script execution, however some widgets doesn't resize at all, so it wont work. I've thought about listening somehow to DOM changes, however there is no guarantee that all changes will be synchronous.

When trying to execute script inside editable the issue is that script should be loaded after content, so it will replace it. This might not work very well with undo/redo, going to source mode, copying widgets. Also this leads to risk of script breaking editor somehow.

What's more some sites eg. Twitter, uses shadow root for their widgets, which leads to even more complications, so we will need a way to copy contents of shadow root in order to restore widgets content without executing script over and over again. Shadow root can be used with mode: 'closed', in that case we probably won't be able to clone it in any way.

iparamonau commented 5 years ago

Hey guys, @f1ames @. Ivan from Iframely here.

I see you've dipped your toe into joy if dealing with intricates of the embed codes.

The preview issue is known for as long as our integration exists. We even guide people to build a workaround for it.

There can be numerous different solutions for the issue. The users are certainly able to implement what works best for them, but the main blocker is that CKEditor keeps the gate shut by not running any JavaScript.

If you guys could just give a simple callback that takes URL, html of embed code, and a div where it is (or has to be) put - that should be sufficient for all cases in our experience.

In case of Iframely hosted embeds, it would be as simple as running iframely.load(); in the callback (our embed.js can be added to the page separately off our CDN or even as self-hosted package).

In case of other previews - users could build and host their own iFrame (similar to Iframely) that takes HTML and posts messages to the parent when it's time to resize. So in preview they would simple link to that hosted iFrame (for example, it is known as '3p iFrame' in AMP framework).

The main challenge for you guys, as far as I understand, would be to implement the shadow in such a way that callbacks don't affect the history manager of CKEditor.

By the way, you may also want to check how the issue is solved in CKEditor 5. Essentially, they skip API calls completely for HTML codes and and build a templated preview using callbacks inside CKEditor itself. The editor produces only the semantic output so that previews and actual embed codes are kept separate - the latter have to be handled inside CMS itself.

f1ames commented 5 years ago

Hello @iparamonau, thanks for your detailed insights! We are aware that it is intended behavior in CKEditor 4 (and you probably went through it with different members of CKEditor 4 team few times ๐Ÿ˜„). As mentioned, we did a small research when working on a #2306 fix and decided to go back to this topic and see if anything can be improved.

There can be numerous different solutions for the issue. The users are certainly able to implement what works best for them, but the main blocker is that CKEditor keeps the gate shut by not running any JavaScript.

This is mostly due to security reasons and won't change so we need to check what alternatives we have.

If you guys could just give a simple callback that takes URL, html of embed code, and a div where it is (or has to be) put - that should be sufficient for all cases in our experience.

I'm not sure I understand correctly, a callback which is fired when embed is ready? Or did you mean something else?

In case of Iframely hosted embeds, it would be as simple as running iframely.load(); in the callback (our embed.js can be added to the page separately off our CDN or even as self-hosted package).

Does it returns a Promise or anything similar so we can detect when the preview was fully loaded? As far as I understand this will require adding embed.js script?

In case of other previews - users could build and host their own iFrame (similar to Iframely) that takes HTML and posts messages to the parent when it's time to resize. So in preview they would simple link to that hosted iFrame (for example, it is known as '3p iFrame' in AMP framework).

Good point, it may make creating generic solution a little bit more difficult if we decide to skip checking resize event. Is there a way to detect if the user own Iframe is used?

The main challenge for you guys, as far as I understand, would be to implement the shadow in such a way that callbacks don't affect the history manager of CKEditor.

Yes, the undo integration is the main issue here, but there are also few other challenges. Btw. do you have any opinion on solution mentioned above - the approach to "render" entire embed outside of CKEditor (in iframe or shadow DOM by attaching entire response HTML) and copy its HTML when it's ready (here is the problem with detecting when it's really ready mentioned by @engineering-this ) to CKEditor (and cache to use on undo/redo). Do you see any potential issues? You probably have some experience when it comes to dealing with 3rd-party embed codes?

iparamonau commented 5 years ago

@f1ames

You don't really need to know when an embedded media has finished loading. We don't know ourselves, for example.

What you do need inside an iFrame is a script that keeps checking for the internal height changes, and then posts a message every time with the new height to the parent page (window.postMessage). Another script on the parent page then changes the iFrame height of message's source (hence our embed.js script). You need such height listener when dealing with iFrame anyway: user can resize the browser, and the widget's height will need to change. And once you have this height-adjuster in place, why not use it for initial load as well?

For the latter, take a look at how CKEditor 5 handles the renders. Here's what we recommend to our users about it: gist, full doc.

Basically, CKEditor 5 lets site owners to script their own preview. So, there's no security concern with user's own code. You could give such a callback after you insert the html embed code. But I see why it might be a challenge to maintain the undo action of CKEditor 4 in that case. Perhaps, you could give people a config option to configure iFrame src template for previews and its original height, taking in {url} and / or {html}.

Such iFrame src template would work best for Iframely customers. Others can self-host, and have their own height-adjuster script on their page, etc.

I do believe that doing iFrame with srcdoc would probably be the best all-in-one solution and worth checking whether it's feasible. But we've also put a proposal to CKEditor 5 team to sponsor and host all default previews in Iframely-hosted iFrame for all CKEditor users. Basically, there will be a default iFrame src that doesn't require an API key with us. Please check with Wiktor about the proposal. In that case, all you'd need to do is listen for the height changes messages from Iframely iFrame (our embed.js is also on NPM).

iparamonau commented 5 years ago

So to cut a long story short. iFrame with the srcdoc is the way to go.

I asked my team to do a quick PoC. Here's the code that has no external dependencies and doesn't run any 3rd party JavaScript on the parent page:

https://jsbin.com/sihayuzome/edit?html,output

Simply change var html= with anything you'd like to test

f1ames commented 5 years ago

@iparamonau thanks for the explanation and the PoC! We will take a look and see if we are able to integrate it with embed plugin ๐Ÿคž๐Ÿ‘

engineering-this commented 5 years ago

The biggest challenge is to not have content rendered again when:

I managed to do it all, by caching references to elements inside iframe, and appending them to new iframe after each of above actions. However, this is not working on IE/Edge. There are few reasons for that.

  1. Some embed providers (e.g Twitter) might create another iframe inside. This is problematic because when iframe is removed from DOM all its contents is removed. We could store references to elements in the same way, however we don't know when it's created. We could use interval and update cache when needed, but it's sounds a bit like an overkill. This doesn't happen in Chrome/Firefox/Safari - Twitter uses shadowroot instead which can be moved around DOM without problems.
  2. Edge/IE allows running code from script tags inside detached iframe. This leads to third party scripts throwing errors when widget is removed, I don't think we can't fix in any way. Other browsers seems to prevent execution of all scripts inside detached iframe, so they don't produce any errors.

That's how it works on Chrome/Firefox:

Aug-01-2019 16-20-47

engineering-this commented 5 years ago

โ˜๏ธ Ad.2 Apparently this leads to more upstream (browser) issues with current implementation.

  1. Open https://ckeditor.com/ckeditor-4/demo/ in MS Edge.
  2. Open dev console.
  3. Paste https://placekitten.com/g/200/300 into editor.
  4. Press undo or cut or delete widget.
  5. Press redo button.

Unexpected

Error thrown by provider script.

Although we don't execute any scripts, they come from iframe included in response.html. By allowing more iframes with scripts inside widgets in Edge we will have more errors, so this is another reason to keep old logic for Edge/IE ๐Ÿ˜ž

iparamonau commented 5 years ago

@engineering-this I'd just recreate the iframe with src if it was deleted (and remember - you only need it when there's script in the html code...).

Waiting isn't such a big issue for users. Your bells and whistles, from the other hand, will never be as perfect as the provider's own code - hence the users will always be a tad unhappy.

While I'm on it - I think I saw it in your code that you check for "if-cdn.com/embed.js". That CDN is actually configured only on CKEditor's demo account. Others will have a default cdn.iframe.ly/embed.js or can even self-host, or give us their CDN domain name...

f1ames commented 5 years ago

While I'm on it - I think I saw it in your code that you check for "if-cdn.com/embed.js". That CDN is actually configured only on CKEditor's demo account. Others will have a default cdn.iframe.ly/embed.js or can even self-host, or give us their CDN domain name...

Good catch @iparamonau, indeed we used this to decided how to process the response (see code). Is there any way to reasonable detect that open graph was used (not custom 3rd-party embed, like Twitter, Spotify, etc) so we can apply custom styling only in those cases?


@engineering-this I think it will be reasonable to go with what we have in the PoC for Chrome, Firefox and Safari and fallback to #3270 in Edge/IE (if we are able to fix the detection mechanism mentioned above). Also could you elaborate a little more on why we need catching here (for undo, clipboard and get/set data) so we can get broader context on this solution comparing to one suggested by @iparamonau:

@engineering-this I'd just recreate the iframe with src if it was deleted (and remember - you only need it when there's script in the html code...).

Waiting isn't such a big issue for users. Your bells and whistles, from the other hand, will never be as perfect as the provider's own code - hence the users will always be a tad unhappy.

iparamonau commented 5 years ago

Unfortunately, there's no reliable way to detect when a summary card is returned in oEmbed response. Embed.js script is added in a number of other cases as well. You could, of course, try and switch to our more detailed /iframely?url= format, but it's just wishful thinking.

Luckily, you don't really need to know when Iframely returns a summary card either. Our cards will work with the srcdoc solution just as well as any other widget.

engineering-this commented 5 years ago

Also could you elaborate a little more on why we need catching here (for undo, clipboard and get/set data) so we can get broader context on this solution comparing to one suggested by @iparamonau:

@f1ames It's optional, it just makes things looks nicer.

Compare gifs below:

Without caching (srcdoc would work the same) no-caching

With caching caching

According to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe srcdoc is not supported in Edge and IEs.

Solution proposed in #3311 works fine on Webkits/Firefox.

While I'm on it - I think I saw it in your code that you check for "if-cdn.com/embed.js". That CDN is actually configured only on CKEditor's demo account. Others will have a default cdn.iframe.ly/embed.js or can even self-host, or give us their CDN domain name...

@iparamonau That's bigger problem now, as I used this solution for fallback. srcdoc or not we need a good fallback for Edge/IE. I'm afraid that it would cost significant amount of work, but it's not guaranteed we can make good one. Maybe omit improvements for Edge/IE and just keep how things currently work. WDYT @f1ames?

f1ames commented 5 years ago

@engineering-this with catching it looks much smoother so ๐Ÿ‘ from me.

I'm afraid that it would cost significant amount of work, but it's not guaranteed we can make good one. Maybe omit improvements for Edge/IE and just keep how things currently work. WDYT @f1ames?

Yes, that's what I was thinking, the fallback for Edge/IE should be just current solution so no improvements here unfortunately. So we should just proceed with this solution for Chrome/FF/Safari and skip Edge/IE.

f1ames commented 5 years ago

Yes, that's what I was thinking, the fallback for Edge/IE should be just current solution so no improvements here unfortunately. So we should just proceed with this solution for Chrome/FF/Safari and skip Edge/IE.

I was wondering if we could somehow detect this Open Graph case differently and still try to use #3270 as a fallback for Edge/IE.

From what I see Open Graph has a specific HTML response structure:

<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>

maybe based on this, we could create some heuristic which will detect that. Still, it may fail if IFramely response format will change some day. Anyway, I'm for finishing #3311 for now and then consider getting back to Edge/IE in #3270 if we find it necessary.

biplobice commented 2 years ago

(รฒ_รณ)