BuddyPress Embeds for activity, user profiles, groups

[imath]

I've been working on embeddable user profiles for my plugin ​WP Idea Stream, here's an example of a user's profile embed :

1/ I think it could be a nice feature to add to BuddyPress
2/ Embedding a post into an activity doesn't work, so maybe we should take a new look on BP_Embeds

What do you think ?

[netweb]

I saw this ​meta changeset this morning and started thinking more about embeds for the bb's, heres some more related references:
​#WP34278 Add support for embeds in the theme template hierarchy
​#WP34561 Abstract some embed template code into functions
​#meta1464 Enable oEmbed for Themes

[DJPaul]

I started to think about oEmbed for BuddyPress about 2 years ago. In one of my many notepads, I've got a bunch of ideas about it that I need to write up (I can't find it real quick, but I'll keep an eye out for it next weekend). For example, the media extracter classes I wrote some releases ago? It was built with embeds in mind so that we could always provide embeds with media.

I suspect this will turn into a big discussion, so here's my current thoughts:

1. Any JS AJAX must use the REST API.
2. Embeds will be the second BuddyPress template that is explicitly designed (emails will be the first). The template in WP core is too immature to consider using as a base template (etc) for ours. We must make the new template look and work great.
3. This opens up new areas of contribution for designers, so we must use this to try to get new contributors involved.
4. I think Activity would be a more usable thing to have embeddable than Profiles, so I'd suggest we start there. As we know, Activity isn't dissimilar in concept from Comments, so we can learn a lot by seeing what WordPress(.com and o.rg) does/might do here.
5. Read versus write. A first pass at embeds could (should?) be read-only, but we need plans to have them be interactable. For example, likes and new comments. I think this is very important for a social network.
6. I'm not sure if WP 4.4 does this, but we should register the current site as an oEmbed provider so people could highlight an Activity item (again, for example) in a blog post and have it "just work".

[r-a-y]

Attached is a first pass at activity oEmbeds.

Some dev notes:

1. Activity oEmbed links

• The activity oEmbed link is the members activity permalink - example.com/members/USER/activity/XXX/ not example.com/activity/p/XXX
• Pasting this link in the WP TinyMCE editor works (see screenshot)
• Pasting in the BP activity post form works, but after AJAX, the javascript required to hide the fallback <blockquote> does not fire. I also had to register the activity link as an oEmbed provider for this to work in BuddyPress. See bp_activity_embed_add_oembed_provider(). WP's TinyMCE appears to work without registering an oEmbed provider. Not sure how this works at the moment, but that is probably the reason why WP posts do not show up as oEmbed items in BuddyPress.
• You can view what the oEmbed looks like by appending ?embed=true to it - example.com/members/USER/activity/XXX/?embed=true

2. Embed templates

• The default WP embed template - /wp-includes/embed-template.php - is a template that contains all the markup for embedding.
• For BP, I've customized this default template to /embeds/template.php.
• I've also split up this template into multiple template parts - /embeds/header.php, /embeds/footer.php, etc. - for better customization.
• The BP activity component injects content into this template via the existing 'embed_content' hook and a custom template part - '/embeds/activity.php. This works similar to how BP plugins use the 'bp_template_content' hook.
• I've also added a simplistic template hierarchy - /embeds/template-single-activity.php, /embeds/template.php. Theme devs can override these by copying and pasting into their theme directory. (eg. /buddypress/embeds/template.php.)
• I'm using the /embeds/css.php file to add inline CSS temporarily. This should probably be moved to the 'embed_head' hook, so disregard this for the time being.

3. Hooking into oEmbed for BuddyPress purposes

• Requires some hackery. WordPress expects a WordPress post ID when formulating the oEmbed response. Since we haven't added any integration into WP's REST API, to overcome this at the moment, I'm passing our Members Directory page ID and adding some markers so BP can render the oEmbed response. See bp_activity_embed_filter_oembed_request_post_id() for what I'm doing.
• The bp_activity_embed_filter_html() function alters the fallback <blockquote> HTML for BuddyPress purposes.
• The bp_activity_embed_filter_oembed_response_data() function filters the oEmbed JSON response.

Todo:

• Linking to activity comments does not show any comments, only the parent activity update at the moment. Would have to think of something similar to how Twitter displays conversations in embedded tweets. See https://dev.twitter.com/web/embedded-tweets#hide-conversation for an example.
• Adding attached media to the template. Basically what DJPaul mentioned in the opening paragraph of comment:3.

[imath]

Nice patch @r-a-y thanks a lot, i will try to test it asap.

[r-a-y]

02.patch removes most of the hackery I noted in comment:5.

In particular:

