oEmbed plugin

[Piotrek Koszuliński]

Can't wait to use this :)

[Piotrek Koszuliński]

Pushed ​t/10925 with a prototype.

[Marek Lewandowski]

Just a note, from time being we decided to use name media embed (mediaembed in source) for this plugin.

[Marek Lewandowski]

Current solution pushed at ​t/10925 dev and t/10925 tests.

We need still to discuss few things:

ACF integration

Embedded content may contain lots of fancy stuff which is stripped by ACF for instance:

• gist contains tags like <span class"mi">123</span> which are used for syntax highlighting.
• that's bigger issue: twitter injects <script /> tag in order to display formatted date

Currently there is no ACF integration and it strips lot of things.

Our dialog system is displayed behind flash content

Lets say that we imported youtube movie - if movie is in center of the document, then dialog will be spawned behind the video. In the old days i've dealed with such things easily using swfobject, but it was years ago - times may changed and there might be easier solutions for that.

Regexps needs still to be adjusted

Plugin was not working for urls if they had extra stuff in url (in particular these were url query params). I've created a quickfix for it in ​git:be232f2dc999da.

CSS formatting embedded stuff

We need to decide where we want to put CSS formatting the content, Fred stands for putting css into content.css. I've took a look at noembed styles, and there's a lot of stuff, ~16kB ( see ​https://github.com/leedo/noembed/tree/master/share/styles ). I would suggest to reconsider putting it in combined css file, inside media embed plugin.

[Marek Lewandowski]

It should have been mentioned before:

Major features:

Media embed plugin supports two data output formats:

• default - which will produce plain html, ready to be placed at any page
• oembed - plugin will create special tags, containing only oembed resource link, like follows:

  <oembed>https://twitter.com/ckeditor/status/425619710181650432</oembed>
  

Content caching:

In order to avoid extra http calls each time when i.e. users switch to source and back to wysiwyg mode, we should cache results from noembed service.
Additionally in default output mode we already have markup defined, so we even don't have to fetch it from noembed (ofc assuming that end-user did not changed it himself). Therefore we use that markup instead of calling noembed.
Refetching content from noembed will occur if innerhtml of oembed wrapper was stripped, so for instance, user left following code:

<div data-oembed-url="https://twitter.com/ckeditor/status/425619710181650432"></div>

Providers whitelisting:

Using editor#config.mediaembed_providersWhitelist variable you're able to specify which providers are allowed, instead using all of them.

[Marek Lewandowski]

PK stood against idea of linking css for media embed contents by default.

Lets write down pros and cons of both options (please edit this post to insert your thoughts):

Adding css in plugin, attaching it by default

Pros:

• Developer sees nice, formatted oembed stuff the moment he used CKE for the very first time
• Css is encapsulated inside its plugin directory which makes sense
• Requries the least work from developer

Cons:

• Developer will have to insert this css file at his output page anyway
• Extra http request

Adding extra file in plubin, but not attaching it by default

Pros:

• As developoer is required to manually add it into configuration file, he is likely to figure out quicker that he needs to do the same for page
• Css is encapsulated inside its plugin directory which makes sense
• Extra http request, only if placed in configuration - so theoretically we don't have 1 http request for generic package

Cons:

• In worst scenario developer will have 2-step issue, first finding that he has to link css in CKE configuration, and his page. Therefore we must put these two informations next to each other.

Using css inside main contents file (ckeditor-dev/contents.css)

Any1 want this?

[Wim Leers]

Just to make sure everyone understand the CSS problem: I think you are talking about each OEmbed provider's own CSS, right? Not some generic CSS?

[Frederico Caldeira Knabben]

Replying to Wim Leers:

Just to make sure everyone understand the CSS problem: I think you are talking about each OEmbed provider's own CSS, right? Not some generic CSS?

It's about this:
​https://github.com/leedo/noembed/tree/master/share/styles

By default CKEditor will use the HTML produced by providers, but those are unstyled and look ugly in the editor. Therefore we could come with a default piece of CSS to be included in the editor to make things look good.

[Frederico Caldeira Knabben]

Replying to m.lewandowski:

Adding css in plugin, but not attaching it by default

That was my initial proposal, but....

Adding css in plugin, attaching it by default

... after a talk with wwalc and m.lewandowski, I've been totally convinced that this is the best option.

The thing is that is all cases, developers will be required to include this CSS in their website to make data produced with CKEditor to work. We can also ask then to do so in CKEditor as well, but if we do it by default it'll be one step less for developers in terms of configuration and it'll work in the editor out of the box straight. Then, when the content will look ugly on their site, they'll go around to figure out what to do.

In fact, if we don't enable it by default it may happen that many will not even know that it can look beautiful and will just think that the ugly thing they see in the editor is the right way for it.

[Wim Leers]

I see.

As usual, I'm firmly against inline CSS, or the equivalent evil of "inline external CSS". In general, I understand the need for everything to work nicely out of the box on simpler/more static-ish websites. But in this case, I think that's problematic. What if the HTML returned for a certain oEmbeddable thing changes, and therefor the CSS changes? Then you'll have to update the CSS file to work with both the old and the new HTML. Therefor, there would only be linear growth of the CSS files.

---

More importantly, AFAICT, those CSS files are also only necessary because of noembed's custom code that pretends those sites have oEmbed support, even though they don't support it. Hence my question is: is it truly wise to support these? What if those sites sunset the APIs that noembed uses to simulate oEmbed support? Customers would be furious if something stops working today if it worked yesterday. Therefor, I think it's better to support only those sites that actually support oEmbed:

• it's safer: no unpleasant surprises
• it's more efficient: no CSS to be loaded

---

Finally, isn't it also dangerous in general to rely on an external service like noembed.com? Most important sites actually support oEmbed natively… I would personally feel safer knowing that this wasn't relying on a 3rd party service that might go offline at any point in time, and whose copyright notice says that it was last updated in 2011 — 3 years ago!

(That of course doesn't mean this isn't a fine way to get this off the ground.)

[Frederico Caldeira Knabben]

Replying to Wim Leers:

I'm with you on most of your points Wim. Still we need to provide a simple out of the box solution for the general use. In any case, we're making all this configurable for those who will be seriously implementing it:

• There will be always a single oEmbed provider, but this provider can be configured. It can be a local installation of noembed or any other custom implementation (that's probably the Drupal way).

• Many sites oEmbed-enabled don't provide HTML, just data. The key thing about Noembed or custom provider implementations is guaranteeing that HTML is passed to the editor, simplifying the whole thing.

• CSS is needed for the above HTML also because people want to easily customize things. A local installation of Noembed could bring a good enough level of guarantee that the HTML will look the same.

• The CSS file can be configured and is optional.

• The plugin is configurable to produce <oembed> tags with no HTML at all. That's the best option so this whole thing can be controlled by server side implementations. I'm sure that this is the Drupal way.

Anyway, the goal is bringing something that works good enough by default but can be enhanced by power-devs.

[Piotrek Koszuliński]

I raised the question whether CSS file should be attached by default because of two things.

1. It cannot be done easily because it has to be done differently for inline and iframed *editables*. In case of inline ones stylesheet has to be attached to host page and it should be done only once for all editors. In case of framed editable editor.config.contentsCss has to be extended (extending CKEDITOR.config.contentsCss is not a good idea). We lack easy way to attach stylesheet in this kind of situations. Although, this is rather a question whether we should implement a method for this.

2. In every case developer has to figure out that he has to attach stylesheet to the final page. So there will be a surprise anyway and confusion where it is, why it works here and not here, where should I look for help, etc. Therefore we need to document this well anyway. Of course, by attaching it by default we'll be sure that it's going to work at least in samples and as we know first impression is very important. But on the other hand solutions which are too smart are confusing because of that too. The more it does, the more someone has to understand. So combination of the need for attaching stylesheet on target page and confusing automation makes me worried that it's not worth it.

There's one more aspect of this puzzle. As long as this will not be a standard plugin we'll know that developer at least saw the name of this plugin, so it will be easier for him to find its documentation. But when this plugin will become a core one, then I'll opt more for attaching stylesheet by default or even merging it into contents.css. In fact, we may need to go even further and introduce new file that contains contents specific styles which have to attached to target page. The more rich content becomes, the more we'll need this file.

PS. Apart from all of that, the most important thing is that it must be configurable, because many developers will want to change those styles, concatenate them with contents.css, etc.

[Frederico Caldeira Knabben]

Replying to Reinmar:

1. It cannot be done easily because it has to be done differently for inline and iframed *editables*.

We took this in consideration and therefore this should be done for framed editors only... inline editing will still require the developer to configure things on their site. KISS.

As long as this will not be a standard plugin

This is supposed to be available in the Standard distro.

PS. Apart from all of that, the most important thing is that it must be configurable, because many developers will want to change those styles, concatenate them with contents.css, etc.

Definitely.

[Marek Lewandowski]

Added minified css files (sources will be added later on if we will decide if we want to go with these).

With adding update for youtube (opaque issue) i've created these changes in separate branch ​t/10925b, because they are a little bit hacky, since noembed does not provide way to do it.

[Marek Lewandowski]

Handling issues when working with noembed service:

strategy: default

• user adds link, but it returned an error
• oembed wrapper is created (which contains content url)
• it's upcasted to a widget
• error message is being displayed inside a widget (simply as a text)
• what happens if it's downcasted? it's downcasted to wrapper, without any children - that way mediaembed will attempt to reload it again upon next chance

strategy: oembed

We don't really care because all we do in both case is to downcast widget to <oembed>https://gist.github.com/foobar/123456</oembed>

[Wiktor Walc]

Just to not forget: straight after the dialog is opened a dummy request is sent, at least in Firefox (found in t/10925):

​http://noembed.com/embed?nowrap=on&url=undefined&callback=CKEDITOR._.oembedCallbacks[0]

(see the 'undefined' url attribute which makes no sense)

[Wiktor Walc]

Another issue:

1. Open the mediaembed dialog
2. Paste the folllowing URL: ​http://youtu.be/WXIJFzl3olM
3. Switch to source mode, the result is:

   <div data-oembed-url="http://youtu.be/UUncRmVumo0"></div>
   

4. Switch to wysiwyg and source again, the result now is:

   <div data-oembed-url="http://youtu.be/UUncRmVumo0">
   <div class="noembed-embed-inner noembed-youtube"><iframe allowfullscreen="" frameborder="0" height="344" src="https://www.youtube.com/embed/UUncRmVumo0?feature=oembed" width=" 459"></iframe></div>
   </div>
   

Whatever the result should be, one would expect to have always the same result, regardless of the number of switches to source mode.

Possibly related: straight after inserting content into wysiwygarea (even after waiting for the movie to load), the Preview popup does not work properly (the movie does not show up). It starts working after switching to source mode and back.

[Wiktor Walc]

Both issues seems to be resolved now.

Last thing to discuss: why there is no context menu to edit this, considering that for all other objects in wysiwygarea we used to provide a context menu item?

[Wiktor Walc]

1) When using the Amazon provider, switching to source and back twice, I get elements like <img data-cke-saved-src= or <a data-cke-saved-href=

To reproduce, use the following link: ​http://www.amazon.com/Aurora-Plush-Bella-Flopsie-12/dp/B000TKDMYM/ and switch to source and back twice.

2) Second thing: I believe the button name should be CamelCased (MediaEmbed, not Mediaembed)

