Add Descript.com embeds support #22234

alshakero · 2022-01-05T22:26:13Z

Similar to #21101. Which was thoroughly reviewed.

Changes proposed in this Pull Request:

This PR adds support for Descript.com embedding.

It adds a Descript.com embedding block, you can test it by adding a "Descript" block and use this URL to test.
Descript normal URLs using the embed block.
Descript shortcodes like [descript id="gXEWtXaGTyK" width="640" height="480"].
Descript embed code, it transforms it into a safe shortcode.

<iframe src="https://share.descript.com/embed/gXEWtXaGTyK" width="640" height="360" frameborder="0" allowfullscreen></iframe>

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Apply D72574-code diff on your sandbox.
Sandbox a site. Create a new post.
Using the Descript block, embed https://share.descript.com/view/gXEWtXaGTyK.
Using the embed block, embed https://share.descript.com/view/gXEWtXaGTyK.
Using the shortcode block, please test [descript id="gXEWtXaGTyK" width="640" height="480"].
Using custom HTML block, use the embed code they provide:

<iframe src="https://share.descript.com/embed/gXEWtXaGTyK" width="640" height="360" frameborder="0" allowfullscreen></iframe>

matticbot · 2022-01-05T22:26:19Z

Caution: This PR has changes that must be merged to WordPress.com
Hello alshakero! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D72574-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

github-actions · 2022-01-05T22:27:31Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
⚠️ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: February 1, 2022.
Scheduled code freeze: January 24, 2022.

jeherve

TIL about Descript! It seems nice. I left a bit of feedback below.

jeherve · 2022-01-06T07:18:43Z

projects/plugins/jetpack/modules/shortcodes/descript.php

+		'id'     => $id,
+		'width'  => $width,
+		'height' => $height,


I think it would be nice if we could cast those as int / escape if possible.

jeherve · 2022-01-06T07:22:30Z

projects/plugins/jetpack/modules/shortcodes/descript.php

+	// Descript rejects any request with the dnt query param, this removes the dnt from the oembed request.
+	add_filter( 'oembed_fetch_url', 'jetpack_descript_remove_dnt' );


This is a bit annoying. I wonder if we should opt to not embed, but only display a link when dnt is enabled? This way we would honor dnt at all times, and not just ignore it. I think someone who enabled dnt may prefer that over being tracked without knowing about it?

Hello, I'm on the Descript engineering team. Good catch! We'll add support for the dnt parameter

Hey @pdesantis! That's awesome. Thanks a lot.

@alshakero just following up, the dnt parameter is now supported

jeherve · 2022-01-06T07:22:59Z

projects/plugins/jetpack/extensions/shared/icons.js

