Site Editor Compatibility: Podcast Player block issues in editor and front end views #19578

ramonjd · 2021-04-20T11:37:20Z

This PR addresses Podcast Player block compatibility issues in the site editor, and on the frontend in block-based themes.

Fixes #19483

Changes proposed in this Pull Request:

Adds a check to see if the DOM is loaded before initializing the block on the frontend.
Adds a temporary workaround to inject required assets into the Site Editor iframe.

Jetpack product discussion

Related to #19504

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

No

Testing instructions:

Using a block-based theme, e.g., TT1 add a Podcast block to the site editor. Here's an RSS feed link: https://distributed.blog/category/podcast/feed/ 😄
Once loaded, you should be able to interact with the player as expected.
Add another one.
Both should work
Check that, for every podcast block you add, we're not duplicating the mediaelement-css assets.
Take a look at the frontend. The player(s) should both load.
Check a few other themes for regressions.
Sync the changes to WPCOM and ensure that both the block and site editors work as expected.

matticbot · 2021-04-20T11:37:25Z

Caution: This PR has changes that must be merged to WordPress.com
Hello ramonjd! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D60400-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 · 2021-04-20T11:39:14Z

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 🤖

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: May 4, 2021.
Scheduled code freeze: April 26, 2021.

andrewserong

Thanks for investigating this one @ramonjd. I think this looks like a reasonable temporary workaround. I haven't tested it yet, but just wondered if we should add a couple of extra checks just in case (for some reason?) the expected CSS hasn't been enqueued in the parent document?

projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js

andrewserong · 2021-04-21T05:30:46Z

Oh, it's complaining about a missing changelog entry too 🙂

…til the dom is loaded before we execute DOM-based queries.

I don't like doing this, but there's no other way to enqueue/inject the styles we need for the podcast player into the site editor iframe. So, using JS and the DOM, we find the CSS we need from the head of the parent window and move it into the site editor. YAY. Changelog Adding null checks to the elements we should ensure that the parent-level dom elements are there before we use them

…e now before we load. Abstracting the element copier to be more generic.

glendaviesnz · 2021-04-22T04:26:10Z

@ramon it looks like there might be some funny race condition as it doesn't work for me locally as the useEffect never gets fired with podcastPlayerRef.current defined.

TIL - it seems like you can't rely on ref to cause a rerender when the dom node is assigned/changed and should use a callback ref instead. I switched it to the following locally and that seemed to fix it for me:

	const podCastPlayerRef = useCallback(
		node => {
			if ( node !== null && ! hasMigratedStyles ) {
				maybeCopyElementsToSiteEditorContext(
					[ 'link#mediaelement-css', 'link#wp-mediaelement-css' ],
					node
				);
				setHasMigratedStyles( true );
			}
		},
		[ hasMigratedStyles ]
	);

…ent ref

projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js

In the case of style sheets it won't matter much for our current use case (podcast player) but for other elements we might want to leave the element in both contexts. I can't for the life of me think of a reason why right now.

andrewserong · 2021-04-22T05:15:48Z

This is testing well in FSE across Safari, FF, Chrome, and Edge. Unfortunately, I think this approach introduces a regression in a non-FSE theme in wpcom. Because in that context we are rendering Gutenberg within an iframe from wordpress.com, it looks like it's attempting to access the wrong parent document (e.g. the ref is not within the site editor iframe, but is within the iframe rendered within wordpress.com, so it's now trying to break out of Gutenberg). Here's the error after entering a podcast feed URL in the placeholder:

andrewserong · 2021-04-22T05:19:14Z

projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js

