Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Twitter raw embeds are not all converted #4653

Closed
westonruter opened this issue May 4, 2020 · 5 comments
Closed

Twitter raw embeds are not all converted #4653

westonruter opened this issue May 4, 2020 · 5 comments
Labels
Bug Something isn't working Embeds P2 Low priority Punted WS:Core Work stream for Plugin core

Comments

@westonruter
Copy link
Member

Bug Description

There are various options for embedding content from Twitter, as can be seen at https://publish.twitter.com/:

image

Of all these options, only the Tweet currently is properly converted.

This necessitated Jetpack adding special support for embedding Twitter timelines: Automattic/jetpack#15328

This should not have been the case, however: https://github.com/Automattic/jetpack/pull/15328/files#r419632716

Expected Behaviour

Raw embeds code coming from Twitter should be converted into AMP components as required.

Steps to reproduce

Paste this markup while editing code in the block editor:

<!-- wp:heading -->
<h2>Collection</h2>
<!-- /wp:heading -->

<!-- wp:html -->
<a class="twitter-timeline" href="https://twitter.com/TwitterDev/timelines/539487832448843776?ref_src=twsrc%5Etfw">National Park Tweets - Curated tweets by TwitterDev</a> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<!-- /wp:html -->

<!-- wp:heading -->
<h2>Tweet</h2>
<!-- /wp:heading -->

<!-- wp:html -->
<blockquote class="twitter-tweet"><p lang="en" dir="ltr">Sunsets don&#39;t get much better than this one over <a href="https://twitter.com/GrandTetonNPS?ref_src=twsrc%5Etfw">@GrandTetonNPS</a>. <a href="https://twitter.com/hashtag/nature?src=hash&amp;ref_src=twsrc%5Etfw">#nature</a> <a href="https://twitter.com/hashtag/sunset?src=hash&amp;ref_src=twsrc%5Etfw">#sunset</a> <a href="http://t.co/YuKy2rcjyU">pic.twitter.com/YuKy2rcjyU</a></p>&mdash; US Department of the Interior (@Interior) <a href="https://twitter.com/Interior/status/463440424141459456?ref_src=twsrc%5Etfw">May 5, 2014</a></blockquote> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<!-- /wp:html -->

<!-- wp:heading -->
<h2>Profile: Embedded Timeline</h2>
<!-- /wp:heading -->

<!-- wp:html -->
<a class="twitter-timeline" href="https://twitter.com/TwitterDev?ref_src=twsrc%5Etfw">Tweets by TwitterDev</a> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<!-- /wp:html -->

<!-- wp:heading -->
<h2>Profile: Follow Button</h2>
<!-- /wp:heading -->

<!-- wp:html -->
<a href="https://twitter.com/TwitterDev?ref_src=twsrc%5Etfw" class="twitter-follow-button" data-show-count="false">Follow @TwitterDev</a><script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<!-- /wp:html -->

<!-- wp:heading -->
<h2>Profile: Mention Button</h2>
<!-- /wp:heading -->

<!-- wp:html -->
<a href="https://twitter.com/intent/tweet?screen_name=TwitterDev&ref_src=twsrc%5Etfw" class="twitter-mention-button" data-show-count="false">Tweet to @TwitterDev</a><script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<!-- /wp:html -->

<!-- wp:heading -->
<h2>List</h2>
<!-- /wp:heading -->

<!-- wp:html -->
<a class="twitter-timeline" href="https://twitter.com/TwitterDev/lists/national-parks?ref_src=twsrc%5Etfw">A Twitter List by TwitterDev</a> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<!-- /wp:html -->

<!-- wp:heading -->
<h2>Moment</h2>
<!-- /wp:heading -->

<!-- wp:html -->
<a class="twitter-moment" href="https://twitter.com/i/moments/625792726546558977?ref_src=twsrc%5Etfw">Michelle Obama Opens Special Olympics</a> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<!-- /wp:html -->

<!-- wp:heading -->
<h2>Likes Timeline</h2>
<!-- /wp:heading -->

<!-- wp:html -->
<a class="twitter-timeline" href="https://twitter.com/TwitterDev/likes?ref_src=twsrc%5Etfw">Tweets Liked by @TwitterDev</a> <script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<!-- /wp:html -->

<!-- wp:heading -->
<h2>Hashtag Button</h2>
<!-- /wp:heading -->

<!-- wp:html -->
<a href="https://twitter.com/intent/tweet?button_hashtag=LoveTwitter&ref_src=twsrc%5Etfw" class="twitter-hashtag-button" data-show-count="false">Tweet #LoveTwitter</a><script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>
<!-- /wp:html -->

Screenshots

Validation errors are raised for all embeds other than Tweets:

image


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

The logic in AMP_Twitter_Embed_Handler::is_tweet_raw_embed() is lacking:

