WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 9 months ago

#28195 closed task (blessed) (fixed)

Preview embeds with wpview and auto embed on paste

Reported by: iseulde Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: needs-testing has-patch
Focuses: javascript Cc:

Description

So I've made a patch entirely based on @wonderboymusic's plugin (http://wordpress.org/plugins/audio-video-bonus-pack/), that has a module to preview embed shortcodes. The only thing I've added is something that adds scripts from the embed code to the TinyMCE iframe (I'm sure there are major concerns...) and something that auto embeds urls. I've just taken the regex used in class-wp-embed.php, not sure what regex suits this situation best. The only problem is that it will embed *any* url. Somehow it should be checked whether the url is embedable, either when preprocessing the pasted content, or (even better I think) when the wpviews are created.

Other issues: whenever I paste/undo/redo the views are recreated which causes the content to flash.

Attachments (24)

28195.patch (5.0 KB) - added by iseulde 13 months ago.
28195.diff (6.0 KB) - added by wonderboymusic 13 months ago.
28195.2.diff (1.9 KB) - added by kovshenin 12 months ago.
28195.3.diff (2.0 KB) - added by wonderboymusic 12 months ago.
28195.4.diff (528 bytes) - added by kovshenin 12 months ago.
28195.2.patch (5.5 KB) - added by iseulde 12 months ago.
28195.3.patch (5.5 KB) - added by iseulde 12 months ago.
28195.4.patch (5.5 KB) - added by iseulde 12 months ago.
28195.5.patch (5.5 KB) - added by iseulde 12 months ago.
28195.6.patch (7.3 KB) - added by iseulde 12 months ago.
28195.7.patch (7.3 KB) - added by iseulde 12 months ago.
28195.8.patch (12.0 KB) - added by iseulde 12 months ago.
28195.9.patch (12.0 KB) - added by iseulde 12 months ago.
28195.10.patch (14.0 KB) - added by iseulde 12 months ago.
28195.11.patch (14.5 KB) - added by iseulde 12 months ago.
28195.12.patch (14.0 KB) - added by azaozz 12 months ago.
28195.13.patch (14.5 KB) - added by iseulde 12 months ago.
28195.14.patch (3.0 KB) - added by azaozz 12 months ago.
28195.15.patch (1.6 KB) - added by iseulde 12 months ago.
28195.16.patch (2.8 KB) - added by azaozz 11 months ago.
28195.5.diff (646 bytes) - added by paulwilde 11 months ago.
border-space-fix.png (1.1 MB) - added by paulwilde 11 months ago.
28195.17.patch (494 bytes) - added by iseulde 11 months ago.
28195.18.patch (1.1 KB) - added by ocean90 10 months ago.

Download all attachments as: .zip

Change History (104)

@iseulde13 months ago

comment:1 follow-up: @wonderboymusic13 months ago

  • Focuses javascript added
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.0

Nice - one caveat, I think jQuery parses scripts as soon as you drop them into .html(), so throwing them in the head would dupe them (? ...)

Thank you for doing this, we should get this in soon

comment:2 in reply to: ↑ 1 @iseulde13 months ago

Replying to wonderboymusic:

Nice - one caveat, I think jQuery parses scripts as soon as you drop them into .html(), so throwing them in the head would dupe them (? ...)

Thank you for doing this, we should get this in soon

No, I think TinyMCE blocks all scripts. At least it doesn't work when I embed a tweet. Adding the script to the iframe's head works for me.

comment:3 @wonderboymusic13 months ago

Boom - I will try to commit this today

@wonderboymusic13 months ago

comment:4 @wonderboymusic13 months ago

28195.diff is an update after testing. This is pretty slick, will even show an embed preview if you paste a URL into the editor.

comment:5 @wonderboymusic13 months ago

In 28358:

First pass at wpview logic for the [embed] shortcode. URLs on a their own line are parsed as well. The toolbar will appear with the "remove" button when the view is clicked. Edit has not been implemented yet.

Props avryl, wonderboymusic.
See #28195.

comment:6 @wonderboymusic13 months ago

#27679 was marked as a duplicate.

comment:7 follow-up: @azaozz13 months ago

This looks fun but don't think it will work well in its current form. Appending arbitrary JS that comes with the embeds in the editor is (very) undesirable.

If the embed is "insulated" in an iframe, all seems good as long as it doesn't touch anything outside the iframe (for example youtube). However when the embed is not insulated, the included JS would affect the editor in unpredictable ways. Don't think this is much of a security concern (we trust the providers). Rather that JS is intended for the front-end and would manipulate the DOM, attach events, etc. outside of the "wrapper" element. For example embedding a tweet appends an <iframe id="rufous-sandbox" style="display: none;"... to the editor body.

To make this work, all embeds will have to be in iframes in the editor. Played a bit with inserting an iframe instead of fetching the HTML with ajax. That would still hit the server simultaneously for all embeds every time the editor DOM is rebuild. Another approach would be to create the iframe with JS when needed, similarly to how the TinyMCE iframe is created. That would either need to do document.write() or append any JS to the iframe head (unfortunately IE doesn't support srcdoc for iframes). However it would be possible to cache the initial HTML instead of requesting it every time.

Both of these would need setting the iframe height after it has been fully loaded.

Last edited 13 months ago by azaozz (previous) (diff)

comment:8 @ircbot13 months ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

@kovshenin12 months ago

comment:9 @kovshenin12 months ago

Hi there! Looking at r28358 and I think the_content is too extreme. That filter is meant to run through the whole content, not just a chunk of it. Many plugins will append/prepend all sorts of stuff, like star ratings, social sharing buttons, author bios, ads. We really don't want those around our embedded YouTube video :)

In 28195.2.diff: remove the generic filter and call it parse-embed instead, use WP_Embed to parse the embed shortcode. The post ID is still needed for caching.

@wonderboymusic12 months ago

comment:10 follow-up: @wonderboymusic12 months ago

28195.3.diff makes sure the response from ->run_shortcode() is passed to do_shortcode() when it matches get_shortcode_regex(), which is what happens when you pass an audio or video file.

comment:11 @wonderboymusic12 months ago

In 28580:

Don't pass embeds through the_content() when trying to render MCE previews, leverage WP_Embed and do_shortcode() instead.

Props kovshenin.
See #28195.

@kovshenin12 months ago

comment:12 @kovshenin12 months ago

There's no need to run that regex an extra time before calling do_shortcode(), which already runs the same regular expression along with a couple pre-checks, that can speed things up significantly. See 28195.4.diff.

comment:13 @SergeyBiryukov12 months ago

In 28587:

Remove redundant get_shortcode_regex() check.

props kovshenin.
see #28195.

comment:14 in reply to: ↑ 7 @iseulde12 months ago

Replying to azaozz

Yeah. Appending arbitrary elements in the editor really messes up the content. So for now it's better to remove the script searching part and just leave the twitter embed as a blockquote, or just don't embed when scripts are found.

An iframe sounds like a good idea, but how do you know when a tweet is loaded?

comment:15 follow-up: @wonderboymusic12 months ago

Here are some sample links for testing:

https://www.youtube.com/watch?v=haunJARHPm4
https://www.youtube.com/playlist?list=PL55CC978D1F679CDE
https://twitter.com/wonderboymusic/status/468600297934561280 <--- oEmbed response is fine, doesn't display anything other than link
https://soundcloud.com/haimtime/haim-falling-duke-dumont
http://instagram.com/p/oMsjcZzI35/ <--- returns wrong URL for image
http://www.mixcloud.com/8_8s/disclosurefriends/
http://vimeo.com/74209333
http://www.dailymotion.com/video/x1fwmf_the-national-start-a-war-a-take-awa_creation
https://www.flickr.com/photos/goodbyepicasso/5160634460/ <--- oEmbed response is fine but renders a link only
http://www.collegehumor.com/video/6949868/hipsters-morning-win
http://issuu.com/vmagazine/docs/v87 <--- requires JS, thus not rendering
http://www.meetup.com/Internet-Club-NYC/
https://imgur.com/gallery/YmWARhC <-- only returns link
https://play.spotify.com/artist/5r5Va4lVQ1zjEfbJSrmCsS <--returning a link
http://rd.io/x/Rl4F5xw-bsh5/
http://www.slideshare.net/drylk/elastic-overview-wordcamp-nyc-lightning-round
http://www.funnyordie.com/videos/41124b5136/between-two-ferns-with-zach-galifianakis-sean-penn
http://wordpress.tv/2011/01/29/aaron-jorbin-what-are-you-saying/
http://www.scribd.com/read/224424561/Angels-Demons <-- this is now a paid service, returns a link only
http://s756.photobucket.com/user/basikp/library/animals <-- these URLs don't even resemble what we try to parse
http://revision3.com/diytryin/turn-a-car-battery-into-an-emergency-power-charger/
Viddler appears to be a paid hosting service now, I can't even find any Viddler hosted videos
http://www.hulu.com/watch/640010
http://johnbrunner.smugmug.com/Concert/Chris-Shiflett-the-Dead/i-LfxzTgB <--- SmugMug appears to have gone mostly paywall - the oEmbed endpoint returns 401 often
http://blip.tv/nostalgiacritic/nc-the-lorax-6873339

comment:16 @wonderboymusic12 months ago

In 28594:

When parsing an [embed] shortcode into a TinyMCE view, don't attempt to append any returned <script>s to the editor's <head>. This affects only a few supported endpoints: Issuu and Twitter.

See #28195.

comment:17 @wonderboymusic12 months ago

Twitter works, but you may get: "Internal SSL engine error encountered during the SSL handshake" - when testing on your local box

comment:18 @wonderboymusic12 months ago

Instagram (currently) is taking about 10 seconds to alternately return 502 Bad Gateway or 200 with a response that contains a broken image.

Our default timeout is 5 seconds.

Might not be the worst idea ever to create our own embed handler for Instagram that fixes both issues:

  • Increase the timeout
  • change the end of the image URL from _7.jpg to _8.jpg after requesting *_8.jpg and confirming that it returns 200.

Both are giant hacks and the internet is the wild wild west.

comment:19 in reply to: ↑ 15 @SergeyBiryukov12 months ago

Replying to wonderboymusic:

http://instagram.com/p/oMsjcZzI35/ <--- returns wrong URL for image

Related: #27237

comment:20 follow-up: @iseulde12 months ago

All the links just return a view with a link for me in the latest trunk...
Maybe I'm doing something wrong?

[embed]https://www.youtube.com/watch?v=haunJARHPm4[/embed]

[embed]https://www.youtube.com/playlist?list=PL55CC978D1F679CDE[/embed]

[embed]https://twitter.com/wonderboymusic/status/468600297934561280[/embed]

[embed]https://soundcloud.com/haimtime/haim-falling-duke-dumont[/embed]

[embed]http://instagram.com/p/oMsjcZzI35/[/embed]

[embed]http://www.mixcloud.com/8_8s/disclosurefriends/[/embed]

[embed]http://vimeo.com/74209333[/embed]

[embed]http://www.dailymotion.com/video/x1fwmf_the-national-start-a-war-a-take-awa_creation[/embed]

[embed]https://www.flickr.com/photos/goodbyepicasso/5160634460/[/embed]

[embed]http://www.collegehumor.com/video/6949868/hipsters-morning-win[/embed]

[embed]http://issuu.com/vmagazine/docs/v87[/embed]

[embed]http://www.meetup.com/Internet-Club-NYC/[/embed]

[embed]https://imgur.com/gallery/YmWARhC[/embed]

[embed]https://play.spotify.com/artist/5r5Va4lVQ1zjEfbJSrmCsS[/embed]

[embed]http://rd.io/x/Rl4F5xw-bsh5/[/embed]

[embed]http://www.slideshare.net/drylk/elastic-overview-wordcamp-nyc-lightning-round[/embed]

[embed]http://www.funnyordie.com/videos/41124b5136/between-two-ferns-with-zach-galifianakis-sean-penn[/embed]

[embed]http://wordpress.tv/2011/01/29/aaron-jorbin-what-are-you-saying/[/embed]

[embed]http://www.scribd.com/read/224424561/Angels-Demons[/embed]

[embed]http://s756.photobucket.com/user/basikp/library/animals[/embed]

[embed]http://revision3.com/diytryin/turn-a-car-battery-into-an-emergency-power-charger/[/embed]

[embed]http://www.hulu.com/watch/640010[/embed]

[embed]http://johnbrunner.smugmug.com/Concert/Chris-Shiflett-the-Dead/i-LfxzTgB[/embed]

[embed]http://blip.tv/nostalgiacritic/nc-the-lorax-6873339[/embed]

comment:21 @wonderboymusic12 months ago

Flickr works, but you may get: "Internal SSL engine error encountered during the SSL handshake" - when testing on your local box

comment:22 @iseulde12 months ago

Nevermind. On the front-end they don't seem to embed either... Something with my local install? Same for 3.9. Worked a couple of weeks ago though.

comment:23 in reply to: ↑ 20 @wonderboymusic12 months ago

Replying to avryl:

All the links just return a view with a link for me in the latest trunk...
Maybe I'm doing something wrong?

I pasted all of the embeds into one post, just now: all render, except for Twitter, Flickr, imgur, Spotify, Scribd, Photobucket, SmugMug

1/2 are due to the local SSL issue, the others are dead services I mentioned here: #28379

comment:24 @wonderboymusic12 months ago

imgur is a weird local issue - the service returns 403, but it worked the first time I tried on one of my prod sites.

comment:25 @iseulde12 months ago

Looks like it has something to to with my local install. Works fine on a proper website. Weird, because two weeks ago it worked fine on the same installs.

comment:26 @wonderboymusic12 months ago

Spotify was due to local SSL, worked first time I tried on prod

comment:27 follow-up: @wonderboymusic12 months ago

@avryl - how fast is your internet? it could be timeouts to some of the services. I have blazing fast Verizon FiOS and Instagram was still taking 10 seconds.

comment:28 in reply to: ↑ 27 @iseulde12 months ago

Replying to wonderboymusic:

@avryl - how fast is your internet? it could be timeouts to some of the services. I have blazing fast Verizon FiOS and Instagram was still taking 10 seconds.

Average. :)
But the link inside the views render after less than a second.

comment:29 @ircbot12 months ago

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.

@iseulde12 months ago

comment:30 @iseulde12 months ago

Okay, let's fix some things here. :)

28195.2.patch

  1. Created a separate view for the embed shortcode and just links. The latter searches for paragraphs with just a link in it and then tries to convert them to a non-shortcode view.
  2. On paste, detect if the cursor is currently in an empty P node and only then try to convert it to a view.
  3. If oEmbed fails (and returns a link), replace the view with the link.

comment:31 @iseulde12 months ago

I think this is much better than wrapping everything in [embed] tags. Now it preserves the original content and it will convert embeds to views form old posts (not just when pasting).

@iseulde12 months ago

comment:32 @iseulde12 months ago

28195.3.patch:

It's probably better to return the original content when oEmbed fails as well, instead of returning a link.
Sorry about the trailing whitespace in the last patch.

@iseulde12 months ago

comment:33 @iseulde12 months ago

The embed view should also not overwrite any content that's already in the node with $().html(). It should be inserted before the ins tag instead. (Now the ins tag is overwritten...)

Added to 28195.4.patch, which includes 28195.3.patch and 28195.2.patch.

comment:34 @iseulde12 months ago

Getting a JQMIGRATE wrning... Trimming the html ($( $.trim( this.getHtml() ) ).insertBefore( $( this.node ).find( 'ins' ) );) solves it.

[Warning] JQMIGRATE: $(html) HTML strings must start with '<' character (jquery-migrate.js, line 41)

@iseulde12 months ago

comment:35 @wonderboymusic12 months ago

When I paste the YouTube URL, I am getting no preview - the test tab shows:

<pre class="wiki">https://www.youtube.com/watch?v=haunJARHPm4</pre>

If I paste into Text tab and then switch to Visual, I get the preview.

comment:36 @iseulde12 months ago

Are you pasting a plain url? For me it works... Maybe you copied a bit of something else with <pre class="wiki">?

comment:37 @iseulde12 months ago

Yeah, when I copy from Trac, TinyMCE receives this: <span>https://www.youtube.com/watch?v=haunJARHPm4</span>...
But the patches use the same regex. We could try to find a way around it.

@iseulde12 months ago

comment:38 follow-up: @iseulde12 months ago

28195.6.patch includes all of the previous patches and detects if there are scripts being added. If so it creates an iframe, stuffs the html in it and resizes it. Just putting it here, not sure if it's all OK.

So ideally the interaction with TinyMCE's DOM should be done by the API. A plugin shouldn't have to do this this.node.insertBefore( dom.createFragment( html ), this.node.lastChild ); to insert content before the ins tag.

I'll also work on a patch so that an error is displayed if the ajax request fails.

comment:39 @iseulde12 months ago

I also think, as I mentioned before, that the toolbar should be inserted automatically as well. Look if there's an edit function and add an edit button. Also, now you'll get an error if the plugin adds an edit button, but not a function... Should check before calling edit().

comment:40 @iseulde12 months ago

Whoops. Now the iframe only works on the first time... The second time the template is used. Working on it... :)