• This patch properly registers our own oEmbed endpoint instead of piggybacking off of WP's default one. See bp_activity_embed_register_rest_route().
• I didn't want to duplicate many of WP's embed functions just to change a line or two. For example, check out how I'm using get_oembed_response_data() in bp_activity_embed_rest_route_callback(). get_oembed_response_data() requires a WP post, so I'm faking one here. To fake the post, I moved some of the logic from bp_theme_compat_reset_post() into a separate function, bp_theme_compat_create_dummy_post().

Feedback appreciated.

[imath]

@r-a-y i've just tested so here is my first feedbacks :)

About DJPaul's concerns on the maturity of the embed-template.php i think the WP tickets @netweb shared in his first comment are worthing to be favorited and followed.

Then i think it's important we check our BP_Embed class because i think if bp_activity_embed_add_oembed_provider() is needed that's because of this class. WP's class has changed its behavior in 4.4 authorizing by default "discover" and BP_Embed is not. A good way to figure this out is to do this

add_filter( 'embed_oembed_discover', '__return_false' );

and you'll see that WP Editor won't build the Post Embed Box anymore. And another good reason to look at it is Embedding a post into the Activity Stream is failing.
But this might be another ticket's story :)

The improvements you added in 02.patch are really, really interesting: great work. Maybe there's something to improve again, because if i do http://site.url/members/imath/activity/2/?embed=true everything is great, but if i do http://site.url/members/imath/activity/2/embed/ then the content is displayed but true === is_404(). And i wonder if as BuddyPress is not supporting "unpretty" permalinks if the second url is better...

About the new embed directory in bp-template/bp-legacy/buddypress, i think i would have put the new templates into the assets directory, but it's a detail :)

I think maybe we should 'bp_' prefix the embed_content hook inside your template as it's used by WordPress, or maybe use a specific template tag instead.. But maybe you did it on purpose to let any plugin adding content to post embeds to also put this content into the activity embeds ?

I saw css.php was temporary :)

First targetting the Activity object seems logic, but i have a fiew concerns on this/your approach:
1) i don't know if it's possible but maybe we should try to abstract some functions. @r-a-y if i wanted to add embeddable profile i would have to create a bp-members-embeds.php and copy/paste/adapt most functions of bp-activity-embeds.php ? Could it be possible to have a bp-core-embeds.php containing a set of functions components could use to "register" their embed support ?

2) it's also very complex object to start with imho, here are some issues your 2 patches are not dealing with :

• A spammed activity keeps on being displayed into the post it's embed into.
• A deleted activity is displayed like this :

• When an activity contains an embed code like youtube, and is embed into a post, we get a black screen

• to embed an activity, we need to open the single view, copy the Url, and put it in a WordPress site. We don't get the "not powered by WordPress embed code". For my plugin i've added a button to get it, i think it can be interesting to have it too so that it's more user friendly to embed a BuddyPress content :) here's an example of what i'm doing in my plugin: ​https://flic.kr/p/Ca26Zr

[r-a-y]

Hi imath,

Thanks for your detailed analysis!

You are so correct about oEmbed discovery being false in BuddyPress. (Can't believe I missed this!) I've created a new ticket here - #6778.

03.patch addresses most of your points:

• For plugin devs, check out the new BP_oEmbed_Component class. This should make things way easier to add a custom oEmbed endpoint. Any feedback here would be great.
• Fixed bug with spammed / deleted activity items being embeddable.
• Added responsive video JS when an activity item has an embed. I added a new function for this - bp_core_check_content_for_media_type(). This is a small utility function that uses the BP_Media_Extractor class DJPaul built. imath, let me know if this fixes your issue with videos not displaying.

About the share button, I think that would be neat to add with the existing activity loop buttons - "Comment", "Favorite" and "Delete".

[imath]

Hi r-a-y

Just gave the patch a quick look, and it's looking pretty promising!! Thanks a lot for your great work on it.

I promise i'll test it more in detailled asap.

For the 3rd point i'm not sure we should try to display an embed media inside an embed activity. I think we should just use bp_core_check_content_for_media_type() to check if there's an embed content and display a link like 'View Full activity/Embed content'. I haven't tested but i can imagine cases like activity embedding a youtube video, being embed into another activity, being embed into a WordPress post etc... WordPress is using the excerpt of the post and i think it's removing embed content.

About the share button, i agree with you. FYI I've been using the sharing dialog box generated by WordPress for my plugin because a profile is displayed once per parge, but i think in this case we shouldn't load this script for each activity item, but instead try to see if we can load it once, extend it and get the activity link/title within data- attributes of the button.

[imath]

So i've just tested the patch.

1/ there's something wrong with the way the embed link is build inside the sharing dialog box of the embed content :

The part ?embed=true/ shouldn't be in the link.

2/ I really think we should not try to display an embed in embed, because it's failing when we do an embed in embed in embed see:

Because when you delete the first embedded activity its content not interprated (see image) keeps on being displayed into the activity that embed and embed another activity. (?!? becoming crazy with all these embeds!!)
And because i remember a discussion about iframes, and i think one per embed is already the max contributors will be able to "tolerate" :)