/**
* Checks whether it's a twitter blockquote or not.
*
* @param DOMElement $node The DOMNode to adjust and replace.
* @return bool Whether node is for raw embed.
*/
private function is_tweet_raw_embed( $node ) {
// Skip processing blockquotes that have already been passed through while being wrapped with <amp-twitter>.
if ( $node->parentNode && 'amp-twitter' === $node->parentNode->nodeName ) {
return false;
}
$class_attr = $node->getAttribute( 'class' );
return null !== $class_attr && false !== strpos( $class_attr, 'twitter-tweet' );
}

Namely, in addition to checking for twitter-tweet it also needs to account for all the other class names that the embeds utilize, including twitter-timeline, twitter-hashtag-button, twitter-moment and so on.

QA testing instructions

Demo

Changelog entry

@pierlon
Copy link
Contributor

pierlon commented May 5, 2020

This may be unintentionally fixed by #4650 as I noticed some of the tests in AMP_Twitter_Embed_Handler_Test began to fail due to them not being sanitized properly by AMP_Twitter_Embed_Handler::sanitize_raw_embeds. I'll ensure this issue is accounted for in the aforementioned PR.

@pierlon
Copy link
Contributor

pierlon commented May 8, 2020

The "follow", "hashtag", and "mention" buttons do not seem to be supported by the amp-twitter component.

When the embed is rendered by the accompanied script it produces an iframe:

<iframe id="twitter-widget-0" scrolling="no" frameborder="0" allowtransparency="true" allowfullscreen="true" class="twitter-follow-button twitter-follow-button-rendered" style="position: static; visibility: visible; width: 129px; height: 20px;" title="Twitter Follow Button" src="https://platform.twitter.com/widgets/follow_button.c63890edc4243ee77048d507b181eeec.en.html#dnt=true&amp;id=twitter-widget-0&amp;lang=en&amp;screen_name=TwitterDev&amp;show_count=false&amp;show_screen_name=true&amp;size=m&amp;time=1588909307584" data-screen-name="TwitterDev"></iframe>

We could attempt to create an amp-iframe and imitate the iframe src URL above, but that seems like a brittle solution to the problem. Any suggestions?

@westonruter
Copy link
Member Author

It looks like there is a feature request to support this: ampproject/amphtml#24678

In the meantime, we'll have to just skip converting such elements. At least the link is a suitable fallback.

@kmyram kmyram added the WS:Core Work stream for Plugin core label May 15, 2020
@westonruter westonruter added this to the v1.6 milestone Jun 15, 2020
@westonruter westonruter modified the milestones: v1.6, v1.7 Jun 28, 2020
@westonruter westonruter modified the milestones: v2.1, v2.2 Feb 11, 2021
@rickalee
Copy link

rickalee commented Feb 15, 2021

@westonruter We are seeing Twitter not being modified to amp-twitter when the user pastes Tweet URL into the block editor or uses 'Twitter' embed block. My assumption is this is because of lack of a tag or blockquote or potentially a change in WP 5.6.

Here is what is stored in post_content

<!-- wp:embed {"url":"https://twitter.com/Twitter/status/1360273390070865928","type":"rich","providerNameSlug":"twitter","responsive":true,"className":""} -->
<figure class="wp-block-embed alignwide is-type-rich is-provider-twitter wp-block-embed-twitter"><div class="wp-block-embed__wrapper">
https://twitter.com/Twitter/status/1360273390070865928
</div></figure>
<!-- /wp:embed -->

@westonruter
Copy link
Member Author

@rickalee I'm not seeing that. When I view the AMP version, I get amp-twitter rendered on the frontend:

<figure class="wp-block-embed is-type-rich is-provider-twitter wp-block-embed-twitter alignwide">
	<div class="wp-block-embed__wrapper">
		<amp-twitter width="600" height="480" layout="responsive" data-tweetid="1360273390070865928" class="twitter-tweet i-amphtml-layout-responsive i-amphtml-layout-size-defined" data-width="550" data-dnt="true" i-amphtml-layout="responsive">
			<i-amphtml-sizer style="display:block;padding-top:80%"></i-amphtml-sizer>
			<blockquote class="twitter-tweet" data-width="550" data-dnt="true" placeholder=""><p lang="en" dir="ltr">
				roses are red <br>our avi is blue <br>if you're single<br>we'll reply to you</p>— Twitter (@Twitter)
				<a href="https://twitter.com/Twitter/status/1360273390070865928?ref_src=twsrc%5Etfw">February 12,
					2021</a></blockquote>
		</amp-twitter>
	</div>
</figure>

image

@westonruter westonruter added the P2 Low priority label Oct 25, 2021
@westonruter westonruter modified the milestones: v2.2, v2.3 Nov 30, 2021
@westonruter westonruter modified the milestones: v2.3, v2.4 Dec 23, 2021
@westonruter westonruter removed this from the v2.4 milestone Apr 14, 2022
@westonruter westonruter closed this as not planned Won't fix, can't repro, duplicate, stale Jul 26, 2022
@github-project-automation github-project-automation bot moved this to In Progress in Ongoing Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Embeds P2 Low priority Punted WS:Core Work stream for Plugin core
Projects
Archived in project
Development

No branches or pull requests

4 participants