@iseulde12 months ago

comment:41 in reply to: ↑ 38 @azaozz12 months ago

Replying to avryl:

So ideally the interaction with TinyMCE's DOM should be done by the API. A plugin shouldn't have to do this this.node.insertBefore( dom.createFragment( html ), this.node.lastChild ); to insert content before the ins tag.

Yeah, couple of months ago we were discussing to have an "inner wrapper" for views. Then all API elements (toolbar, clipboard, etc.) would go in the view element and the actual view content will be in the inner wrapper. This will make the API better and will prevent plugins from touching needed tags like the "end mark".

That would look something like:

<div class="wpview-element" contenteditable="false" ...>
  <div class="wpview-clipboard">...</div>
  <div class="toolbar">...</div>
  <div class="overlay">...</div> (if needed)
  <div class="wpview-wrap">
    [view content]
  </div>
  <ins data-wpview-end="1"></ins> (regex helper, can be <wbr data-wpview-end="1" />)
</div>

@iseulde12 months ago

comment:42 @iseulde12 months ago

Voilà, another patch. Adds toolbar automatically. Overlay as well. Wraps the content in another div.

comment:43 @wonderboymusic12 months ago

holy crap this looks cool - I am gonna wait til @azaozz or @gcorne blesses all of this

comment:44 @iseulde12 months ago