+ */
+export function isElementInIframe( elementRef ) {
+	const { currentWindow } = getLoadContext( elementRef );
+	return currentWindow.self !== currentWindow.top;


We might need to add an additional check either here or within getLoadContext that we're in the correct iframe (the site editor) and not within another version of an iframed editor (e.g. the post/page editor iframed within wordpress.com/post/<blog_name>/<post_id>).

Oh wow, thanks for double checking that. 🤕

I did check in WPCOM when I was initially targeting 'iframe[name="editor-canvas"]'

I'll try to reinstate that test.

THANK YOU

I re-added an iframe name check. Not ideal, but the whole thing is (theoretically) transitory.

I could try to check in the back end and add a global JS variable if it might help future endeavours.

We could also test that the iframe passes the same-origin-policy before we try to start injecting things into the header and bork it all again. :D

…e name. It's not 100% bullet-proof, but aligns with the general theme of ephemerality in relation to these site editor bug fixes

andrewserong

Thanks for implementing all those changes @ramonjd! This is testing nicely for me now. I re-tested in FSE across Safari, FF, Edge, and Chrome, via wordpress.com/site-editor and directly from the the site, and double-checked that the addition of checking for the iframe name resolves the regression 👍

LGTM!

…mixed return types Testing whether we indeed have access to the parent DOM before we try to avoid fatals

ramonjd · 2021-04-22T08:51:08Z

I re-tested in FSE across Safari, FF, Edge, and Chrome, via wordpress.com/site-editor and directly from the the site

Thanks for testing so thoroughly! I added another, small condition to see if the child iframe has access to its parent (same origin). Hopefully will prevent borks if the editor iframe name ever changes.

andrewserong

Thanks for adding the additional check @ramonjd 👍 that looks safer. Just gave this another quick smoke test in the post/page editor (via wordpress.com iframe and via wp-admin) and the site editor (again via wordpress.com/site-editor and the direct /wp-admin url). LGTM! 🎉

jeherve

This tests well for me. 👍

github-actions · 2021-04-26T05:39:22Z

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

Thank you!

ramonjd · 2021-04-26T05:48:50Z

r224746-wpcom

ramonjd · 2021-05-18T01:54:40Z

An api is being added to core Gutenberg that we may be able to use instead of the CSS shuffling: WordPress/gutenberg#31873

matticbot added the Touches WP.com Files label Apr 20, 2021

ramonjd self-assigned this Apr 20, 2021

ramonjd added [Block] Podcast Player [Status] In Progress labels Apr 20, 2021

ramonjd mentioned this pull request Apr 20, 2021

Site Editor Compatibility: Tracking issue for remaining fixes for Jetpack block FSE support #19504

Closed

19 tasks

github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Apr 20, 2021

ramonjd requested a review from a team April 21, 2021 05:22

andrewserong reviewed Apr 21, 2021

View reviewed changes

projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js Outdated Show resolved Hide resolved

projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js Outdated Show resolved Hide resolved

ramonjd force-pushed the fix/site-editor-podcast-player-block branch from 908e318 to e2e6aae Compare April 21, 2021 05:41

ramonjd added 3 commits April 22, 2021 11:39

[not verified] For TT1 block-based theme at least, we have to wait un…

113c09a

…til the dom is loaded before we execute DOM-based queries.

Using the block element to check whether it's sitting inside an ifram…

68eda5a

…e now before we load. Abstracting the element copier to be more generic.

ramonjd force-pushed the fix/site-editor-podcast-player-block branch from e2e6aae to 68eda5a Compare April 22, 2021 03:17

Remove unused method

269d941

Switch from useEffect to ref callback for getting podcast player elem…

a04c762

…ent ref

andrewserong reviewed Apr 22, 2021

View reviewed changes

projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js Outdated Show resolved Hide resolved

Making element removal optional.

48dacca

In the case of style sheets it won't matter much for our current use case (podcast player) but for other elements we might want to leave the element in both contexts. I can't for the life of me think of a reason why right now.

andrewserong reviewed Apr 22, 2021

View reviewed changes

This updates the iframe check to also look for the block editor ifram…

611b1a9

…e name. It's not 100% bullet-proof, but aligns with the general theme of ephemerality in relation to these site editor bug fixes

andrewserong previously approved these changes Apr 22, 2021

View reviewed changes

andrewserong added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Apr 22, 2021

Returning an array from maybeCopyElementsToSiteEditorContext and not …

a27e87c

…mixed return types Testing whether we indeed have access to the parent DOM before we try to avoid fatals

ramonjd dismissed andrewserong’s stale review via a27e87c April 22, 2021 08:49

jeherve added this to the jetpack/9.7 milestone Apr 22, 2021

andrewserong approved these changes Apr 22, 2021

View reviewed changes

jeherve approved these changes Apr 23, 2021

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 23, 2021

ramonjd merged commit 871d75b into master Apr 26, 2021

ramonjd deleted the fix/site-editor-podcast-player-block branch April 26, 2021 05:38

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 26, 2021

ramonjd mentioned this pull request May 20, 2021

Map block: update to use new gutenberg API #19896

Open

ramonjd mentioned this pull request Jun 28, 2021

Site Editor Compatibility: register wp-mediaelement styles for Podcast Player #20193

Merged

anomiex mentioned this pull request Sep 23, 2024

Slideshow block has duplicated caption in the Editor for sites with FSE themes. #39459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Site Editor Compatibility: Podcast Player block issues in editor and front end views #19578

Site Editor Compatibility: Podcast Player block issues in editor and front end views #19578

ramonjd commented Apr 20, 2021 •

edited

Loading

matticbot commented Apr 20, 2021

github-actions bot commented Apr 20, 2021 •

edited

Loading

andrewserong left a comment

andrewserong commented Apr 21, 2021

glendaviesnz commented Apr 22, 2021 •

edited

Loading

andrewserong commented Apr 22, 2021

andrewserong Apr 22, 2021

ramonjd Apr 22, 2021

ramonjd Apr 22, 2021

ramonjd Apr 22, 2021 •

edited

Loading

andrewserong left a comment

ramonjd commented Apr 22, 2021

andrewserong left a comment

jeherve left a comment

github-actions bot commented Apr 26, 2021

ramonjd commented Apr 26, 2021

ramonjd commented May 18, 2021

Site Editor Compatibility: Podcast Player block issues in editor and front end views #19578

Site Editor Compatibility: Podcast Player block issues in editor and front end views #19578

Conversation

ramonjd commented Apr 20, 2021 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

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

Testing instructions:

matticbot commented Apr 20, 2021

github-actions bot commented Apr 20, 2021 • edited Loading

andrewserong left a comment

Choose a reason for hiding this comment

andrewserong commented Apr 21, 2021

glendaviesnz commented Apr 22, 2021 • edited Loading

andrewserong commented Apr 22, 2021

andrewserong Apr 22, 2021

Choose a reason for hiding this comment

ramonjd Apr 22, 2021

Choose a reason for hiding this comment

ramonjd Apr 22, 2021

Choose a reason for hiding this comment

ramonjd Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

andrewserong left a comment

Choose a reason for hiding this comment

ramonjd commented Apr 22, 2021

andrewserong 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 Apr 26, 2021

ramonjd commented Apr 26, 2021

ramonjd commented May 18, 2021

ramonjd commented Apr 20, 2021 •

edited

Loading

github-actions bot commented Apr 20, 2021 •

edited

Loading

glendaviesnz commented Apr 22, 2021 •

edited

Loading

ramonjd Apr 22, 2021 •

edited

Loading