[Marek Lewandowski]

Pushed to ​t/10925 at dev and t/10925 at tests.

[Marek Lewandowski]

Because of issues reported by Wiktor, i will have to check them tommorow morning, and then push ticket to review.

[Wiktor Walc]

FYI the noembed scripts generate HTML source which looks like a blocker to me. All images are proxied through https://noembed.com/i/

For example when using Twitter: ​https://twitter.com/ckeditor/status/451343945457737729

The resulting HTML contains:

<div class="tweet_image"><a href="http://www.twitter.com/ckeditor" target="_blank">
<img src="https://noembed.com/i/http://pbs.twimg.com/profile_images/2534229690/27xd5v85l46syvxgmobi_normal.jpeg" style="height:36px; width:36px" /> 
</a></div>

which refers to an image served by noembed.com ​https://noembed.com/i/http://pbs.twimg.com/profile_images/2534229690/27xd5v85l46syvxgmobi_normal.jpeg

[Marek Lewandowski]

Pushed to ​t/10925 at dev and t/10925 at tests.

[Marek Lewandowski]

Plugin still needs an icon, Olek says that he will find one when he's done with codesnippet.

[Piotrek Koszuliński]

Blockers and code style

• The sample has much too simple toolbar. It should be the default one like in all the other samples. You can eventually show which icon is the mediaembed icon just like it's done in the placeholder sample.

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L17 DTD needs to be better augmented. All elements that can contain eombed needs to be extended and eombed needs to have '#' defined as allowed child. Without that e.g. <ul><li><oembed>xxx</oembed></li></ul> is broken because editor does not know that oembed may be li's child.