Cool. :)

It's probably not a good idea to use tinymce.activeEditor, since that might change. I think the editor instance needs to be passed, but maybe it's okay to use tinymce.DOM instead of tinymce.editor.dom?

I'm also not sure if the MutationObserver instance needs to be disconnected when the target is removed. Can't find it in the documentation. I guess it just takes care of itself very well.
Right now, if the browser doesn't support it (IE <= 10 :-( and Safari <= 5)*, the iframe will resize after a second, which should be long enough for a tweet etc. to load.

*https://developer.mozilla.org/en/docs/Web/API/MutationObserver#Browser_compatibility

@iseulde12 months ago

comment:45 @iseulde12 months ago

Refreshed the patch against the latest trunk.

comment:46 @iseulde12 months ago

In trunk we're now doing this -> this.node = node; after the html is set by the API to use it later when we get an ajax response. But this will cause problems if there are two identical embeds. When the API sets the HTML for the second one, and we didn't get an ajax response for the first one yet, this.node will be the second one, and the first preview will be empty.

@iseulde12 months ago

comment:47 @iseulde12 months ago

28195.10.patch works a lot better! :)

comment:48 @iseulde12 months ago

It doesn't tie the node to the view instance, so identical embeds in the same editor work. Should also work for multiple editors now. It also doesn't send an ajax request twice for the same embed.