So i strongly suggest to take the bp_activity_embed_excerpt() road, displaying a link to the activity single view instead of the embed content: 'View full activity ->' (like i've was saying in my previous comment)

3/ Before it's lost in the great discussion we're having so far, why bp-templates/bp-legacy/buddypress/embeds and not bp-templates/bp-legacy/buddypress/assets/embeds ?

[r-a-y]

Thanks for your feedback again, imath!

04.patch does the following:

• Fixes the URL in the 'WordPress Embed' block
• Replaced BP_oEmbed_Component::$page_conditional_fn property with the abstract BP_oEmbed_Component::is_page() method. If a plugin has complicated page logic, it's easier to add that logic in this method.
• Moves the embed template parts to /assets/. I created a new function called bp_get_asset_template_part() to avoid having to add assets/ to each bp_get_template_part() call.
• Uses a text-based excerpt in the embed template. I've taken some logic out of bp_activity_create_summary() and created a new function, bp_activity_create_excerpt().

As for:

I really think we should not try to display an embed in embed, because it's failing when we do an embed in embed in embed

I think it is useful to display the first embed. 04.patch does this. This only works for registered oEmbed providers, which means that oEmbed-discovered items will not be embedded. I think this is fine and works similar to Twitter.

Let me know what you think.

[imath]

oops @r-a-y since .04 it's no more possible to embed an activity :( I guess there must be a trouble somewhere.

i have 6778.01.patch in. If i apply .03 it's working. Applying .04, it's not.

So I wasn't able to test something about

I think it is useful to display the first embed

What happens if you embed a WordPress post into an activity, could we have possible troubles with the sharing box dialog ?

Anyway i think i wouldn't try to embed an embed. If WordPress is not doing it, there might be a good reason... And not doing embed in embed saves us 1 iframe and an inline javascript :)

Also, i was thinking about the date at the bottom, what about putting it in the embed title like imath posted on December... ?

[r-a-y]

Oops! Sorry about 04.patch, imath!

05.patch does the following:

• Fixes issues with embedding from 04.patch.
• Adds a "Read More" link to the embed excerpt. I made a few tweaks to bp_activity_truncate_entry() and bp_create_excerpt() as I was working on this.
• The BP_oEmbed_Component class now supports registering your own oEmbed endpoint args with the set_route_args() method. See how the BP_Activity_oEmbed_component class registers the 'hide_media' parameter.
• Moved embed media block to a function and hooked to the new 'bp_activity_embed_after_content' action.

Regarding:

Anyway i think i wouldn't try to embed an embed.

Twitter allows an embed within an embed:
https://dev.twitter.com/cards/overview

I also think it should be up to the BP site if they want to disable this or not.

In 05.patch, you can disable displaying embeds with this - remove_filter( 'bp_activity_embed_display_media', '__return_false' );.

I also think it's useful for devs to ping our oEmbed endpoint to see if they want to hide media or not.

05.patch registers 'hide_media' as an oEmbed parameter and allows devs that are pinging our oEmbed activity endpoint to set this parameter.

So if a dev does this request:

// Make sure the activity permalink is valid before doing this!
http://example.com/wp-json/oembed/1.0/embed/activity?hide_media=1&url=http%3A%2F%2Fexample.com%2Fmembers%2FUSER%2Factivity%2F1%2F

The embedded item will not be shown.

This is similar to ​how Twitter's oEmbed works.

Let me know what you think.

[imath]

@r-a-y first, thanks a lot for 05.patch it does fix the issue i was having with 04.patch.
I really like the class you added to abstract BP Embeds, great work!

Thanks a lot for your suggestion about the filter to disable embeds in embeds and the hide_media query args to eventually display them. But i'm sorry, the fluidvids.js workaround is an *absolute no go* for me:

• because the version added seems outdated, it's saying v1.2.0 in the inline script added although it seems stable version is now ​2.4.1
• because, the copyright is outdated and we would need to include a license as it's MIT
• because it only solves the issue for embedded youtube or vimeo videos, see the following cases for Dailymotion or WordPress.tv

Dailymotion

WordPress TV

• because embed content can be videos, but also other kind of content, so even if we were adding players as it's explained ​here, it would only work for videos.

So i'm feeling a lot better with the following kind of output, because i think it would be too complex to make sure embeds in embeds are working great for any kind of embeds.

Read more...

I think we should apply most of the bp_get_activity_content_body filters to the excerpt to be sure it's well displayed (in the first screenshot, you can see that slashes are output without them).

I think we should find something better for the css for embeds (see suggestions in the patch)

In BP_oEmbed_Component->filter_embed_html __( 'Embedded WordPress Post' ) is not passing the commit grunt task because of a missing text domain. Do you think there's another way to replace the title attribute of the iframe ?

Then i'd like to come back on a few concerns 05.patch didn't replied to.

I think maybe we should bp_ prefix the embed_content hook inside your template as it's used by WordPress, or maybe use a specific template tag instead.. But maybe you did it on purpose to let any plugin adding content to post embeds to also put this content into the activity embeds ?

The WP Action embed_content is there to add content to the output *once* the main content has been printed. What we're doing here is printing the main content. I think we should prefix this action in favor of bp_embed_content because i think people filtering embed_content will try to get informations about the post like get_the_ID() etc..

So .05.suggestions.patch is containing some suggestions, you'll need to apply 05.patch and ​6778.02.patch first for it to work. The last point is not included in the patch

Also, i was thinking about the date at the bottom, what about putting it in the embed title like imath posted on December... ?

I know Twitter is doing like you're suggesting so far: put the date/timestamp under the excerpt, but as we're building BuddyPress embeds, i think we should consider moving this part near what normally is the WordPress title & link to the content.

[mrjarbenne]

I think in sites that are more text-based, having an except/teaser is sufficient because invariably the content can't all fit within the embed, and readers will need to click further to consume the entirety of the content; but as BP moves toward becoming more media rich with tools like the attachment API (for example), allowing media-enriched embeds becomes more important. In the example above, in which @imath is showing a [read more] link for a post comprised solely, or at least initially (within the first 250 odd characters that would comprise an embed excerpt) of a video, I think showing that video within the content of the embed becomes expected behavior. If I post a video or picture to my media-rich activity stream (I'm thinking of functionality available not only in Twitter embeds but also Instagram and the like) I would want my audience to be able to see video within the embed.