• <style> cannot be a child of <body> - wrong in dev/ and samples/.

• We need a source of used CSS in the repo. Especially that there's a problem with that CSS - .noembed-embed-inner has border-bottom:0 what looks bad. We should fix that, question is whether directly in our repo or we should officially fork the repo from which we took this file? BTW. why do we keep compressed CSS if builder will compress it? (PS. nope, it doesn't compress... but it should.)

• Documentation for config options isn't clear. I don't understand (really) what whitelist does, there's no documentation for providers and there's no list of providers that could be somehow copied and modified by user. In my opinion we shouldn't minify (GCC will do that) the list of default providers and since JSDuck will not find it because it's in private variable it should be copied to the documentation.

• ​https://github.com/cksource/ckeditor-dev/blob/8272a6a81c6592dc187d5dc5344736c6504b4e06/plugins/mediaembed/plugin.js#L195 we can't document it in JSDuck unfortunately, because that would break widget documentation.

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L105 - is the script tag removed?

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L114 - this is needed only in case of the default provider.

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L122 - we need a better name - "upcast" won't be understandable for others.

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L192 - why initializing by empty object? Following methods should be defined in it.

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L203 - why caps?

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L258-L266 - what is this code doing? A comment should explain what it is meant to do and why, no how it does this (good example: ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L272).

• There's no documentation for config.mediaEmbed_output. And the documentation for pasteStrategies and outputStrategies does not have any value for developer because it does not explain how it is related to the config option, the config option is not explained in terms of what does it change, and there's no explanation of what one can achieve adding new strategies. That's why we exposed them, so that must be documented.

• The CKEDITOR.plugins.mediaembed should be marked as singleton.

• The sample does not show mediaembed features. I miss stuff like - "paste this link and see what happens", "see - it can produce <oembed> tags", etc. The whole oembed concept needs to be explained too.

Important (can be done after first release)

• The integration with undo manager is not good. For example - in empty editor insert one ME, then doubleclick it and change URL to some other source, close the dialog. Now, start undoing and you'll see that you need to undo twice to get "through" the first ME.

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L63 - adding this class may be recorder by undo manager - update snashot after doing that.

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L244 - not a good idea to involve parser for this simple task - you might use htmlParser.element&text constructors - much lighter, much wow.

Future

• It would be good to extend DTD only when at least one editor has output strategy oembed, but it may be tricky.

• Pasting http://instagram.com/p/lCdFHPGG97/# on IE creates ME, but leaves # as text. This is caused by... IEs bug - it does not include the # in pasted <a>.

• ​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L298 - it may make sense to parse links on all browsers - some may generate links when copying URL from URL bar or a link could be copied from e.g. email sent to the user.

[Marek Lewandowski]

Questions:

​​https://github.com/cksource/ckeditor-dev/blob/8272a6a81c6592dc187d5dc5344736c6504b4e06/plugins/mediaembed/plugin.js#L195 we can't document it in JSDuck unfortunately, because that would break widget documentation.

• why would that break jsduck doc?

​​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L105 - is the script tag removed?

