From 113c09a3c2384dce24960f9d6463472ca7572bc9 Mon Sep 17 00:00:00 2001 From: ramonjd Date: Tue, 20 Apr 2021 21:34:26 +1000 Subject: [PATCH 1/8] [not verified] For TT1 block-based theme at least, we have to wait until the dom is loaded before we execute DOM-based queries. --- .../extensions/blocks/podcast-player/view.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/projects/plugins/jetpack/extensions/blocks/podcast-player/view.js b/projects/plugins/jetpack/extensions/blocks/podcast-player/view.js index e6a774c072da8..0317f92e13cc3 100644 --- a/projects/plugins/jetpack/extensions/blocks/podcast-player/view.js +++ b/projects/plugins/jetpack/extensions/blocks/podcast-player/view.js @@ -96,9 +96,11 @@ const initializeBlock = function ( id ) { block.setAttribute( 'data-jetpack-block-initialized', 'true' ); }; -document - .querySelectorAll( '.wp-block-jetpack-podcast-player:not([data-jetpack-block-initialized])' ) - .forEach( player => { - player.classList.remove( 'is-default' ); - initializeBlock( player.id ); - } ); +document.addEventListener( 'DOMContentLoaded', () => { + document + .querySelectorAll( '.wp-block-jetpack-podcast-player:not([data-jetpack-block-initialized])' ) + .forEach( player => { + player.classList.remove( 'is-default' ); + initializeBlock( player.id ); + } ); +} ); From 79333a88f9982716cb3cf9079238638deb1f744c Mon Sep 17 00:00:00 2001 From: ramonjd Date: Wed, 21 Apr 2021 15:12:43 +1000 Subject: [PATCH 2/8] [not verified] # This is a combination of 3 commits. 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 --- .../fix-site-editor-podcast-player-block | 4 ++++ .../extensions/blocks/podcast-player/edit.js | 3 ++- .../extensions/blocks/podcast-player/utils.js | 23 +++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 projects/plugins/jetpack/changelog/fix-site-editor-podcast-player-block diff --git a/projects/plugins/jetpack/changelog/fix-site-editor-podcast-player-block b/projects/plugins/jetpack/changelog/fix-site-editor-podcast-player-block new file mode 100644 index 0000000000000..3c8d7a418cdc0 --- /dev/null +++ b/projects/plugins/jetpack/changelog/fix-site-editor-podcast-player-block @@ -0,0 +1,4 @@ +Significance: patch +Type: compat + +Ensure compatibility with the Site Editor by injecting required media assets into the Site Editor canvas, and loading frontend scripts onDomReady diff --git a/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js b/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js index 9ae6439e18236..2b27e22288402 100644 --- a/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js +++ b/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js @@ -45,7 +45,7 @@ import { queueMusic } from './icons/'; import { isAtomicSite, isSimpleSite } from '../../shared/site-type-utils'; import attributesValidation from './attributes'; import PodcastPlayer from './components/podcast-player'; -import { makeCancellable } from './utils'; +import { makeCancellable, __maybeInjectStylesIntoSiteEditor } from './utils'; import { fetchPodcastFeed } from './api'; import { podcastPlayerReducer, actions } from './state'; import { applyFallbackStyles } from '../../shared/apply-fallback-styles'; @@ -157,6 +157,7 @@ const PodcastPlayerEdit = ( { ); useEffect( () => { + __maybeInjectStylesIntoSiteEditor(); return () => { cancellableFetch?.current?.cancel?.(); }; diff --git a/projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js b/projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js index 652f789ba2523..ee482a88c7dc7 100644 --- a/projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js +++ b/projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js @@ -123,3 +123,26 @@ export const getColorsObject = memoize( generateColorsObject, config => { // Cache key is a string with all arguments joined into one string. return Object.values( config ).join(); } ); + +/** + * A temporary work-around to inject the styles we need for the media player into the site editor. + */ +export function __maybeInjectStylesIntoSiteEditor() { + // Check to see if we're in the site editor. + const iframe = document.querySelector( 'iframe[name="editor-canvas"]' ); + + if ( iframe ) { + const mediaElementCSS = document.querySelector( 'link#mediaelement-css' ); + const wpMediaElementCSS = document.querySelector( 'link#wp-mediaelement-css' ); + + if ( mediaElementCSS && ! iframe.contentDocument.querySelector( 'link#mediaelement-css' ) ) { + iframe.contentDocument.head.appendChild( mediaElementCSS.cloneNode() ); + mediaElementCSS.remove(); + } + + if ( wpMediaElementCSS && ! iframe.contentDocument.querySelector( 'link#wp-mediaelement-css' ) ) { + iframe.contentDocument.head.appendChild( wpMediaElementCSS.cloneNode() ); + wpMediaElementCSS.remove(); + } + } +} From 68eda5af6478362248a40417a1b285d7e0eb0c64 Mon Sep 17 00:00:00 2001 From: ramonjd Date: Thu, 22 Apr 2021 13:17:17 +1000 Subject: [PATCH 3/8] Using the block element to check whether it's sitting inside an iframe now before we load. Abstracting the element copier to be more generic. --- .../extensions/blocks/podcast-player/edit.js | 24 +++++-- .../shared/block-editor-asset-loader.js | 64 +++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js diff --git a/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js b/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js index 2b27e22288402..0d229dec4f7a8 100644 --- a/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js +++ b/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js @@ -7,7 +7,7 @@ import { debounce, noop } from 'lodash'; /** * WordPress dependencies */ -import { useCallback, useEffect, useRef, useReducer } from '@wordpress/element'; +import { useCallback, useEffect, useState, useRef, useReducer } from '@wordpress/element'; import { Button, ExternalLink, @@ -45,11 +45,12 @@ import { queueMusic } from './icons/'; import { isAtomicSite, isSimpleSite } from '../../shared/site-type-utils'; import attributesValidation from './attributes'; import PodcastPlayer from './components/podcast-player'; -import { makeCancellable, __maybeInjectStylesIntoSiteEditor } from './utils'; +import { makeCancellable } from './utils'; import { fetchPodcastFeed } from './api'; import { podcastPlayerReducer, actions } from './state'; import { applyFallbackStyles } from '../../shared/apply-fallback-styles'; import { PODCAST_FEED, EMBED_BLOCK } from './constants'; +import { maybeCopyElementsToSiteEditorContext } from '../../shared/block-editor-asset-loader'; const DEFAULT_MIN_ITEMS = 1; const DEFAULT_MAX_ITEMS = 10; @@ -94,6 +95,9 @@ const PodcastPlayerEdit = ( { const playerId = `jetpack-podcast-player-block-${ instanceId }`; + const podCastPlayerRef = useRef(); + const [ hasMigratedStyles, setHasMigratedStyles ] = useState( false ); + // State. const cancellableFetch = useRef(); const [ { selectedGuid, checkUrl, ...state }, dispatch ] = useReducer( podcastPlayerReducer, { @@ -156,13 +160,25 @@ const PodcastPlayerEdit = ( { [ replaceWithEmbedBlock, setAttributes ] ); + // Call once on mount or unmount (the return callback). useEffect( () => { - __maybeInjectStylesIntoSiteEditor(); return () => { cancellableFetch?.current?.cancel?.(); }; }, [] ); + // The Podcast player audio element requires wpmedialement styles. + // These aren't available in the Site Editor context, so we have to copy them in. + useEffect( () => { + if ( ! hasMigratedStyles && podCastPlayerRef.current ) { + maybeCopyElementsToSiteEditorContext( + [ 'link#mediaelement-css', 'link#wp-mediaelement-css' ], + podCastPlayerRef.current + ); + setHasMigratedStyles( true ); + } + }, [ hasMigratedStyles ] ); + // Load RSS feed initially and when the feed or selected episode changes. useEffect( () => { // Don't do anything if no url is set. @@ -375,7 +391,7 @@ const PodcastPlayerEdit = ( { -
+
{ + const parentElementToCopy = parentDoc.querySelector( selector ); + const isElementAlreadyPresentInCurrentWindow = !! currentDoc.querySelector( selector ); + if ( parentElementToCopy && ! isElementAlreadyPresentInCurrentWindow ) { + currentDoc.head.appendChild( parentElementToCopy.cloneNode() ); + parentElementToCopy.remove(); + return true; + } + return false; + } ); + + return results; + } +} From 269d941b6da6c3e4144c4f1955ee9c7cf2d98973 Mon Sep 17 00:00:00 2001 From: ramonjd Date: Thu, 22 Apr 2021 13:18:23 +1000 Subject: [PATCH 4/8] Remove unused method --- .../extensions/blocks/podcast-player/utils.js | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js b/projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js index ee482a88c7dc7..652f789ba2523 100644 --- a/projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js +++ b/projects/plugins/jetpack/extensions/blocks/podcast-player/utils.js @@ -123,26 +123,3 @@ export const getColorsObject = memoize( generateColorsObject, config => { // Cache key is a string with all arguments joined into one string. return Object.values( config ).join(); } ); - -/** - * A temporary work-around to inject the styles we need for the media player into the site editor. - */ -export function __maybeInjectStylesIntoSiteEditor() { - // Check to see if we're in the site editor. - const iframe = document.querySelector( 'iframe[name="editor-canvas"]' ); - - if ( iframe ) { - const mediaElementCSS = document.querySelector( 'link#mediaelement-css' ); - const wpMediaElementCSS = document.querySelector( 'link#wp-mediaelement-css' ); - - if ( mediaElementCSS && ! iframe.contentDocument.querySelector( 'link#mediaelement-css' ) ) { - iframe.contentDocument.head.appendChild( mediaElementCSS.cloneNode() ); - mediaElementCSS.remove(); - } - - if ( wpMediaElementCSS && ! iframe.contentDocument.querySelector( 'link#wp-mediaelement-css' ) ) { - iframe.contentDocument.head.appendChild( wpMediaElementCSS.cloneNode() ); - wpMediaElementCSS.remove(); - } - } -} From a04c7622d2eb0035abc0fc686bab5ca153d719cb Mon Sep 17 00:00:00 2001 From: Glen Davies Date: Thu, 22 Apr 2021 16:33:59 +1200 Subject: [PATCH 5/8] Switch from useEffect to ref callback for getting podcast player element ref --- .../extensions/blocks/podcast-player/edit.js | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js b/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js index 0d229dec4f7a8..da9bbba14145d 100644 --- a/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js +++ b/projects/plugins/jetpack/extensions/blocks/podcast-player/edit.js @@ -95,7 +95,6 @@ const PodcastPlayerEdit = ( { const playerId = `jetpack-podcast-player-block-${ instanceId }`; - const podCastPlayerRef = useRef(); const [ hasMigratedStyles, setHasMigratedStyles ] = useState( false ); // State. @@ -169,15 +168,18 @@ const PodcastPlayerEdit = ( { // The Podcast player audio element requires wpmedialement styles. // These aren't available in the Site Editor context, so we have to copy them in. - useEffect( () => { - if ( ! hasMigratedStyles && podCastPlayerRef.current ) { - maybeCopyElementsToSiteEditorContext( - [ 'link#mediaelement-css', 'link#wp-mediaelement-css' ], - podCastPlayerRef.current - ); - setHasMigratedStyles( true ); - } - }, [ hasMigratedStyles ] ); + const podCastPlayerRef = useCallback( + node => { + if ( node !== null && ! hasMigratedStyles ) { + maybeCopyElementsToSiteEditorContext( + [ 'link#mediaelement-css', 'link#wp-mediaelement-css' ], + node + ); + setHasMigratedStyles( true ); + } + }, + [ hasMigratedStyles ] + ); // Load RSS feed initially and when the feed or selected episode changes. useEffect( () => { From 48dacca6f90cb3b10d4353f4d265a77bb02d82f9 Mon Sep 17 00:00:00 2001 From: Ramon Date: Thu, 22 Apr 2021 14:52:12 +1000 Subject: [PATCH 6/8] Making element removal optional. 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. --- .../shared/block-editor-asset-loader.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js b/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js index cb45ae0162e1c..456b5cbdfd50f 100644 --- a/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js +++ b/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js @@ -32,14 +32,15 @@ export function isElementInIframe( elementRef ) { * For use until Gutenberg offers a standardized way of including enqueued/3rd-party assets. * Target usage is the Podcast Playerblock: projects/plugins/jetpack/extensions/blocks/podcast-player/. * - * @param {Array} elementSelectors - An array of selectors, e.g., [ '#conan', '#robocop' ] - * @param {HTMLElement} elementRef - The current element. - * @returns {Array} - An array of successfully migrated selectors; + * @param {Array} elementSelectors - An array of selectors, e.g., [ '#conan', '#robocop' ] + * @param {HTMLElement} elementRef - The current element. + * @param {boolean} shouldRemoveSource - Optional. Whether to remove the source element in the parent frame. + * @returns {Array} - An array of successfully migrated selectors; */ -export function maybeCopyElementsToSiteEditorContext( elementSelectors = [], elementRef ) { +export function maybeCopyElementsToSiteEditorContext( elementSelectors, elementRef, shouldRemoveSource = false ) { // Check to see if we're in an iframe, e.g., the Site Editor. // If not, do nothing. - if ( ! elementRef || ! elementSelectors.length || ! isElementInIframe( elementRef ) ) { + if ( ! elementRef || ( ! elementSelectors && ! elementSelectors.length ) || ! isElementInIframe( elementRef ) ) { return; } @@ -53,7 +54,9 @@ export function maybeCopyElementsToSiteEditorContext( elementSelectors = [], ele const isElementAlreadyPresentInCurrentWindow = !! currentDoc.querySelector( selector ); if ( parentElementToCopy && ! isElementAlreadyPresentInCurrentWindow ) { currentDoc.head.appendChild( parentElementToCopy.cloneNode() ); - parentElementToCopy.remove(); + if ( shouldRemoveSource ) { + parentElementToCopy.remove(); + } return true; } return false; From 611b1a9d22d1195e54fc3443e85288d6eb610d38 Mon Sep 17 00:00:00 2001 From: ramonjd Date: Thu, 22 Apr 2021 16:17:49 +1000 Subject: [PATCH 7/8] This updates the iframe check to also look for the block editor iframe name. It's not 100% bullet-proof, but aligns with the general theme of ephemerality in relation to these site editor bug fixes --- .../shared/block-editor-asset-loader.js | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js b/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js index 456b5cbdfd50f..23d14c5c8bc11 100644 --- a/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js +++ b/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js @@ -13,15 +13,15 @@ export function getLoadContext( elementRef ) { } /** - * Returns whether a given element is contained within an iframe. - * Useful to check if a block sits inside the Site Editor. + * Returns whether a given element is contained within an Editor iframe. + * See: https://github.com/WordPress/gutenberg/blob/bee52e68292357011a799f067ad47aa1c1d710e1/packages/block-editor/src/components/iframe/index.js * * @param {HTMLElement} elementRef - The element whose context we want to return. - * @returns {boolean} - Whether `elementRef` is contained within an iframe. + * @returns {boolean} - Whether `elementRef` is contained within an Editor iframe. */ -export function isElementInIframe( elementRef ) { +export function isElementInEditorIframe( elementRef ) { const { currentWindow } = getLoadContext( elementRef ); - return currentWindow.self !== currentWindow.top; + return currentWindow.name === 'editor-canvas' && currentWindow.self !== currentWindow.top; } /** @@ -37,10 +37,18 @@ export function isElementInIframe( elementRef ) { * @param {boolean} shouldRemoveSource - Optional. Whether to remove the source element in the parent frame. * @returns {Array} - An array of successfully migrated selectors; */ -export function maybeCopyElementsToSiteEditorContext( elementSelectors, elementRef, shouldRemoveSource = false ) { +export function maybeCopyElementsToSiteEditorContext( + elementSelectors, + elementRef, + shouldRemoveSource = false +) { // Check to see if we're in an iframe, e.g., the Site Editor. // If not, do nothing. - if ( ! elementRef || ( ! elementSelectors && ! elementSelectors.length ) || ! isElementInIframe( elementRef ) ) { + if ( + ! elementRef || + ( ! elementSelectors && ! elementSelectors.length ) || + ! isElementInEditorIframe( elementRef ) + ) { return; } From a27e87c6d74838ff6ea40e4e2efed16a2466650d Mon Sep 17 00:00:00 2001 From: ramonjd Date: Thu, 22 Apr 2021 18:48:53 +1000 Subject: [PATCH 8/8] Returning an array from maybeCopyElementsToSiteEditorContext and not mixed return types Testing whether we indeed have access to the parent DOM before we try to avoid fatals --- .../shared/block-editor-asset-loader.js | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js b/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js index 23d14c5c8bc11..51576f5a9ee08 100644 --- a/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js +++ b/projects/plugins/jetpack/extensions/shared/block-editor-asset-loader.js @@ -24,6 +24,20 @@ export function isElementInEditorIframe( elementRef ) { return currentWindow.name === 'editor-canvas' && currentWindow.self !== currentWindow.top; } +/** + * Returns whether a iframe has domain access to its parent. + * + * @param {HTMLElement} currentWindow - The window context for which we want to test access. + * @returns {boolean} - Whether we have access to the parent window. + */ +function canIframeAccessParentWindow( currentWindow ) { + try { + return !! currentWindow?.parent?.location.href; + } catch ( e ) { + return false; + } +} + /** * This function will check if the current element (e.g., a block) sits inside an Iframe (e.g., the Site Editor) * and tries to move elements from the parent window to the iframe. @@ -42,6 +56,7 @@ export function maybeCopyElementsToSiteEditorContext( elementRef, shouldRemoveSource = false ) { + let results = []; // Check to see if we're in an iframe, e.g., the Site Editor. // If not, do nothing. if ( @@ -49,12 +64,16 @@ export function maybeCopyElementsToSiteEditorContext( ( ! elementSelectors && ! elementSelectors.length ) || ! isElementInEditorIframe( elementRef ) ) { - return; + return results; } const { currentDoc, currentWindow } = getLoadContext( elementRef ); + + if ( ! canIframeAccessParentWindow( currentWindow ) ) { + return results; + } + const parentDoc = currentWindow?.parent?.document; - let results = []; if ( currentDoc && parentDoc ) { results = elementSelectors.filter( selector => {