I think we need to parse out the intended differences between embedding something like a tweet, and embedding a post, and given that BP activity updates aren't character limited, appease both types of content.

• When I embed a tweet, I do so within the context of a blog post, to add context or support an argument. I don't want my audience leaving my blog post to visit Twitter: I want them to get what they need from the embed. This also goes for YouTube embeds, Instagram embeds, Vines, etc.

• When I embed a blog post, I'm expecting that readers will go and explore that post further, leaving my site.

So is an activity update more like a Tweet, or more like a blog post? What expectation will end users have when attempting to embed an update from their niche social network out on their blog?

If that BP site was one in which multimedia sharing takes centre stage, I would want media-rich embeds (a film-maker community in which video from multiple other services is curated together). If it was more text based in nature, particularly long-form updates, then an except becomes more appropriate.

It would be great to have functionality to appease both types of communities, if technically possible.

[r-a-y]

06.patch is refreshed and addresses the following:

• Upgrades fluidvids.js to v2.4.1. This fixes the majority of issues with ​embedding video content from core WP oEmbed providers. (I've listed fluidvids.js on the BP credits page.)
• I tested each WP registered oEmbed provider and there are only some minor issues:
  • For Twitter and Soundcloud, the embeds require that our parent <iframe> has the sandbox="allow-same-origin" attribute. I've added this attribute only when the oEmbed item is from Twitter or Soundcloud.
  • For Tumblr, the issue is their content loads dynamically. WP's embed template tries to calculate the height of the IFRAME and ​dynamically changes the height via JS. I'm filtering WP's JS and making sure their JS fires with a delay of 550ms only for Tumblr embeds, so the iframe height is calculated after Tumblr loads their content.
• Embeds that return images are now responsive (Flickr, Smugmug, etc.).
• Embeds now have a max-width of 550px.
• Ports over imath's suggestions - bp_enqueue_embed_scripts() and 'bp_embed_content'.
• Fixes previous grunt issue from 05.patch with directly referencing __( 'Embedded WordPress Post' ) by using substr_replace() to replace the string.
• Temporarily moves custom embed CSS to inline CSS using wp_add_inline_style(). We shouldn't be using an external stylesheet; WP uses inline CSS for their embed template as well. However, WP's system uses grunt to include the CSS; this is not flexible for theme devs wanting to override the embed CSS. What I would prefer to do is flesh out BP_Legacy::locate_asset_in_stack() and do something like what imath suggested - ​bp_locate_template_asset_uri() - in a future BP version. In the meantime, I think using wp_add_inline_style() is okay for the short-term.
• Fixes some issues with bp_activity_truncate_entry() and bp_create_excerpt() when using 'html=false'.

---

For those that haven't been following along, these are the current issues for discussion:

1) Should we show an embedded item (from YouTube, etc.) in our activity embed template?