Now we need to add an uncached error message for when the ajax request fails. There should also be an error message when the embed shortcode is used explicitly and oEmbed fails...

@iseulde12 months ago

comment:49 @iseulde12 months ago

28195.11.patch adds error messages. Needs to be localised...

@azaozz12 months ago

comment:50 @azaozz12 months ago

Yeah, this looks really good :)

12.patch is almost identical to 11.patch, just removes the double P wrapping when embedding URLs and tweaks the regex there a bit ('BeforeSetContent' is fired either after wpautop or after inserting some content as string. So we need to match either <p>url</p> or ^url$).

Thinking we should test this a bit more in all browsers and commit.

Next steps: needs a workaround for when the admin is served over https (the browsers would block iframes served over http). Also, are we going to match the URLs to the oEmbed providers similarly to how they're handles in PHP? That would mean converting the PHP style regex to JS and outputting it from PHP.

Currently when pasting an URL that is not embeddable, we try to embed it, fail, and leave it as string. However while waiting for the ajax response, we insert the view wrapper that adds another block tag, so the URL ends up in another paragraph.

Last edited 12 months ago by azaozz (previous) (diff)

comment:51 @iseulde12 months ago

Currently when pasting an URL that is not embeddable, we try to embed it, fail, and leave it as string. However while waiting for the ajax response, we insert the view wrapper that adds another block tag, so the URL ends up in another paragraph.