• yes it is ( ​https://github.com/cksource/ckeditor-dev/blob/8272a6a81c6592dc187d5dc5344736c6504b4e06/plugins/mediaembed/plugin.js#L92 )

​​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L192 - why initializing by empty object? Following methods should be defined in it.

• i've simply found it to be much more readable

​​https://github.com/cksource/ckeditor-dev/tree/8272a6a/plugins/mediaembed/plugin.js#L203 - why caps?

• it was done that way initialy, as capsed variables are widely considered to be constant, i found it ok to be capsed

todo:

• There's no documentation for config.mediaEmbed_output. And the documentation for pasteStrategies and outputStrategies does not have any value for developer because it does not explain how it is related to the config option, the config option is not explained in terms of what does it change, and there's no explanation of what one can achieve adding new strategies. That's why we exposed them, so that must be documented.

yes i belive we need to spent some time to make good overview of concept, features and extending possibilities.

• The sample does not show mediaembed features. I miss stuff like - "paste this link and see what happens", "see - it can produce <oembed> tags", etc. The whole oembed concept needs to be explained too.

i will focus on the sample now, to bring thing which you've mentioned

• things from "Important (can be done after first release)" and "Future" headlines

[Marek Lewandowski]

I've ported MediaEmbed tests to Bender and rebased it in branch ​t/10925.

After quick chat with Piotrek Koszulinski we came with 3 enhancements which I will take care of:

• cache - we should cache content downloaded with MediaEmbed
• undo - better integration with undo, see issue pointed by PK ​earlier
• timeout - currently there is no indication when service will fail to return content nor when timeout will occur. I think it's very important UX violation we should take care of (with little effort).

[Wim Leers]

Where are you going to cache? In window.sessionStorage?

Regarding timeout: will the user have the ability to retry? Or will e.g. one retry be attempted automatically?

[Marek Lewandowski]

As for cache, we're considering temporary caching at runtime. We want to avoid long-term caching, since the resource may easily be outdated, so it's good to load the item at least once during the editing session.

As of retrying, we were considering adding such a feature, but at this point we can't guarantee it will make it into plugin's first release/iteration.

[Marek Lewandowski]

Pushed to ​t/10925b at dev.

You might also take a look into ​plugins/mediaembed/dev directory, there are some samples how to customize filtering / content / querying.

THe only thing we still need to discuss is handling of Twitter. Twitter sends a HTML representation along with a script tag. Script currently is not evaluated inside the editable, but it is at preview. That's not a wysiwyg, so we need to decide one of these two ways.

There is one test failing when run with whole suite. If ran separately, it passes. It's in ​test sendJsonpRequest removes listener I'll deal with it later.

Edit: I've fixed failing test today morning, so it's fine now.

[Piotrek Koszuliński]

Missing:

• No custom name in the elements path.
• ACF integration. In all dev samples which I checked the content created by mediaembed was filtered out. I recall something that you missed a feature in ACF and I think it was ​http://docs.ckeditor.com/#!/api/CKEDITOR.filter-method-addElementCallback which is already available.
• Sample for the plugin.
• Icon.

To be improved:

• Docs. They are incorrectly formatted and their contents is incorrect as well (make sure to generate them and check the status).
  • BTW. You don't need to specify @member all the time.
  • BTW2. We don't use @type.
• mediaEmbed_disablePaste - the name should be more self-explanatory - e.g. mediaEmbed_pastedLinkDiscovery or mediaEmbed_decoratePastedLinks.
• mediaEmbed_disableCache -> mediaEmbed_cache (as well as the above option this may be "positive" too)?
• mediaEmbed_output -> mediaEmbede_outputStrategy?
• mediaEmbed_provider - why an object? I think that additional information (if anyone will need them in the future) can be set directly in the config object - e.g. mediaEmbed_providerApiKey. We avoid objects, because then you can't just change one of the options - you need to set entire object.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L30 - unnecessary white spaces.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L60 & ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L58 - you sure?
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L43 - why priority 100? If you want to say "at the end", then we usually use 999.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L78 - the cache variable is defined so far from this place that it's wrong. And why not just making it the mediaembed.cache's private property?
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L103 - editor.document.getDocumentElement()
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L100 - this looks odd. We're putting editor in the busy state, but loadingCalls is shared between all editors (and it's again defined somewhere far far away and below that code).
• You defined some methods in the editor.plugins.mediaembed object, but that's unfortunately a bad idea (though, provoked by our API). As you perhaps noticed yourself - you must pass the editor instance to all of them because the plugin object is shared between editors. At the same time you defined CKEDITOR.plugins.mediaembed as the global and static scope for some other methods. These are good because they can be documented and are really global and static, so there's no confusion. If you want to have editor specific methods, then you have to do that like in the widgets - define CKEDITOR.plugins.mediaembed as a constructor which instance will be set on editor.mediaEmbed and there you can have editor's specific methods. Static ones can still be kept in the CKEDITOR.plugins.mediaembed constructor.
  • BTW. I believe that since you fire events on the editor.plugins.mediaembed object, which is shared between editor instances, then it's hard to handle stuff differently depending on editor instance. Besides, this is just very confusing.
