#6772 closed idea (fixed)
BuddyPress Embeds for activity, user profiles, groups
Reported by: | imath | Owned by: | |
---|---|---|---|
Milestone: | 2.6 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Core | Keywords: | dev-feedback has-patch commit |
Cc: | hnla, stephen@…, brajesh@…, espellcaste@… |
Description
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 ?
Attachments (19)
Change History (89)
#3
@
9 years ago
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:
- Any JS AJAX must use the REST API.
- 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.
- This opens up new areas of contribution for designers, so we must use this to try to get new contributors involved.
- 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.
- 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.
- 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".
#5
@
9 years ago
- Keywords has-patch added
Attached is a first pass at activity oEmbeds.
Some dev notes:
- Activity oEmbed links
- The activity oEmbed link is the members activity permalink -
example.com/members/USER/activity/XXX/
notexample.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. Seebp_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
- 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.
- 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.
#8
@
9 years ago
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()
inbp_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 frombp_theme_compat_reset_post()
into a separate function,bp_theme_compat_create_dummy_post()
.
Feedback appreciated.
#9
@
9 years ago
@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
This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.
9 years ago
#11
@
9 years ago
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 theBP_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".
#12
@
9 years ago
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.
#13
@
9 years ago
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
?
#14
@
9 years ago
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 abstractBP_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 calledbp_get_asset_template_part()
to avoid having to addassets/
to eachbp_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.
#15
@
9 years ago
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...
?
#16
@
9 years ago
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()
andbp_create_excerpt()
as I was working on this. - The
BP_oEmbed_Component
class now supports registering your own oEmbed endpoint args with theset_route_args()
method. See how theBP_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.
#17
@
9 years ago
@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
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.
#18
@
9 years ago
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.
This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.
9 years ago
#21
@
9 years ago
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 thesandbox="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.
- For Twitter and Soundcloud, the embeds require that our parent
- 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 usingsubstr_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 outBP_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 usingwp_add_inline_style()
is okay for the short-term. - Fixes some issues with
bp_activity_truncate_entry()
andbp_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.
This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.
9 years ago
#23
@
9 years ago
@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.
#25
@
9 years ago
FYI > WordPress embed templating has evolved in 4.5 https://core.trac.wordpress.org/changeset/36693
#26
@
9 years ago
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.
#27
@
8 years ago
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.
This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.
8 years ago
#29
@
8 years ago
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.
#30
@
8 years ago
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
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
8 years ago
#32
@
8 years ago
@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 as 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
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 aheader-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.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
8 years ago
#34
@
8 years ago
@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.
- 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 usewp_enqueue_script()
for fluidvids too. This way we are actually using the function you createdbp_enqueue_embed_scripts()
(you were not using it in 08.patch) - About fluidvids.js as activity embeds requires 4.5 i think we should enjoy
wp_add_inline_script()
to set fluidvids options. - About the
assets/embeds/header.php
, now that you createdassets/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. - 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. - About the activity loop fired twice (in
assets/embeds/header-activity.php
&assets/embeds/activity.php
) i suggest to introduce a new wrapperbp_activity_embed_has_activity()
. This way if the$activities_template
is already set for the embed activity, theassets/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.
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
8 years ago
#36
@
8 years ago
@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.
#37
@
8 years ago
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 ;)
#38
@
8 years ago
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.
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
8 years ago
#40
@
8 years ago
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 thesandbox="allow-top-navigation"
attribute. The other gotcha here is in order for links to be clickable, links must usetop.location.href
. (Seebp_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 ofgrunt 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:
#41
@
8 years ago
@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 ?
#42
@
8 years ago
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.
#43
@
8 years ago
@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!
#44
@
8 years ago
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:
#45
@
8 years ago
@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.
#46
@
8 years ago
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.
#47
@
8 years ago
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?
#48
@
8 years ago
- Keywords commit added
@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 :)
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
8 years ago
#50
@
8 years ago
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.
#51
@
8 years ago
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
#52
@
8 years ago
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.
This ticket was mentioned in Slack in #bbpress by netweb. View the logs.
8 years ago
#68
@
8 years ago
@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 = '' ); }
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