The current patch will show the first oEmbed item at the bottom of the template. Only embeds that are ​registered WP oEmbed providers will display. This means that oEmbed-discovered items will not be embedded.

I think this is fine as we are only displaying one embed item and that the oEmbed providers are limited to WP's registered oEmbed providers.

2) Embed template resembles Twitter too much.

I think this is a fair critique as I pretty much copied Twitter's UI as I was building this out.

I understand that BP !== Twitter :) So I'm definitely not stuck with the current design and would welcome contributions for something better here.

3) Should the embed excerpt be plain-text or rich?

I went back and forth about this numerous times in the previous patches. WP uses a plain-text excerpt for their embed template. Current patch is plain-text, but allows links to be rendered.

---

It would be great to have functionality to appease both types of communities, if technically possible.

I've tried to keep this in mind while building this out and I think I've accomplished that.

I think we're going to have to bump this to 2.6. Apologies for not getting this to a shippable version for 2.5.

[imath]

@r-a-y Thanks a lot for your work on this new patch, i promise i'll test it asap. But i'm afraid it won't be soon enough for beta :(
I'm also in favor to bump the ticket to 2.6.

[imath]

FYI > WordPress embed templating has evolved in 4.5 ​https://core.trac.wordpress.org/changeset/36693

[imath]

Before i forgot: we'll probably need to deal with some specific cases :

• the embed activity was deleted
• the embed activity was posted inside a private/hidden group the current user has not access to.

[r-a-y]

07.patch is updated to use the new WP 4.5 embed templates. This bumps the requirement for BP embeds to WP 4.5.

Since most themes will use WP's included wp-includes/theme-compat/embed.php file for the embed template, I'm also making sure we will be using this. However, to override WP's get_template_part( 'embed', 'content' ) call in the embed.php template, I'm doing some object buffering to manually wipe out the markup generated by the embed-content.php template and injecting our BP embed content there. See BP_oEmbed_Component::setup_template_parts() for more info.

Some work still needs to be done to the embed template for activity items with no content, but we can revisit this when my comments in comment:21 are looked at.

---

To answer imath's questions from comment:26:

the embed activity was deleted

In this instance, the iframe content will use WP's embed 404 template.

the embed activity was posted inside a private/hidden group the current user has not access to.

Attempting to embed a private activity will render just the activity permalink.

[mercime]

Hi @r-a-y. Tested the latest patch on WP Trunk and BP Trunk with Twitter, Cloudup, YouTube, and WordPress.tv.

Embeds work fine when posted in Comment/Reply in Activity stream whether or not I add any text before the embed link. However, when I use the Activity Post Form in my Activity > Personal screen and add some text before pasting the link to either cloudup.com, youtube.com, or wordpress.tv, the embed shows up twice on screen - the first one in my item-meta area and the second one is posted in activity stream as expected.
​https://cloudup.com/iVfMLYl4ToH (animated gif)
Refreshing the browser converts the embedded cloudup image or video in the member item meta area into text links.

[imath]

Hi @r-a-y

First i'd like to say you did a really great work on this ticket. I've really tried to convert myself about "embed in embed", to prove it here's the beginning of a suggestion patch i was about to make -> ​gist

But after embedding an activity containing a youtube video or a vimeo video into a post, this is what i got:
​https://cloudup.com/cXrNHE63sjx

The 2 videos refused to display or play because of a cross domain issue. So i definitively think we shouldn't try to iframe an iframe! Because anyway we can't see the content.