Yes, so that's why 28195.11.patch checks if the cursor is in an empty paragraph. I don't think it's a good idea to embed the url in any other case.

@iseulde12 months ago

comment:52 @iseulde12 months ago

I wish it was possible to make a patch for a patch. Would be so much clearer. :)

Anyway, 28195.13.patch adds the following to 28195.12.patch:

		node = editor.selection.getNode();

		// When a url is pasted, only try to embed it when pasted in an empty paragrapgh.
		if ( event.content.match( /^\s*(https?:\/\/[^\s"]+)\s*$/im ) &&
			( node.nodeName !== 'P' || node.parentNode !== editor.getBody() || ! editor.dom.isEmpty( node ) ) ) {
			return;
		}

comment:53 @iseulde12 months ago

It looks like copying embeds doesn't work in Safari.

comment:54 @azaozz12 months ago

In 28748:

wpView:

  • Don't wrap single-line URLs in [embed]. Use them directly in generating a view.
  • If the embedding HTML contains a script, "sandbox" it in an iframe to prevent it from changing the editor DOM.
  • Automatically add toolbar and overlay when needed.
  • Try to embed single-line URLs only if they are pasted in an empty paragraph.

Props avryl, see #28195

comment:55 @iseulde12 months ago

  • Keywords has-patch removed

Cool! :) Couldn't attend the meeting, sorry. Will read the logs.

TODO:

  • Localise the error message;
  • Allow pasting things like <span>https://www.youtube.com/watch?v=haunJARHPm4</span>, so people don't get frustrated when copy pasting a link that's wrapped in an element;
  • SSL.

comment:56 @iseulde12 months ago

Going through undo levels is really weird btw...

comment:57 @iseulde12 months ago

  • For some reason, when there are already embeds in the post, the editor start with an undo level.
  • Ajax responses are cached, but the iframes have to load every time you click undo, which is quite annoying.

@azaozz12 months ago

comment:58 @azaozz12 months ago

In 28195.14.patch: handle embed errors/error message. Thinking this is the most straightforward way to do that (add a filter and call wp_send_json_error() from the callback).

comment:59 follow-up: @iseulde12 months ago

.14 looks nice! I didn't know you could change the response. :)

Btw, I reopened #28088 because the "text" for embedded URLs is <p>URL</p>. Not sure if it should stay like that or not, but you can't copy/paste it in Safari.

@iseulde12 months ago

comment:60 @iseulde12 months ago

setContent keeps setting the toolbar etc. inside the content area instead of overwriting everything. Patch .15 fixes that.

comment:61 @azaozz12 months ago

In [28754]

wpView: improve handling of embed errors/error messages, see #28195

comment:62 in reply to: ↑ 59 @azaozz12 months ago

Replying to avryl:

...the "text" for embedded URLs is <p>URL</p>.

This should be only the URL, no tags?

For some reason, when there are already embeds in the post, the editor start with an undo level.

This seems caused by replacing the views "text" with the wrapper divs. Was playing with resetting the undo levels on initial load. Seems kind of doable but risky when the server is slow and the user starts typing before all views are loaded. We can cancel it on keyup, but then are left with the "weird" undos...

Last edited 12 months ago by azaozz (previous) (diff)

@azaozz11 months ago

comment:63 @azaozz11 months ago

  • Keywords needs-testing added

In 28195.16.patch: when the admin is ssl and the user tries to embed an URL that is http, try using ssl embed if supported by the provider or show a placeholder with error message (Preview not available...).

Last edited 11 months ago by azaozz (previous) (diff)

comment:64 in reply to: ↑ 10 @azaozz11 months ago

Replying to wonderboymusic:

28195.3.diff makes sure the response from ->run_shortcode() is passed to do_shortcode() when it matches get_shortcode_regex(), which is what happens when you pass an audio or video file.

WP_Embed::run_shortcode() already calls do_shortcode(), is there a need to repeat it? Once $parsed = $wp_embed->run_shortcode( $shortcode ); the $parsed = do_shortcode( $parsed ); is trying to find shortcodes in the embed html returned by the oEmbed provider.

comment:65 @azaozz11 months ago

In 28919:

Secure embeds in the editor (first run):

  • When the user pastes an embeddable http URL, try to get the https embed.
  • If an embed provider doesn't support ssl embeds, show a placeholder/error message.
  • Revise the way we return error messages.

See #28195, #28507.

comment:66 @ircbot11 months ago

This ticket was mentioned in IRC in #wordpress-dev by johnbillion. View the logs.

@paulwilde11 months ago

comment:67 @paulwilde11 months ago

My above patch removes the space added below iframe/embeds and removes the border and background colour on hover as they don't really serve much of a purpose. Looks cleaner this way as the border gets cut off at the right edge anyway.

Last edited 11 months ago by paulwilde (previous) (diff)

@iseulde11 months ago

comment:68 @iseulde11 months ago

Replying to paulwilde:

My above patch removes the space added below iframe/embeds and removes the border and background colour on hover as they don't really serve much of a purpose. Looks cleaner this way as the border gets cut off at the right edge anyway.

I'm not sure if we should do that, but if we do, you can just set dispay: block for iframes. I don't think we should remove the border. Instead we should fix the styling for a selected view. See #28533.

Last edited 11 months ago by iseulde (previous) (diff)

comment:69 @helen11 months ago

  • Type changed from enhancement to task (blessed)

comment:70 follow-up: @Ipstenu11 months ago

Possibly related to https://wordpress.org/support/topic/cant-right-clickpaste-urls-in-editor-in-40-beta1?replies=1&view=all

If you try to right-click paste a URL into the visual editor, the URL won't be pasted in. Works fine in HTML/Text view, of course, and Crtl-v works fine, but not the right-click.

comment:71 in reply to: ↑ 70 @mrwweb11 months ago

Replying to Ipstenu:

Possibly related to https://wordpress.org/support/topic/cant-right-clickpaste-urls-in-editor-in-40-beta1?replies=1&view=all

If you try to right-click paste a URL into the visual editor, the URL won't be pasted in. Works fine in HTML/Text view, of course, and Crtl-v works fine, but not the right-click.

Same problem also affects Edit > Paste functionality, at least in Chrome.

comment:72 @jeremyfelt11 months ago

I'm not seeing a specific mention of this issue yet, so...

If I paste a non-embeddable URL as the first text of a new line, wpview attempts to do something with it. This causes a box to flash and then the cursor to jump to the top of the editor - above the first line of text. Typing starts a new line above the pasted URL.

Screen capture - https://cloudup.com/cWX2Bi5DmfJ

This specific behavior starts in [28994]. Before that, the cursor would jump to the line below the pasted URL and the non-embeddable URL would be displayed as if it was embeddable with a box/delete button when selected.

I think I expect non-embeddable URLs to result in no visual change with the cursor still available at the end of the pasted text. The other auto-embed stuff is looking fantastic. :)

comment:73 @iseulde11 months ago

Thanks, I'll try to fix it asap. For non shortcake embeds, I don't think it's a good idea to display the loader... It'd be a much smoother experience if we just paste the url and later replace it with an embed if there is one.

comment:74 @afercia11 months ago

About the loader, please also consider the admin-media dashicon, representing a camera and two eighth notes, may be not so appropriate for non audio/video embeds like, for example, Twitter embeds.

comment:75 @M-BP11 months ago

Hi

I just wanted to say that while this looks really cool and already works nicely in the backend, it doesn't seem to be working at all (tested with IE and FF) when used in an instance of wp_editor() in the frontend - is this intended behaviour?

Since wp_editor() basically uses those tinyMCE plugins, it would be nice to have those previews working regardless of where someone adds content (or maybe even for user submitted forms?).

(related: wpview is also rather buggy when used with wp_editor() in the frontend, cf. ticket #28756)

comment:76 follow-up: @Looimaster11 months ago

I'm using 4.0-beta1-20140716. Below are a couple of bug reports.

  1. After updating from 4.0-beta1-20140714 to 4.0-beta1-20140716 I can no longer paste YouTube links to the back-end editor. An error I get since this update:
Uncaught SecurityError: Blocked a frame with origin "http://localhost" from accessing
a frame with origin "https://www.youtube.com".  The frame requesting access has a
protocol of "http", the frame being accessed has a protocol of "https". Protocols must match.
  1. Like M-BP noticed "Insert from URL" doesn't show previews when Media Library window is enabled somewhere else (like front-end). I enable it using wp_enqueue_media().
  1. This is probably related to the above issue. Currently, "Insert from URL" will output: <a href="https://www.youtube.com/watch?v=xxxxXXXXxxxx">https://www.youtube.com/watch?v=xxxxXXXXxxxx</a> and not [embed]https://www.youtube.com/watch?v=xxxxXXXXxxxx[/embed] when I use:
wp.media.editor.open();
var send_to_editor_backup = window.send_to_editor;
window.send_to_editor = function(output){
	console.log(output);
	window.send_to_editor = send_to_editor_backup;
};

Is this intended output? I think no because for images it outputs <img src="http://example.com/image.jpg">.

  1. Minor: Preview of YouTube player stays when I close the Media Library window and reopen it while the link is reset to http://.
  1. Minor: Shouldn't the title field appear only after there is some link specified and after it's been recognized as image or hasn't been recognized as oEmbed? Currently it is present there since the beginning even if only http:// is present in the field above it. Someone who may want to paste a YouTube link may start by filling the "title" field and they'll be surprised that after pasting the YouTube link this field disappeared.
  1. Confusing:
    • Paste YouTube URL, the loading indicator appears.
    • The loading indicator disappears, the title field is still there but the YouTube preview doesn't appear.
    • After 3 seconds the title field disappears and the YouTube preview appears. There is some lag that lasts 3 seconds for me. The loading indicator should disappear at the same moment the preview appears.
  1. It's not possible to paste YouTube links in empty paragraphs anymore in TinyMCE in 4.0-beta1-20140714 and since 4.0-beta1-20140716 it's not possible to paste YouTube links in both empty paragraphs and existing paragraphs (because of this https:// issue).
  1. Clicking on the "edit" icon while YouTube player is in TinyMCE brings up the Media Library window but the preview is not there and it never loads (4.0-beta1-20140716).

comment:77 in reply to: ↑ 76 @azaozz11 months ago

Replying to Looimaster:

Thanks for the bug reports.

  1. Was fixed yesterday, [29202].
  2. Yeah, wp_enqueue_media() would need some changes/fixes, patch welcome :)
  3. Yes, seems related to the above. Works properly on the Edit Post screen.
  4. Don't see this in Chrome and FF. Could you list all steps to reproduce.
  5. Think before it used to appear after typing/pasting in the URL field. Not sure why it's changed.
  6. Yes, we may need to try to keep the "placeholder" shown for longer. Currently we hide it as soon as the external iframe html is returned. However it takes couple more seconds for the embedded content to load in that iframe.
  7. Seems to work properly here, with or without ssl admin. Could you clear cache and try again.
  8. Same as above: seems to work here in both ssl and non-ssl.

(7 and 8 may be related to 1, so they work now).

Last edited 11 months ago by azaozz (previous) (diff)

@ocean9010 months ago

comment:78 @ocean9010 months ago

  • Keywords has-patch added
  • Version set to trunk

28195.18.patch: The URL shouldn't be escaped via esc_url() in the error message, makes debugging harder.

comment:79 @ocean9010 months ago

In 29354:

To improve troubleshooting use esc_html() for a failed embed.

see #28195.

comment:80 @azaozz9 months ago

  • Resolution set to fixed
  • Status changed from new to closed

This works well. New tickets for any bugs please.

Note: See TracTickets for help on using tickets.