• What's the difference between insertWidget and insertMediaEmbed methods? I think that the latter is confusing.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L372 - properties go before methods.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L469 - the attribute is required.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L541 - just call this.edit().
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L544 - that I believe is the default action, so there's no need to define this listener.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L561 - widgets aren't documentable, so don't use /** */ comments.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L580 - the paste listener does strange thing - it sets data.dataValue to an empty string. Why does it do that? This, after recent changes in the clipboard plugin, makes the paste process to be canceled, so there's no afterPaste (and that's why pasting tests fail). As I see it - you should only modify the data.dataValue to be a proper data format of the widget that should be created - all this can be done synchronously and right in the editor#paste listener.
  • Unless the problem is that we don't know which link we can replace without requesting the provider. But in this case I believe we must do it differently. We can't affect every link paste and editor should be responsive, what means that we can't delay the moment when something appears in the editor (or editor clearly changes state to "waiting"), because when user does not see a response (s)he tends to repeat action that didn't have a result.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L598 - better if the event name could be associated with pasting, because inserting is something else - it's done later and is more general.
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/dev/customrequesting.html#L38 - do we need the evt.data.requestFunction? Can't this be done like here ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/dev/customfilter.html#L39
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/dev/changingprovider.html#L46 - is there also a simple way to change the accepted links validator so error is thrown while doing dialog validation?
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L276 - so we throw an error... is it handled somewhere?
• Do we need the inline image styles in the oembedAdjustmentListener? Can't the max-width be moved to contents.css? Do we need height at all?
• setProvider and getProvider - I don't think we need them at this stage.
• fireMediaRequested - I find this method confusing (it's too long and the name is vague).
  • First of all, I would avoid adding "media" to same names like mediaRequested event or insertMediaEmbed - be a little more general.
  • Second of all, the method name should tell what it does from an external POV, not explain its internals. Here you say that it fires mediaRequested event, when this is an implementation detail. Say for instance that it requests provider and if it does more than you can explain in the method's name, then it means that you need to create more methods.
  • Be consistent - why using function declaration and function expression next to each other?
• It is impossible to reuse this widget. One of the use cases we've been thinking of is that there should be an "oembed widget" factory which would be used by us to create the mediaembed widget, while other developers could use it to create other widgets for including other types of content. The thing is that that content may come from different providers, hence that setting as well as most methods and events should be bound to the specific implementation - not to the general "oembed widget" or the editor instance. As I see it:
  • Factor (CKEDITOR.plugins.oembedwidget kept in a separate plugin) creates a widget definition with all the general methods and sets the provider inside this widget. This plugin should also expose methods needed to integrate features like paste discovery.
  • Media embed plugin creates media embed widget using the factory and defines all media embed specific features.
  • I left this point at the end of the list, because it may be too late to make such changes. In this case we could at least analyze what's possible now and how could we quickly improve the situation. For example I noticed that mediaembed widget definition is created by a private function and that's wrong, and that if we introduce stateless CKEDITOR.plugins.mediaembed which will be used to create statefull editor.mediaEmbed, the situation may very quickly improve.

[Piotrek Koszuliński]

I pushed a rebased ​branch:t/10925b plus some additional commits.

[Anna Tomanek]

Tasks for creating the ​developer documentation and ​sample are now tracked separately in appropriate GitHub repositories and should not affect the review of this feature.

As discussed with Wiktor, we will not be adding a new Media Embed sample in the /samples/ folder for CKEditor 4.5 -- the new sample will be created directly in the SDK. This does not apply to the dev samples.

API docs are still a part of this ticket and should be reviewed in this ticket.

[Marek Lewandowski]

Pushed to ​t/10925b.

Replying to Reinmar:

Missing:

• No custom name in the elements path.

Done.

• ACF integration. In all dev samples which I checked the content created by mediaembed was filtered out. I recall something that you missed a feature in ACF and I think it was ​http://docs.ckeditor.com/#!/api/CKEDITOR.filter-method-addElementCallback which is already available.

Done.

• Sample for the plugin.

It was supposed to be extracted to the SDK. I've ported a sample that was removed some time ago to ​SDK. That resulted with ​#123 task.

• Icon.

Done.

To be improved:

• Docs. They are incorrectly formatted and their contents is incorrect as well (make sure to generate them and check the status).

Updated builds well with sdk.

• BTW. You don't need to specify @member all the time.

Reduced @member occurrences.

• BTW2. We don't use @type.

Removed.

• mediaEmbed_disablePaste - the name should be more self-explanatory - e.g. mediaEmbed_pastedLinkDiscovery or mediaEmbed_decoratePastedLinks.

Lets keep it simple, given names are too complex. mediaEmbed_disablePaste says exactly what it does.

• mediaEmbed_disableCache -> mediaEmbed_cache (as well as the above option this may be "positive" too)?

It's good to keep names in similar fashion, and this one is similar to disablePaste.

• mediaEmbed_output -> mediaEmbede_outputStrategy?

output is simple enough. I feel like strategy postfix does not add anything new except for few letters.

• mediaEmbed_provider - why an object? I think that additional information (if anyone will need them in the future) can be set directly in the config object - e.g. mediaEmbed_providerApiKey. We avoid objects, because then you can't just change one of the options - you need to set entire object.

Yup, lets keep it simpler. Changed it to a string.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L30 - unnecessary white spaces.

Minified.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L60 & ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L58 - you sure?

Fixed.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L43 - why priority 100? If you want to say "at the end", then we usually use 999.

This one is intended to be simply a lower priority than the default one. So it doesn't matter, but any way changed to requested 999.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L78 - the cache variable is defined so far from this place that it's wrong. And why not just making it the mediaembed.cache's private property?

This way we allow developers to reimplement it as they would like it to be. Be it: cache only particular domain, or given media embed responses, use local storage for multisession cache etc. As for cache declaration positioning, it's used only in this particular place because cache var is used only for cache. Variable itself is pretty much implementation detail so it make sense to keep it at the bottom of the file.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L103 - editor.document.getDocumentElement()

Done.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L100 - this looks odd. We're putting editor in the busy state, but loadingCalls is shared between all editors (and it's again defined somewhere far far away and below that code).

Correct, this was actually a major issue. Fixed.

• You defined some methods in the editor.plugins.mediaembed object, but that's unfortunately a bad idea (though, provoked by our API). As you perhaps noticed yourself - you must pass the editor instance to all of them because the plugin object is shared between editors. At the same time you defined CKEDITOR.plugins.mediaembed as the global and static scope for some other methods. These are good because they can be documented and are really global and static, so there's no confusion. If you want to have editor specific methods, then you have to do that like in the widgets - define CKEDITOR.plugins.mediaembed as a constructor which instance will be set on editor.mediaEmbed and there you can have editor's specific methods. Static ones can still be kept in the CKEDITOR.plugins.mediaembed constructor.

Yup that was very inconvenient at a time of writing. It make senese to instance it with an editor as a property. Done.

• BTW. I believe that since you fire events on the editor.plugins.mediaembed object, which is shared between editor instances, then it's hard to handle stuff differently depending on editor instance. Besides, this is just very confusing.

Events are now fired per MediaEmbed class instance.

• What's the difference between insertWidget and insertMediaEmbed methods? I think that the latter is confusing.

insertWidget focuses solely on inserting widget to a editable, and create a widget instance. insertMediaEmbed is a complete, convenient method for running whole mediaembed plugin logic.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L372 - properties go before methods.

Fixed.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L469 - the attribute is required.

Fixed.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L541 - just call this.edit().

Done.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L544 - that I believe is the default action, so there's no need to define this listener.

Solved with previous change.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L561 - widgets aren't documentable, so don't use /** */ comments.

Done.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L580 - the paste listener does strange thing - it sets data.dataValue to an empty string. Why does it do that? This, after recent changes in the clipboard plugin, makes the paste process to be canceled, so there's no afterPaste (and that's why pasting tests fail). As I see it - you should only modify the data.dataValue to be a proper data format of the widget that should be created - all this can be done synchronously and right in the editor#paste listener.
  • Unless the problem is that we don't know which link we can replace without requesting the provider. But in this case I believe we must do it differently. We can't affect every link paste and editor should be responsive, what means that we can't delay the moment when something appears in the editor (or editor clearly changes state to "waiting"), because when user does not see a response (s)he tends to repeat action that didn't have a result.

