-
Notifications
You must be signed in to change notification settings - Fork 384
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
Add muted message via direct DOM manipulation #6573
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,8 +11,8 @@ import { isFunction, isObject, isString } from 'lodash'; | |
import { __, sprintf } from '@wordpress/i18n'; | ||
import { SelectControl, ToggleControl, Notice, PanelBody } from '@wordpress/components'; | ||
import { InspectorControls } from '@wordpress/block-editor'; | ||
import { select } from '@wordpress/data'; | ||
import { cloneElement, isValidElement } from '@wordpress/element'; | ||
import { select, useSelect } from '@wordpress/data'; | ||
import { cloneElement, isValidElement, useLayoutEffect, useRef } from '@wordpress/element'; | ||
|
||
/** | ||
* Internal dependencies | ||
|
@@ -274,7 +274,7 @@ export const filterBlocksEdit = ( BlockEdit ) => { | |
} | ||
|
||
const EnhancedBlockEdit = function( props ) { | ||
const { attributes: { ampLayout }, name } = props; | ||
const { isSelected, attributes: { ampLayout }, name } = props; | ||
|
||
let inspectorControls; | ||
|
||
|
@@ -283,7 +283,7 @@ export const filterBlocksEdit = ( BlockEdit ) => { | |
} else if ( 'core/image' === name ) { | ||
inspectorControls = setUpImageInspectorControls( props ); | ||
} else if ( 'core/video' === name ) { | ||
inspectorControls = setUpVideoInspectorControls( props ); | ||
inspectorControls = isSelected ? VideoInspectorControls( props ) : null; | ||
} else if ( 0 === name.indexOf( 'core-embed/' ) ) { | ||
inspectorControls = setUpInspectorControls( props ); | ||
} | ||
|
@@ -354,23 +354,68 @@ export const setImageBlockLayoutAttributes = ( props, layout ) => { | |
/** | ||
* Show notice if video player have autoplay enabled and mute is disabled. | ||
* | ||
* @param {Object} props Props. | ||
* @param {Object} props Props. | ||
* @param {boolean} props.isSelected Flag indicating if the block selected. | ||
* @param {Object} props.attributes Block attributes. | ||
* @param {boolean} props.attributes.autoplay Flag indicating if the video has autoplay turned on. | ||
* @param {boolean} props.attributes.muted Flag indicating if the video is muted. | ||
* @return {ReactElement} Inspector Controls. | ||
*/ | ||
export const setUpVideoInspectorControls = ( props ) => { | ||
const { isSelected, attributes: { autoplay, muted } } = props; | ||
export const VideoInspectorControls = ( props ) => { | ||
const { attributes: { autoplay, muted } } = props; | ||
|
||
const message = useRef( null ); | ||
const blockTypeDescription = useSelect( ( _select ) => _select( 'core/blocks' ).getBlockType( 'core/video' )?.description, [] ); | ||
|
||
/** | ||
* Since we are mutating the DOM, it's not the regular `useEffect` hook. | ||
*/ | ||
useLayoutEffect( () => { | ||
const showMessage = true === autoplay && true !== muted; | ||
|
||
if ( showMessage && ! message.current ) { | ||
/** | ||
* When switching over from another block selection, the message is | ||
* tried to be added before the sidebar content changes. This delay | ||
* prevents this issue. | ||
*/ | ||
setTimeout( () => { | ||
/** | ||
* Since there is no actual ID or block type designator exposed | ||
* in the block editor's sidebar DOM, we need to use to a hack. | ||
* In order to determine the video settings DOM container we are | ||
* first searching for a block type description. Once we have it | ||
* we traverse through the DOM tree to finally find out the | ||
* "Muted" toggle container. | ||
*/ | ||
const blockInspector = [ ...document.querySelectorAll( '.edit-post-sidebar .block-editor-block-card__description' ) ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. QUESTION: why no jQuery? 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why use jQuery when few vanilla JS do trick? 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was really hard to resist the temptation 😅 |
||
?.find( ( description ) => description.textContent === blockTypeDescription )?.closest( '.block-editor-block-inspector' ); | ||
|
||
if ( ! blockInspector ) { | ||
return; | ||
} | ||
|
||
if ( ! isSelected ) { | ||
return null; | ||
} | ||
const mutedToggleContainerIndex = 3; | ||
const mutedToggleContainer = blockInspector.querySelector( '.editor-video-poster-control' )?.parentElement.children[ mutedToggleContainerIndex ]; | ||
|
||
if ( ! mutedToggleContainer ) { | ||
return; | ||
} | ||
|
||
message.current = document.createElement( 'p' ); | ||
message.current.classList.add( 'amp-video-autoplay-notice' ); | ||
message.current.textContent = __( 'Autoplay will cause the video to be muted in many browsers to prevent a poor user experience. It will be muted in AMP for this reason as well.', 'amp' ); | ||
|
||
mutedToggleContainer.append( message.current ); | ||
}, 1 ); | ||
} else if ( ! showMessage && message.current ) { | ||
message.current.remove(); | ||
message.current = null; | ||
} | ||
}, [ autoplay, blockTypeDescription, muted ] ); | ||
|
||
return ( | ||
<InspectorControls> | ||
{ ( true === autoplay && true !== muted ) && ( | ||
<PanelBody className="amp-video-autoplay-notice"> | ||
{ __( 'Autoplay will mute the video player by default in AMP mode.', 'amp' ) } | ||
</PanelBody> | ||
) } | ||
<PanelBody title={ __( 'AMP Settings', 'amp' ) }> | ||
<AmpLayoutControl { ...props } /> | ||
<AmpNoloadingToggle { ...props } /> | ||
|
@@ -379,8 +424,12 @@ export const setUpVideoInspectorControls = ( props ) => { | |
); | ||
}; | ||
|
||
setUpVideoInspectorControls.propTypes = { | ||
VideoInspectorControls.propTypes = { | ||
isSelected: PropTypes.bool, | ||
attributes: PropTypes.shape( { | ||
autoplay: PropTypes.bool, | ||
muted: PropTypes.bool, | ||
} ), | ||
}; | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { | ||
createNewPost, | ||
ensureSidebarOpened, | ||
insertBlock, | ||
} from '@wordpress/e2e-test-utils'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { | ||
clickButton, | ||
getBlockEditorSidebarToggle, | ||
uploadMedia, | ||
} from '../../utils'; | ||
|
||
const sampleVideo = 'sample-video.mp4'; | ||
const autoplayNotice = 'Autoplay may cause usability issues for some users.'; | ||
const mutedNotice = 'Autoplay will cause the video to be muted in many browsers to prevent a poor user experience. It will be muted in AMP for this reason as well.'; | ||
|
||
/** | ||
* Tests the notices for the featured image. | ||
*/ | ||
describe( 'Video Block Muted Notice', () => { | ||
beforeEach( async () => { | ||
await createNewPost( { postType: 'post' } ); | ||
} ); | ||
|
||
it( 'displays a message only if the autoplay is turned on and the muted option is off', async () => { | ||
await insertBlock( 'Video' ); | ||
await page.waitForSelector( '.wp-block-video .block-editor-media-placeholder' ); | ||
await clickButton( 'Media Library' ); | ||
await uploadMedia( sampleVideo ); | ||
await page.waitForSelector( 'button.media-button-select:not([disabled])' ); | ||
await expect( page ).toClick( 'button.media-button-select:not([disabled])' ); | ||
|
||
let [ autoplayContainer, autoplayInput ] = await getBlockEditorSidebarToggle( 'Autoplay' ); | ||
let [ mutedContainer, mutedInput ] = await getBlockEditorSidebarToggle( 'Muted' ); | ||
|
||
// Confirm both autoplay and muted are off. | ||
await expect( await autoplayInput.evaluate( ( node ) => node.checked ) ).toBe( false ); | ||
await expect( await mutedInput.evaluate( ( node ) => node.checked ) ).toBe( false ); | ||
await expect( await autoplayContainer.evaluate( ( node ) => node.textContent ) ).not.toMatch( autoplayNotice ); | ||
await expect( await mutedContainer.evaluate( ( node ) => node.textContent ) ).not.toMatch( mutedNotice ); | ||
|
||
await autoplayInput.click(); | ||
|
||
// Check if both notices are displayed after turning autoplay on. | ||
await expect( await autoplayInput.evaluate( ( node ) => node.checked ) ).toBe( true ); | ||
await expect( await mutedInput.evaluate( ( node ) => node.checked ) ).toBe( false ); | ||
await expect( await autoplayContainer.evaluate( ( node ) => node.textContent ) ).toMatch( autoplayNotice ); | ||
await expect( await mutedContainer.evaluate( ( node ) => node.textContent ) ).toMatch( mutedNotice ); | ||
|
||
// Insert new block so that the sidebar content changes. | ||
await insertBlock( 'Code' ); | ||
await page.waitForSelector( '.wp-block-code' ); | ||
await ensureSidebarOpened(); | ||
|
||
// Switch back to the video block and confirm messages have been persisted. | ||
await expect( page ).toClick( '.wp-block-video' ); | ||
|
||
[ autoplayContainer, autoplayInput ] = await getBlockEditorSidebarToggle( 'Autoplay' ); | ||
[ mutedContainer, mutedInput ] = await getBlockEditorSidebarToggle( 'Muted' ); | ||
|
||
await expect( await autoplayInput.evaluate( ( node ) => node.checked ) ).toBe( true ); | ||
await expect( await mutedInput.evaluate( ( node ) => node.checked ) ).toBe( false ); | ||
await expect( await autoplayContainer.evaluate( ( node ) => node.textContent ) ).toMatch( autoplayNotice ); | ||
await expect( await mutedContainer.evaluate( ( node ) => node.textContent ) ).toMatch( mutedNotice ); | ||
|
||
await mutedInput.click(); | ||
|
||
await expect( await autoplayInput.evaluate( ( node ) => node.checked ) ).toBe( true ); | ||
await expect( await mutedInput.evaluate( ( node ) => node.checked ) ).toBe( true ); | ||
await expect( await autoplayContainer.evaluate( ( node ) => node.textContent ) ).toMatch( autoplayNotice ); | ||
await expect( await mutedContainer.evaluate( ( node ) => node.textContent ) ).not.toMatch( mutedNotice ); | ||
|
||
await autoplayInput.click(); | ||
|
||
await expect( await autoplayInput.evaluate( ( node ) => node.checked ) ).toBe( false ); | ||
await expect( await mutedInput.evaluate( ( node ) => node.checked ) ).toBe( true ); | ||
await expect( await autoplayContainer.evaluate( ( node ) => node.textContent ) ).not.toMatch( autoplayNotice ); | ||
await expect( await mutedContainer.evaluate( ( node ) => node.textContent ) ).not.toMatch( mutedNotice ); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/** | ||
* Waits for and returns a block edtior sidebar toggle input and container handles. | ||
* | ||
* @param {string} label The toggle label. | ||
*/ | ||
export async function getBlockEditorSidebarToggle( label ) { | ||
const containerXpath = `div[contains(@class, 'components-toggle-control')][.//label[contains(text(), '${ label }')]]`; | ||
|
||
await page.waitForXPath( `//${ containerXpath }` ); | ||
|
||
const [ containerHandle ] = await page.$x( `//${ containerXpath }` ); | ||
const [ inputHandle ] = await page.$x( `//input[./ancestor-or-self::${ containerXpath }]` ); | ||
|
||
return [ containerHandle, inputHandle ]; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
export { clickButton } from './click-button'; | ||
export { getBlockEditorSidebarToggle } from './get-block-editor-sidebar-toggle'; | ||
export { uploadMedia } from './upload-media'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works perfectly but my only concern is how fragile this would be going forward whenever the video block is to be updated in the future. @delawski could you add an E2E test to verify that the message is shown under the muted toggle? That way we can catch whenever something goes awry with the DOM mutation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a very good idea! I'll take set up an E2E test for that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An E2E test has been added in 3755725.