Then, attaching the inline embed css to open-sans might not be a good idea, because for some reason, WordPress 4.6 is not including the css anymore, meaning the embed css is not attached :(

Then, i think we should use the Activity action as the title of the embed, as it gives a nice piece of information when for instance an activity is posted within a group.

Finally i understand the will to display some content for the embed so i'm suggesting to fetch the image of it so that's it's better than nothing ;) And image added to activity would also be represented using this suggestion...

So after applying your patch, i've quickly drafted what i have in mind an put it into 6772.07.suggestions.patch (attached to this ticket)
NB: you'll first need to apply 6772.07.patch

And here's some before after:

Youtube

Vimeo

A Group activity containing an image

[r-a-y]

@imath - I was finally able to duplicate your problem with YouTube and Vimeo!

Check out 08.patch.

The problem is WP adds a 'sandbox="allow-scripts" attribute to the <iframe>. In order for YouTube and Vimeo embeds to work (as well as Twitter, Soundcloud, Ted, DailyMotion, SlideShare and Kickstarter), we have to add the allow-same-origin value to the <iframe> sandbox attribute as well. I'm only adding the allow-same-origin value if an activity item has an oEmbed item that matches the sites listed above.

We could also fix this by removing the sandbox attribute altogether but I opted to leave it.

*Mozilla says that having both sandbox="allow-same-origin allow-scripts" is about the same as not adding the sandbox attribute:
​https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-sandbox

Anyway, here's a GIF :)

Please let me know if this works for you, imath.

08.patch also:

• Fixes a fatal error when WP < 4.5 is used. Oops! :)
• Fixes a bug with rendering XML oEmbed responses as JSON. See the new BP_oEmbed_Component::oembed_xml_request() method for the fix.
• Adds support to embed inline MP3 and video links. Copy and paste a URL to a MP3 or video file into an activity update and the BP embed template should show it. The browser's native HTML5 player is used for this.
• Based on imath's feedback, the activity embed template now uses the activity action. My only concern with this is with long action strings and long usernames.
• Inline CSS is moved to the 'embed_head' hook for now.
• fluidvids.js is moved to the 'embed_footer' hook for now.
• Based on imath's 07.suggestions.patch, I decided to add a header-activity.php template part.
• Ensures that we do not enqueue the group widget JS on BP embed pages.

---

@mercime - Thanks for testing. I think your bug is not related to this ticket though; I've opened a new issue for this - #7063.

[imath]

@r-a-y thanks you so much for your continuous great work on this ticket.

About iframe in iframe: i confirm the security issues are fixed. So 🚦=== green light :)

Activity embeds are looking really great, i only have some last suggestions.

1. i think we should try to avoid adding too much inline css or js in php code. I see no reason for the embed stylesheet to be private, it seems too bad and anyway we can always override it with !important, so i think we can use wp_enqueue_style(), enjoy rtl, and include a filter if the themes want to customize it. It will avoid super charging css. I think we can use wp_enqueue_script() for fluidvids too. This way we are actually using the function you created bp_enqueue_embed_scripts() (you were not using it in 08.patch)
2. About fluidvids.js as activity embeds requires 4.5 i think we should enjoy wp_add_inline_script() to set fluidvids options.
3. About the assets/embeds/header.php, now that you created assets/embeds/header-activity.php it's not loaded anymore. But maybe you're using it as a fallback, so i've left it but i removed the mention name because it wasn't looking great.
4. About assets/embeds/header-activity.php i think the timestamp should display even if mentions are disabled. So i've edited the template that way.
5. About the activity loop fired twice (in assets/embeds/header-activity.php & assets/embeds/activity.php ) i suggest to introduce a new wrapper bp_activity_embed_has_activity(). This way if the $activities_template is already set for the embed activity, the assets/embeds/activity.php can directly use it without resetting the loop.

.08.suggestions.patch requires .08.patch and contains the above suggestions. (sorry i haven't include the docblocks but i wasn't sure you'd be ok with theses suggestions, so i've been lazy!)

Finally, i now feel pretty confident about this great feature. Thanks again for your perseverance.

[r-a-y]

@imath - Your suggestions are great!

My only concern is with enqueuing assets.

This is what pento ​mentions in the WP Embeds ticket:

For security, we wouldn't allow JS or CSS from the remote site to be embedded in the page - it can too easily be used as an attack vector.

I think this is a strong reason not to allow enqueuing assets.

[imath]

Well, ok. I understand the security concern, but in this case no hooks should be available into the embed template, else we can always enqueue/print JS or CSS. And we should probably avoid the use of any javascript like fuidvids and avoid risky situations like doing iframe in iframe ;)

[r-a-y]

Well, ok. I understand the security concern, but in this case no hooks should be available into the embed template

True, #7087 is an example of such a problem. WP also enqueues their emoji JS script in the embed template as well.

I don't mind if we enqueue assets, but I just note the concerns of those that implemented WP Embeds.

Maybe it makes sense to use your Press This approach to grab the external image and just link to the embed content.

[r-a-y]

So in this week's dev chat, imath and I decided to scrap iframes-within-iframes due to potential security issues.

Instead we're going to display an image and link to the embedded item. This is similar to how Facebook operates.

09.patch does the following:

• Caches the full oEmbed response for activity items so we can show the oEmbed thumbnail in the embed template.
• Shows a thumbnail and caption from a WP oEmbed provider if available in the embed template.
• Uses the SVG play icon from ​Ionicons (licensed under the MIT). If we'd rather stick with WP's dashicons, we can do that, but the dashicons play icon is a little too pointy. This is pretty much a cosmetic thing and isn't really pertinent.
• In order for links to work while using <iframe sandbox>, I had to add the sandbox="allow-top-navigation" attribute. The other gotcha here is in order for links to be clickable, links must use top.location.href. (See bp_activity_embed_excerpt_onclick_location_filter().) If we'd rather links open in a new window, let me know.
• Uses a template part to output the inline CSS (see /embeds/activity/css-activity.php). In order to auto-generate the RTL CSS, I'm piggybacking off of grunt rtlcss to generate the related RTL CSS template part. As to why I'm opting for inline CSS, see comment:36.
• Removes inline images from being displayed if no oEmbed item is found. Reason why I removed it is <img src> could potentially be vulnerable. Maybe I'm being too cautious here. Let me know if we should reinstate this.

Here's a screenshot of a TED video in action:

[imath]

@r-a-y fantastic work. I think we're about to touchdown!

I've found an issue though:

In order for links to work while using <iframe sandbox>, I had to add the sandbox="allow-top-navigation" attribute. The other gotcha here is in order for links to be clickable, links must use top.location.href. (See bp_activity_embed_excerpt_onclick_location_filter().) If we'd rather links open in a new window, let me know

When i click on the play svg button, this is what i get:

The link won't open :( I was thinking what about using the activity permalink instead ?

Then i have a question, the width for the embed is depending on the image width ? Because i find it weird to have this kind of layout

and this kind of layout for youtube:

What about centering it ?

[r-a-y]

Thanks for the feedback, imath.

About the play icon, forgot to add top.location.href to the link. See 09b.play-icon-fix.patch.

About the thumb width, originally I had it set to always maximize to 550px. But, there are some oEmbed providers that have smaller thumbs such as Spotify that looked pixelated if stretched.

Maybe we can drop the max width to something a little smaller like 450px and then stretch? Or just maximize to 550px and do not care about pixelation. Could use some other feedback here.

[imath]

@r-a-y i confirm 09b.play-icon-fix.patch fixed the issue

I think we can simply center the div, something like :

.bp-activity-embed-display-media {
   margin: 0 auto;
}

is doing the centering as you specified a max-width.

The other option is to display the div horizontally having the image on the left and the text on the right if the width is under a specific amount.

But i think we can have this in with the centered div ;) Great work!

[r-a-y]

The other option is to display the div horizontally having the image on the left and the text on the right if the width is under a specific amount.

I like this suggestion better :)

In 09c.two-col.patch (requires 09.patch), two columns are shown if the thumbnail width is smaller than 350px.

If the width is higher than 350px, we revert to the one column layout as before.

Here's a screenshot of a DailyMotion vid:

[imath]

@r-a-y two-col is great. I was so excited about it, that I've just upgraded the test drive to trunk + 6772.09 + 6772.09c
See: ​http://imath-buddypress.wpserveur.net/testing-activity-embeds/

And we have a situation!

When i embed one of the activity of the test drive on my local dev, here's what i got:

I wasn't able to open the vimeo url and security issues were logged. So as long as you embed an activity in your own site it's working great, but when you embed an activity from site A to B, the embed video refuses to open.

[imath]

Actually, any links that has a different domain than imath-buddypress.wpserveur.net is blocked. But when i click on the date which links to the activity permalink on imath-buddypress.wpserveur.net i can access to it.

[r-a-y]

Thanks for finding this important bug, imath!

10th time's a charm :)

Can you try 10.patch and purge your post oEmbed caches and see if this fixes your problems?

[imath]

@r-a-y I confirm it's fixing the issue. I have a last minor suggestion as i noticed if some links were added inside the embed description it wasn't possible to click on it.

See the first embedded vimeo here:
​http://imath-buddypress.wpserveur.net/testing-activity-embeds/

I'm talking about the link i framed in orange

Other than that, i really think we should now have this in 2.6 :)

[r-a-y]

Your suggestion is great, imath!

I've included it in 10b.tweaks.patch (requires 10.patch).

This patch also:

• Tweaks the media layout to be responsive at 480px. At 480px, the two-column layout switches to one-column.
• Displays thumbnails at its original thumb width in two-column mode.
• Makes the play icon slightly smaller in two-column mode.

[imath]

Great idea to bring a responsive layout. I'd like to suggest a last little touch. When the thumb width is bigger than the float width, the caption width should be 100% else the caption is not taking all the available space.

Here's an example before 6772.10c-suggestion.patch

---

And here's an example after 6772.10c-suggestion.patch