Hmm, I don't really remember what was the reason for this approach. Also I did not put any extensive docs on that decission.

Anyway after recent paste API change I've adjusted the code to it, and the job is done within paste event only. I see that undo integration is good, also at IE8 so I find new implementation stable.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L598 - better if the event name could be associated with pasting, because inserting is something else - it's done later and is more general.

It's not really only about pasting, it's also for inserting widget, so general media embed insertion.

It's main purpose is to decide if related URL is valid candidate for a oEmbed content.

The fact is that URL is not really inserted so maybe something like urlRequested sounds better? But then it would be too similar to mediaRequested.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/dev/customrequesting.html#L38 - do we need the evt.data.requestFunction? Can't this be done like here ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/dev/customfilter.html#L39

The key thing here is to provide responseCallback to the developer, so mediaembed process can continue after his logic is done.

Well I don't see why we couldn't pass responseCallback to mediaRequested#evt.data. In fact that might be simpler and more common construction.

Is that ok?

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/dev/changingprovider.html#L46 - is there also a simple way to change the accepted links validator so error is thrown while doing dialog validation?

Yes, urlInserted event is the way to go. See its documentation, there's a listening with that case.

• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L276 - so we throw an error... is it handled somewhere?

No it's not handled. This is a critical error that should not happen.

It should be noted somehow, because any developer that has not been working with mediaembed internals, won't be able to tell what's wrong, and why it's not working.

• Do we need the inline image styles in the oembedAdjustmentListener? Can't the max-width be moved to contents.css? Do we need height at all?

We want inserted images to be responsive as a output, so it's an easy way to do so.

Putting these styles to constents.css would affect only presentation in CKEditor editable.

Also it's very simple.

• setProvider and getProvider - I don't think we need them at this stage.

Gone with mediaEmbed_provider simplification.

