#34429 closed defect (bug) (fixed)
Slack embeds look super ugly
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (17)
This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.
9 years ago
#3
follow-up:
↓ 5
@
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
@
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
@
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
@
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:
↓ 9
@
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:
↓ 10
@
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:
↓ 12
@
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.
#11
@
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:
↓ 15
@
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.
#16
@
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.
This is the fix that has been added to WordPress.org: