Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#34429 closed defect (bug) (fixed)

Slack embeds look super ugly

Reported by: pento's profile pento Owned by: pento's profile pento
Milestone: 4.4 Priority: normal
Severity: blocker Version: 4.4
Component: Embeds Keywords: has-patch commit
Focuses: Cc:

Description

With oEmbed enabled, Slack's built-in link previews are filled with uglified JS, instead of the actual content.

Slack are currently looking into it, but have been quite slow to respond, and we certainly can't ship 4.4 with this behaviour still occurring. The fallback is to disable the oEmbed response when the Slackbot-LinkExpanding UA is detected, so that Slack will scrape the content from the page, instead.

(If anyone has better ideas, this would be the ideal place to share them.)

Attachments (1)

34429.diff (735 bytes) - added by peterwilsoncc 9 years ago.

Download all attachments as: .zip

Change History (17)

#1 @swissspidy
9 years ago

This is the fix that has been added to WordPress.org:

<?php
add_action( 'plugins_loaded', function() {
    if ( false !== strpos( $_SERVER['HTTP_USER_AGENT'], 'Slackbot-LinkExpanding' ) ) {
        remove_filter( 'oembed_response_data', 'get_oembed_response_data_rich' );
    }
}, 20 );

This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.


9 years ago

#3 follow-up: @dd32
9 years ago

and we certainly can't ship 4.4 with this behaviour still occurring

I'd disagree and say that we certainly can. Slack is an oEmbed client, we shouldn't put in workarounds to fix a specific oEmbed clients issues. Slack can choose to ignore the WordPress Embedding if their preview system can't support it, but we shouldn't do it the other way around.

#4 @dd32
9 years ago

Also, as to why it has Javascript on display, it's probably because they're using something like strip_tags() which leaves JS/CSS in the result.. which is why WordPress has a wp_strip_all_tags() method instead.

#5 in reply to: ↑ 3 @pento
9 years ago

Replying to dd32:

we shouldn't put in workarounds to fix a specific oEmbed clients issues.

We have plenty of user agent specific workarounds in core, Slack is just a different kind of UA. :-)

Ideally, Slack will get it fixed in time. But if they don't, I don't think it's the right option for us to force a bad UX onto users that we have the power to fix.

#6 @dd32
9 years ago

Although it's not a nice experience, it's far from an issue that affects a large percentage of users and one where end-users will be stuck with it.

Unlike browsers where all users need to update, Slack can make a single small central code change and fix the experience for all users for WordPress & other platform embeds.

#7 follow-up: @danielbachhuber
9 years ago

Can we serve a no-script fallback in the beginning of the response as Twitter does? Then, the JavaScript would remove the fallback

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


9 years ago

#9 in reply to: ↑ 7 ; follow-up: @pento
9 years ago

Replying to danielbachhuber:

Can we serve a no-script fallback in the beginning of the response as Twitter does? Then, the JavaScript would remove the fallback

Yep, that was on the todo list somewhere - @helen suggested it a while ago. I'm not sure why it wasn't done before merge - @swissspidy, what happened to this?

#10 in reply to: ↑ 9 ; follow-up: @swissspidy
9 years ago

Replying to pento:

Replying to danielbachhuber:

Can we serve a no-script fallback in the beginning of the response as Twitter does? Then, the JavaScript would remove the fallback

Yep, that was on the todo list somewhere - @helen suggested it a while ago. I'm not sure why it wasn't done before merge - @swissspidy, what happened to this?

Can't find a discussion on Slack right now, but there were two GitHub issues about this:

https://github.com/swissspidy/oEmbed-API/issues/138
https://github.com/swissspidy/oEmbed-API/pull/158

The problem was that Slack uses the content from every element, so if you have some JS to remove the <noscript> fallback, the content of both elements would show. And if you use external JS to remove the <noscript>, how do you ensure that it only creates a sandboxed <iframe>?

Note: Using CDATA might be a solution, see https://wordpress.slack.com/archives/core/p1445828420002520

Still waiting for an answer from Slack.

@peterwilsoncc
9 years ago

#11 @peterwilsoncc
9 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

In 34429.diff, I've wrapped the embed JavaScript in the CDATA/HTML comment combination as detailed on MDN's Writing JavaScript for XHTML page:

<!--//--><![CDATA[//><!--
Embed JavaScript
//--><!]]>

Slack does not attempt to display the embed, instead it falls back to the opengraph data. This makes sense if they're running the page through the same regex as strip_tags.

Reduced case tests:
http://gh.peterwilson.cc/testing/html-l.html (without data-secret)
http://gh.peterwilson.cc/testing/html-s.html (with data-secret)

Tested in Browser stack usign the reduced cases for:
IE7+ on WinXP+
Latest Firefoxes on XP+ and last three OSXs

#12 in reply to: ↑ 10 ; follow-up: @pento
9 years ago

  • Keywords commit added; 2nd-opinion removed

Replying to swissspidy:

The problem was that Slack uses the content from every element, so if you have some JS to remove the <noscript> fallback, the content of both elements would show. And if you use external JS to remove the <noscript>, how do you ensure that it only creates a sandboxed <iframe>?

This isn't just to fix Slack. This is also for if the embedded site goes down, then the embedding site has a local copy of the data that they can display. See: https://github.com/swissspidy/oEmbed-API/issues/161

34429.diff fixes the Slack issue, I'll add a new ticket shortly for the text fallback.

#13 @pento
9 years ago

  • Owner set to pento
  • Status changed from new to assigned

#14 @pento
9 years ago

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

In 35406:

Embeds: Add a CDATA and HTML comment wrapper around the JavaScript included in the embed HTML. This ensures that user agents which naively strip unwanted tags, without also stripping the content within those tages (for example, Slack), don't show the embed as a mess of JavaScript.

Props peterwilsoncc.

Fixes #34429.

#15 in reply to: ↑ 12 @pento
9 years ago

Replying to pento:

I'll add a new ticket shortly for the text fallback.

Opened #34462.

#16 @swissspidy
9 years ago

Just got a reply from Slack support:

Thanks for your patience with all of this. We have deployed a change that should address the issue with oEmbeds returning the script tags. Please let us know if that is fixed on your end.

I just tested this with a site of mine in the #test Slack channel and it appears to be working fine. Slackbot falls back to the Open Graph data so I guess CDATA is not needed anymore, at least not for Slack.

Note: See TracTickets for help on using tickets.