+	src: (
+		<SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 43 43" xmlSpace="preserve">
+			<Path transform="translate(-25)" fillRule="evenodd" d="M37 31.908c0 1.376.893 2.268 2.27 2.268H45.9c3.956 0 7.192-1.337 9.392-3.68H37v1.412zM45.901 9H39.27C37.893 9 37 9.893 37 11.269v1.411h18.294C53.093 10.338 49.857 9 45.9 9zm5.787 16.176c0 1.113.726 1.835 1.845 1.835h3.973c.45-1.117.753-2.345.894-3.671h-4.867c-1.119 0-1.845.722-1.845 1.836zm-5.507-7.166c0 1.113.725 1.836 1.844 1.836H58.4a13.644 13.644 0 00-.894-3.672h-9.481c-1.119 0-1.844.723-1.844 1.836zm1.843 7.166c0-1.114-.726-1.836-1.845-1.836H37v3.671h9.18c1.118 0 1.843-.722 1.843-1.835zm-5.508-7.166c0-1.113-.726-1.836-1.844-1.836H37v3.672h3.672c1.118 0 1.844-.723 1.844-1.836zM83.667 21.209c.296-1.437" clipRule="evenodd" />
+		</SVG>  


We'll need to remove that trailing space so we can merge this.

jeherve · 2022-01-06T07:23:51Z

projects/plugins/jetpack/extensions/extended-blocks/core-embed/descript.js

+/*
+ * New `core/embed` block variation.
+ */
+


Maybe it would be better like this?

Suggested change

/*

* New `core/embed` block variation.

*/

/*

* New `core/embed` block variation.

*/

alshakero · 2022-01-06T16:27:06Z

Thanks for the quick review! I addressed all feedback.

To test the dnt change, please go to chrome://settings/cookies and try with "Do Not Track" on and off to see that when it's on, the shortcode is rendered as a link, when off, it's rendered as an iframe.

I also contacted Descript's support asking them to whitelist the dnt param.

jeherve

Thanks for the quick turn-around. I have one more suggestion, since you're working on DNT?

jeherve · 2022-01-06T17:14:19Z

projects/plugins/jetpack/modules/shortcodes/descript.php

+ *
+ * @return bool
+ */
+function jetpack_descript_is_dnt_enabled() {


We have something similar already:

jetpack/projects/plugins/jetpack/modules/stats.php

Lines 95 to 100 in e50c798

/**

* Checks if filter is set and dnt is enabled.

*

* @return bool

*/

function jetpack_is_dnt_enabled() {

What do you think about extracting this to a shared location, maybe projects/plugins/jetpack/functions.global.php for now?

alshakero · 2022-01-08T19:06:08Z

Hi @pdesantis! Would it be possible to quickly whitelist the dnt flag rather than implementing actual dnt support as a first step? This will unlock this PR without any dirty hacks and removes the time pressure.

I've put this on hold considering the above.

Before calling jetpack_is_dnt_enabled.

yansern

Tested on both WPCOM and standalone WP site.

Descript block works.
Embed block with descript URL works.
Shortcode block works (after enabling shortcodes through Jetpack > Settings > Writing > Compose using Shortcodes)
Using <iframe> on Custom HTML block will not work on WPCOM (likely due to kses restriction, and that is OK). This works fine for standalone WP sites.

jeherve

This works well in my tests. 🚢

jeherve

This works well in my tests. 🚢

…tpack into add/descript-embed-support

github-actions · 2022-01-14T12:00:25Z

Great news! One last step: head over to your WordPress.com diff, D72574-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

alshakero · 2022-01-14T12:40:16Z

r238203-wpcom

alshakero requested a review from jeherve January 5, 2022 22:26

matticbot added the Touches WP.com Files label Jan 5, 2022

alshakero added [Feature] Shortcodes / Embeds [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Type] Feature Request labels Jan 5, 2022

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 5, 2022

alshakero added [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 5, 2022

jeherve assigned alshakero Jan 6, 2022

jeherve requested changes Jan 6, 2022

View reviewed changes

alshakero added [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 6, 2022

jeherve reviewed Jan 6, 2022

View reviewed changes

alshakero added [Status] Blocked / Hold [Status] Needs Team Review and removed [Status] Needs Team Review labels Jan 8, 2022

alshakero added 5 commits January 10, 2022 13:26

[not verified] Add descript embeds support

333d328

[not verified] Add changelog entry

840ee43

[not verified] Add Descript embed block

ce8c4bd

[not verified] Honor users' DNT preference

f497c17

[not verified] Fix linitng errors

ec9886a

alshakero added 11 commits January 12, 2022 15:11

Dont enable DNT filter

bca5c4a

[not verified] Add descript embeds support

64eab37

[not verified] Add changelog entry

4f182f9

[not verified] Add Descript embed block

85da7b1

[not verified] Honor users' DNT preference

ccbabd3

[not verified] Fix linitng errors

25c7541

[not verified] Move jetpack_is_dnt_enabled to globals.php

8e66d22

[not verified] Add jetpack_honor_dnt_header_for_stats filter

91c09dd

Before calling jetpack_is_dnt_enabled.

Address liniting issues

ad8ef81

Remove extraneous newline

f54a006

Dont enable DNT filter

6bbfbdd

yansern force-pushed the add/descript-embed-support branch from 7871b34 to 6bbfbdd Compare January 13, 2022 08:50

yansern previously approved these changes Jan 13, 2022

View reviewed changes

jeherve requested changes Jan 13, 2022

View reviewed changes

jeherve previously approved these changes Jan 13, 2022

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Team Review labels Jan 13, 2022

jeherve added this to the jetpack/10.6 milestone Jan 13, 2022

alshakero added 2 commits January 14, 2022 11:41

Merge branch 'add/descript-embed-support' of github.com:Automattic/je…

37e0788

…tpack into add/descript-embed-support

Remove dnt hackery since Descript now supports it

778833f

alshakero dismissed stale reviews from jeherve and yansern via 778833f January 14, 2022 10:43

[not verified] Revert changes to global functions and stats

af0ad35

jeherve approved these changes Jan 14, 2022

View reviewed changes

jeherve merged commit 86b8d6c into master Jan 14, 2022

jeherve deleted the add/descript-embed-support branch January 14, 2022 11:59

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 14, 2022

leogermani pushed a commit that referenced this pull request Jan 14, 2022

[not verified] Add Descript.com embeds support (#22234)

1108822

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Descript.com embeds support #22234

Add Descript.com embeds support #22234

alshakero commented Jan 5, 2022 •

edited

Loading

matticbot commented Jan 5, 2022

github-actions bot commented Jan 5, 2022 •

edited

Loading

jeherve left a comment

jeherve Jan 6, 2022

jeherve Jan 6, 2022

pdesantis Jan 7, 2022

alshakero Jan 8, 2022 •

edited

Loading

pdesantis Jan 13, 2022

jeherve Jan 6, 2022

jeherve Jan 6, 2022

alshakero commented Jan 6, 2022 •

edited

Loading

jeherve left a comment

jeherve Jan 6, 2022

alshakero commented Jan 8, 2022

yansern left a comment

jeherve left a comment

jeherve left a comment

github-actions bot commented Jan 14, 2022

alshakero commented Jan 14, 2022

		// Descript rejects any request with the dnt query param, this removes the dnt from the oembed request.
		add_filter( 'oembed_fetch_url', 'jetpack_descript_remove_dnt' );

	/**
	* Checks if filter is set and dnt is enabled.
	*
	* @return bool
	*/
	function jetpack_is_dnt_enabled() {

Add Descript.com embeds support #22234

Add Descript.com embeds support #22234

Conversation

alshakero commented Jan 5, 2022 • edited Loading

Changes proposed in this Pull Request:

Does this pull request change what data or activity we track or use?

Testing instructions:

matticbot commented Jan 5, 2022

github-actions bot commented Jan 5, 2022 • edited Loading

jeherve left a comment

Choose a reason for hiding this comment

jeherve Jan 6, 2022

Choose a reason for hiding this comment

jeherve Jan 6, 2022

Choose a reason for hiding this comment

pdesantis Jan 7, 2022

Choose a reason for hiding this comment

alshakero Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

pdesantis Jan 13, 2022

Choose a reason for hiding this comment

jeherve Jan 6, 2022

Choose a reason for hiding this comment

jeherve Jan 6, 2022

Choose a reason for hiding this comment

alshakero commented Jan 6, 2022 • edited Loading

jeherve left a comment

Choose a reason for hiding this comment

jeherve Jan 6, 2022

Choose a reason for hiding this comment

alshakero commented Jan 8, 2022

yansern left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

jeherve left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2022

alshakero commented Jan 14, 2022

alshakero commented Jan 5, 2022 •

edited

Loading

github-actions bot commented Jan 5, 2022 •

edited

Loading

alshakero Jan 8, 2022 •

edited

Loading

alshakero commented Jan 6, 2022 •

edited

Loading