NB on ​http://imath-buddypress.wpserveur.net/testing-activity-embeds/ i've applied 6772.10c-suggestion.patch

[r-a-y]

Good catch, imath. Thanks for all your testing and patches!

11.patch includes all the improvements from 10.patch. Pending any other feedback, I will start breaking this up and committing this in separate commits to trunk either tonight or tomorrow.

[imath]

committing this in separate commits to trunk either tonight or tomorrow

👍 🍕👏

[r-a-y]

Refreshed after r10825

[r-a-y]

In 10829:

Embeds: Introduce 'bp_enqueue_embed_scripts' hook.

This will be used to enqueue scripts on embed pages.

See #6772.

[r-a-y]

In 10830:

In bp_create_excerpt(), introduce 'strip_tags' and 'remove_links' as arguments.

This will help to craft better plain-text excerpts.

To be used in activity embed excerpts. See #6772.

[r-a-y]

In 10833:

Activity: Cache full oEmbed response for activity entries.

Currently, we cache the oEmbed HTML response markup. However, oEmbed also
returns other useful information like thumbnail, title, author and provider
name.

This commit caches the full oEmbed response so we can use this data for
other things such as rich activity embeds.

See #6772.

[r-a-y]

In 10836:

Embeds: Bail out of theme compatibility if on an embed page.

Embeds will use a special template. However, theme compatibility takes
precedence, so we must bail from theme compatibility if on an embed page.

See #6772.

[r-a-y]

In 10837:

Core: Introduce BP_oEmbed_Component class.

This class interacts with various aspects of the Embeds functionality
introduced in WordPress 4.4 and is meant to be extended by BuddyPress
components to easily add oEmbed support.

The Activity component will be the first one to utilize this class.

See #6772.

[r-a-y]

In 10838:

Activity: Add Embeds support to single activity items.

Much like the Post Embeds feature introduced in WordPress 4.4, this commit
adds support for single activity URLs to be auto-discovered by oEmbed
consumers.

This allows activity URLs to be rendered into an embeddable format on
third-party websites.

Specifically, this commit:

• Introduces the BP_Activity_oEmbed_Component class, which hooks into WordPress' oEmbed provider API via the BP_oEmbed_Component class.
• Renders oEmbed requests for single activity items into a template suitable for embedding.
• Introduces embed template parts into bp-legacy - /bp-templates/bp-legacy/buddypress/assets/embeds/

See #6772.

[r-a-y]

In 10839:

Activity: Introduce media into embed template.

This commit displays media from an activity item into the embed template.

Specifically, if an activity item contains an oEmbed item from a
WordPress whitelisted oEmbed provider and that oEmbed item contains a
thumbnail, we will display that thumbnail along with a caption. One thing
to note is we only display the first oEmbed item to prevent cluttering up
the embed template.

If an oEmbed item isn't found, we will attempt to find the first inline
video or audio item. If such an item is found, we will embed that item
using the browser's native HTML5 player.

Props r-a-y, imath.

See #6772.

[r-a-y]

In 10840:

Add Ionicons to BuddyPress credits list.

Ionicons is licensed under the MIT:
​https://github.com/driftyco/ionicons

We use the play icon from Ionicons in the activity embed template when a
thumbnail is shown from an oEmbed provider.

See #6772.

[r-a-y]

In 10842:

Activity: Add RTL inline CSS template part for embeds.

See #6772.

[r-a-y]

Okay, I think we're good here!

I've opened some enhancement tickets, #7101 and #7102, for some possible, future improvements.

If you find any bugs or issues, please open new tickets. Thanks! Also, documentation will be coming in the next week or so.

[r-a-y]

In 10863:

Revert "Grunt: Modify RTLCSS to generate RTL files for inline CSS template parts."

Using inline CSS as a template part is a no-no. This reverts r10841.

See #7104, #6772.

[r-a-y]

In 10864:

Activity Embeds: Use CSS asset file for inline CSS.

Previously, we were using a template part to grab the inline CSS. This
is not a great idea.

Instead, we are using the actual CSS file and including it inline.

See #7104, #6772.

[tw2113]

@r-a-y I'm looking at r10838 and ​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-activity/bp-activity-embeds.php?annotate=blame#L120

Isn't this overwriting the passed in content right away for the function?

function bp_activity_embed_excerpt( $content = '' ) {
	echo bp_activity_get_embed_excerpt( $content = '' );
}

[r-a-y]

@tw2113 - Nice catch. Commit whenever you are ready!

[tw2113]

In 11448:

Remove variable overwriting inside bp_activity_embed_excerpt() before passing into bp_activity_get_embed_excerpt().

See #6772.