• fireMediaRequested - I find this method confusing (it's too long and the name is vague).
  • First of all, I would avoid adding "media" to same names like mediaRequested event or insertMediaEmbed - be a little more general.
  • Second of all, the method name should tell what it does from an external POV, not explain its internals. Here you say that it fires mediaRequested event, when this is an implementation detail. Say for instance that it requests provider and if it does more than you can explain in the method's name, then it means that you need to create more methods.
  • Be consistent - why using function declaration and function expression next to each other?

Method name in fact might be better and more general. Though I fell that media is es essential in case. This plugin is about inserting media objects.

But I have hard times finding better name for it:

• commitMediaCreation - This one describes well the method, but is a little long I'm afraid.
• processMedia - Process actually have less and less leaning nowadays.
• createObject - Actualy says nothing.
• createMedia - Once again, almost no meaning.

From the above I like commitMediaCreation but as I said it seems to be long. Any ideas on possible names?

• It is impossible to reuse this widget. One of the use cases we've been thinking of is that there should be an "oembed widget" factory which would be used by us to create the mediaembed widget, while other developers could use it to create other widgets for including other types of content. The thing is that that content may come from different providers, hence that setting as well as most methods and events should be bound to the specific implementation - not to the general "oembed widget" or the editor instance. As I see it:
  • Factor (CKEDITOR.plugins.oembedwidget kept in a separate plugin) creates a widget definition with all the general methods and sets the provider inside this widget. This plugin should also expose methods needed to integrate features like paste discovery.
  • Media embed plugin creates media embed widget using the factory and defines all media embed specific features.
  • I left this point at the end of the list, because it may be too late to make such changes. In this case we could at least analyze what's possible now and how could we quickly improve the situation. For example I noticed that mediaembed widget definition is created by a private function and that's wrong, and that if we introduce stateless CKEDITOR.plugins.mediaembed which will be used to create statefull editor.mediaEmbed, the situation may very quickly improve.

Actually this implementation is very flexible. Events combined with output strategy provides a lot of room for customization.

We spoke about a case where one wants to use multiple providers. I've covered this case in dev/multipleproviders.html.

---

I've gathered my further questions below, just so it will be easier:

Review questions:

• evt.data.requestFunction thing
• ​https://github.com/cksource/ckeditor-dev/blob/1c341a206c5d2459c93585b7f0e5f28ba4c47732/plugins/mediaembed/plugin.js#L276 how do we want to handle this critical error (assuming that exception is not an option)
• Any good propositions for fireMediaRequested?

My own new ideas

• I think that we can change button group for the button:[BR]It's fair to put it next to the image button, though there is already a ton of buttons. : \
• Change cache to work per editor instance.

Global cache is much more efficient, but you're going to face problems when you'll want to customize html output in only one instance.

[Piotrek Koszuliński]

First few things I noticed:

1. JSHint and JSCS bleed.
2. It should be CKEDITOR.plugins.mediaEmbed, not CKEDITOR.plugins.mediaembed. There are few incorrect examples in the repo, but we use camelCase in the namespaces. Note that documentation needs to be updated too.
3. Above applies to button, widget and command names too - button should be named 'MediaEmbed' and command and widget 'mediaEmbed'. Take the codesnippet plugin for instance.
4. .cke_busy class may conflict with other features. Either we should move this CSS into the core or rename the class to .cke_mediaembed_busy. I think that moving this to core is reasonable, because other plugins may need to share this state.
5. CKEDITOR.plugins.mediaEmbed's documentation says that it's "Media Embed Plugin", while this simply is a constructor initialising media embedding feature on specified editor. It is implemented by the mediaembed plugin.
6. Please link to properties and events in the documentation. In some places names are only marked with the backticks, but they should also be linking.
7. We do not use backticks together with {@link ...}. Link is enough.
8. fireMediaRequested is a mix of function declarations and expressions and it's too long. It does too many things. Start from applying the single responsibility principle and names will be easier to find.

[Marek Lewandowski]

Pushed to ​t/10925b.

Replying to Reinmar:

First few things I noticed:

1. JSHint and JSCS bleed.

Done.

1. It should be CKEDITOR.plugins.mediaEmbed, not CKEDITOR.plugins.mediaembed. There are few incorrect examples in the repo, but we use camelCase in the namespaces. Note that documentation needs to be updated too.

Done.

1. Above applies to button, widget and command names too - button should be named 'MediaEmbed' and command and widget 'mediaEmbed'. Take the codesnippet plugin for instance.

Done.

1. .cke_busy class may conflict with other features. Either we should move this CSS into the core or rename the class to .cke_mediaembed_busy. I think that moving this to core is reasonable, because other plugins may need to share this state.

Yea, I agree that this behaviour might be desired in other parts of CKE, though it would be a little more complex.

I've changed CSS class name for mediaembed and created a feature ticket #12755 in CKE .

1. CKEDITOR.plugins.mediaEmbed's documentation says that it's "Media Embed Plugin", while this simply is a constructor initialising media embedding feature on specified editor. It is implemented by the mediaembed plugin.

Headline updated.

1. Please link to properties and events in the documentation. In some places names are only marked with the backticks, but they should also be linking.

The only case that I see is urlInserted in General Implementation section of namespace docs. There are links to events right below.

Ideally we'd like to limit ourself up to 1 link for a given resource in a section (readability, a11y). Since all events are linked in list, it's the best place to link urlInserted event.

1. We do not use backticks together with {@link ...}. Link is enough.

Done.

1. fireMediaRequested is a mix of function declarations and expressions and it's too long. It does too many things. Start from applying the single responsibility principle and names will be easier to find.

Even with logic extraction, there would be still a need to wrap it in a single method, because it's a common operation to do it all at once. I've renamed it to _commonErrorHandling.

[Marek Lewandowski]

Just an update, branch is now moved back to ​t/10925 and all old branches has been removed.

[Marek Lewandowski]

Alright lets review current solution, pushed to ​t/10925.

## What's missing in t/10925 branch?

### Disabling Other Media Embed Plugins

There's no way to disable base plugin (mediaembed) when the inheriting plugin (mediaembedsemantic) is on.

I've placed a proposition for this in a separate branch: ​t/10925_disabling.

It assumes that if editor.mediaEmbed will be set to false during beforeInit (so dependent plugins can disable it) the plugin will skip initialization on that editor.

It also applies for deriving plugin, so eg. editor.mediaEmbedSemantical can be set to false and it'd be disabled too.

### JsDuck Comments For Media Embed Semantical

I did not add docs for SemanticProcessor type (added in mediaembedsemantic) on purpose.

1. I think we should not ship mediaembedsemantic by default in CKEditor (so it should be removed from this branch, and moved to a separate plugin repo).
2. This class is added at a runtime (in first pluginDefinition.init call), so there's no guarantee that it will be available.

[Marek Lewandowski]

We decided to rename plugin once again simply to embed. As a result second plugin should be renamed to embedsemantic.

[Piotrek Koszuliński]

Pushed ​branch:t/10925c which is a generalisation of the previous approach. I didn't include support for pasting links which I would prefer to move to another ticket, because it's hard to make it right.

[Marek Lewandowski]

Some initial thoughts before diving deeper into code, please answer to these general questions.

I've placed more ​readable version on Gist.

Review

General

1. embed - if downloading content fails no notification is shown.

1. Open ​simple manual test.
2. Use "Source" button to change source code to follwoing:

<p>Foo bar</p>

<div data-oembed-url="https://twitter.com/w3c/status/583024403987976195foobar"></div>

1. Go back to the wysiwyg mode.

Expected:

• Error notification is shown.
• Widget is downcasted to <div data-oembed-url="https://twitter.com/w3c/status/583024403987976195foobar"></div> HTML that will try to reload it the next time.

1. embed - Widget won't fetch its content if it's empty. If, during upcast, widget does not have any content then it should be reloaded.

1. embed - if your wrapper div has an empty content it won't load anything from the server.

1. I'm missing a convenient way to insert media embed objects with API. Ideally I'd imagine method like:

editor.insertEmbed( mediaUrl, [callback] );

Where callback would take following parameters:

• error - error code, null otherwise
• widget - widget instance if no error occured, null otherwise

Not sure about parameters order, but I've used ​NodeJS fs order.

1. Dat commit message :) Let's reword it. E.g. "Initial embed plugin concept".

Revision: 283631355dc426181daefbaa9ec44711e4c25675
Author: Piotrek Koszuliński <pkoszulinski@gmail.com>
Date: 3/17/2015 8:08:46 PM
Message:
TMP
----
Added: plugins/embed/icons/embed.png
Added: plugins/embed/icons/hidpi/embed.png
Added: plugins/embed/plugin.js
Added: plugins/embedbase/dialogs/embedbase.js
Added: plugins/embedbase/lang/en.js
Added: plugins/embedbase/plugin.js
Added: plugins/embedsemantic/icons/embedsemantic.png
Added: plugins/embedsemantic/icons/hidpi/embedsemantic.png
Added: plugins/embedsemantic/plugin.js
Added: tests/plugins/embed/embed.html
Added: tests/plugins/embed/embed.md
Added: tests/plugins/embedsemantic/embedsemantic.html
Added: tests/plugins/embedsemantic/embedsemantic.md

UX

1. We need to provide some sort of feedback in embed dialog that it's fetching the content. Now we're doing nothing, and user thinks that his action does not do anything.

We should think about placing a spinner or at least disabling OK button during the process.

2. User needs to be able to cancel widget embeding, currently it's impossible:

1. Open CKE with Embed plugin.
2. Open dialog for widget inserting.
3. Paste an URL of resource that has not yet been cached.
4. Move keyboard focus to OK button with tab key.
5. Do a following sequence:
   1. Press space (start a request).
   2. Press tab (move to cancel button).
   3. Press space (cancel fetching).

Expected: Dialog should be hidden, widget fetching should be aborted.

Actual: Exception: Uncaught TypeError: Cannot read property 'getParent' of null in ​embedbase.js.

1. I'm missing the possiblity to insert GitHub commits:

In example:

URL: https://github.com/quailjs/quail/commit/f73070e241a135fd3815027ffbf9657dd32c5fc8

CKEDITOR._.jsonpCallbacks[115] && CKEDITOR._.jsonpCallbacks[115]({
	"url": "https://github.com/quailjs/quail/commit/f73070e241a135fd3815027ffbf9657dd32c5fc8",
	"type": "link",
	"version": "1.0",
	"title": "imgNotEmpty returns anchor not just img · quailjs/quail@f73070e",
	"provider_name": "GitHub",
	"thumbnail_url": "https://avatars3.githubusercontent.com/u/530687?v=3&s=200",
	"thumbnail_width": 164,
	"thumbnail_height": 164
});

It gives much richer response than "unknown" URLs, e.g. http://www.w3schools.com/html/tryit.asp?filename=tryhtml_basic_paragraphs.

I belive it's due to the fact that GH supports ​ogp.

[Piotrek Koszuliński]

1. embed - if downloading content fails no notification is shown.

Because it does not try to load content on data load. It's not embedsemantic (which shows notifications on data load). The embed plugin shows what was saved in the db and that's it. It is not supposed to do more.

2. embed - Widget won't fetch its content if it's empty. If, during upcast, widget does not have any content then it should be reloaded.

3. embed - if your wrapper div has an empty content it won't load anything from the server.

I don't understand why (2 and 3 are the same point).

4. I'm missing a convenient way to insert media embed objects with API. Ideally I'd imagine method like:

You can say the same about every other widget. There are commands which open dialogs, but there's no single method to create entire widget and initialize it. You need to do this using the widgets API. In this specific case I think that some additional documentation would be useful though, because the insertion process is a bit more complicated.

5. Dat commit message :) Let's reword it. E.g. "Initial embed plugin concept".

Oups. I missed it. I'll fix it before closing.

6. We need to provide some sort of feedback in embed dialog that it's fetching the content.

Totally agree. We cannot show the notification unfortunately and that would be the best option... Or can we? We could improve notifications so they can be opened over dialogs. That would solve the problem completely. Anwyay, a topic for a separate ticket.

7. User needs to be able to cancel widget embeding, currently it's impossible:

Good point. It's topic for a separate ticket.

8. I'm missing the possiblity to insert GitHub commits:

Feature.

[Marek Lewandowski]

Just a memo, sample code to insert a widget by API:

var element = editor.document.createElement( 'div' );
editor.insertElement( element );
var widget = editor.widgets.initOn( element, 'embed' );
widget.loadContent( 'https://www.youtube.com/watch?v=GUl9_5kK9ts' );

Later on it should be noted in docs.

[Marek Lewandowski]

Alright, last part of notes goes to ​gist. I'm going to convert it to Trac markup later on, sorry for inconvenience.

[Piotrek Koszuliński]

We must not keep two different things in one property. It's a case in opts.errorCallback.messageTypeOrMessage, and it was weird to read it at the begining.

Splitting type and custom message will only complicate things further. We will need two properties (which one has higher priority?) and two arguments in every function that accepts it. That's why I decided to merge message and its type together, because there's XOR relation - you either pass one or another. Never two. And the code is simpler, because the logic behind getting error is hidden in a single method.

Maybe we can simplify event names in general?

I decided to not do so. A 'request' error would impose that the request already happen, while the event that we have sends the request. 'validateUrl' is not most beautiful, but I don't see problems with it too. Finally, 'handleResponse' simply matches the other event names which all are verbs in this plugin.

I'd love to see listeners like this extracted:

It's unnecessary. It's flexible enough because this is the last listener which works only if you haven't already handled the response. And you can handle response simply by listening to that event.

Let's think about overriding the default JSON requesting method.

The same situation. There's a very late listener which sends the request. You can easily listen to this event with the default priority and stop it (or cancel - it does not matter) if you handled sending the request. That's all. It's the easiest and cleanest solution. I only changed the event documentation so it does not mention the private method and contains a sample. It should be more user friendly this way.

Do you feel like differences between unsupportedUrl and unsupportedUrlGiven error messages justifies all the complexity overhead? I'd say that going with the same message for both cases is good enough, and it's simple.

I decided to have two different messages for the dialog and for notifications. Obviously, if we wanted to have one message only that would need to be the one from the notification. But the problem with it is that it may be very long due to the URL and it would also be confusing to see the same URL that you just typed in the message - is it the same URL that I typed? Why did they show it to me when it's obvious that the message is about the one I typed?

Reorder parameters for getErrorMessage method and provide default val for suffix.

Done.

_createTask why are we recreating aggregator object?

That's the current API's expected usage.

I believe isUrlValid is not used when adding widgets through API. Instead it's only used in a dialog. I think we should use it to every operation, e.g. in loadContent.

Hm... isUrlValid exists for a UX reason - so we are able to give an immediate response in the dialog if the URL is broken and that someone can customise this. I don't think that loadContent should check the URL. It's a method - it should be executed with the right URL. And the URL will be checked anyway by the provider.

How about giving a way to configure a different provider for embedsemantic? We might change plugin.js to:

Unnecessary. They are not meant to be used together because they are simply a different form of the same feature. But of course it is possible to have different embed widget types using different providers.

It would be cool to briefly describe what provider means. I know that it's obvious for us but might be not simple enough.

Good point. Will do this.

undo test undo and redo after creation seems to be the only one test failing without a browser focus (e.g. when u're running tests with inspector focused). Maybe we can do something about it?

You know how much fun it is to tune and test undo ;).

[Piotrek Koszuliński]

I improved some things based on your feedback and merged the branch to major with ​git:aba11da.

I will report new tickets for:

• things mentioned in comment:50 (points 6 and 7 - UX problems),
• pasting links support,
• widgets repo's method for listening to widget instances' events (it could be a very useful tool).