Require an initial click on embed previews for interactivity #12981
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,58 +17,99 @@ import classnames from 'classnames/dedupe'; | |
| import { __, sprintf } from '@wordpress/i18n'; | ||
| import { Placeholder, SandBox } from '@wordpress/components'; | ||
| import { RichText, BlockIcon } from '@wordpress/editor'; | ||
| import { Component } from '@wordpress/element'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import WpEmbedPreview from './wp-embed-preview'; | ||
|
|
||
| class EmbedPreview extends Component { | ||
| constructor() { | ||
| super( ...arguments ); | ||
| this.hideOverlay = this.hideOverlay.bind( this ); | ||
| this.state = { | ||
| interactive: false, | ||
| }; | ||
| } | ||
|
|
||
| static getDerivedStateFromProps( nextProps ) { | ||
| if ( ! nextProps.isSelected ) { | ||
|
There was a problem hiding this comment. It's worth noting that this function will be called more than just at the point where it transitions from a state of being selected to being unselected: It could be called hundreds of times for a block which isn't (and may never be) selected. I'm not too familiar with whether React does anything internally to optimize for when the state value doesn't change, but it may be a good idea to also consider the current state when returning a non-
https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops To this point as well, I'm not sure whether There was a problem hiding this comment. The two points of this preceding comment were not addressed. I've verified that a warning would be logged by allowing this function to return Of my own curiosity, I dug deeper to see the work involved in generating a new state value for every render of the component. At the very least, it involves a wasteful shallow clone of the state object: Instead, if the intention is that state only needs to update if the block is both not selected, but has the interactive state, this ought to be expressed as: static getDerivedStateFromProps( props, state ) {
if ( ! props.isSelected && state.interactive ) {
// ...
return { interactive: false };
}
return null;
}This ensures that we only update state when There was a problem hiding this comment. Ah, apologies, this one didn't make it on to the todo list. WIll address and update. There was a problem hiding this comment. This should be correct now. Thank you :) |
||
| // We only want to change this when the block is not selected, because changing it when | ||
| // the block becomes selected makes the overlap disappear too early. Hiding the overlay | ||
| // happens on mouseup when the overlay is clicked. | ||
| return { interactive: false }; | ||
| } | ||
| } | ||
|
|
||
| hideOverlay() { | ||
| // This is called onMouseUp on the overlay. We can't respond to the `isSelected` prop | ||
| // changing, because that happens on mouse down, and the overlay immediately disappears, | ||
| // and the mouse event can end up in the preview content. We can't use onClick on | ||
| // the overlay to hide it either, because then the editor misses the mouseup event, and | ||
| // thinks we're multi-selecting blocks. | ||
| this.setState( { interactive: true } ); | ||
| } | ||
|
|
||
| render() { | ||
| const { preview, url, type, caption, onCaptionChange, isSelected, className, icon, label } = this.props; | ||
| const { scripts } = preview; | ||
| const { interactive } = this.state; | ||
|
|
||
| const html = 'photo' === type ? getPhotoHtml( preview ) : preview.html; | ||
| const parsedHost = parse( url ).host.split( '.' ); | ||
| const parsedHostBaseUrl = parsedHost.splice( parsedHost.length - 2, parsedHost.length - 1 ).join( '.' ); | ||
| const cannotPreview = includes( HOSTS_NO_PREVIEWS, parsedHostBaseUrl ); | ||
| // translators: %s: host providing embed content e.g: www.youtube.com | ||
| const iframeTitle = sprintf( __( 'Embedded content from %s' ), parsedHostBaseUrl ); | ||
| const sandboxClassnames = classnames( type, className, 'wp-block-embed__wrapper' ); | ||
|
|
||
| // Disabled because the overlay div doesn't actually have a role or functionality | ||
| // as far as the user is concerned. We're just catching the first click so that | ||
| // the block can be selected without interacting with the embed preview that the overlay covers. | ||
| /* eslint-disable jsx-a11y/no-noninteractive-element-interactions */ | ||
|
There was a problem hiding this comment. Can you provide as a preceding inline comment a "Disable reason" explaining to a future maintainer why the rule was not needed to be respected here. Otherwise, it will be difficult to track whether it can ever be removed (and often, forcing oneself to explain the reason can serve to deter one from actually disabling, though in this case I think it's fine/reasonable). |
||
| /* eslint-disable jsx-a11y/no-static-element-interactions */ | ||
|
There was a problem hiding this comment. FYI: You can disable multiple rules in a single There was a problem hiding this comment. Yeah, just for readability :) |
||
| const embedWrapper = 'wp-embed' === type ? ( | ||
| <WpEmbedPreview | ||
| html={ html } | ||
| /> | ||
| ) : ( | ||
| <div className="wp-block-embed__wrapper"> | ||
| <SandBox | ||
| html={ html } | ||
| scripts={ scripts } | ||
| title={ iframeTitle } | ||
| type={ sandboxClassnames } | ||
| onFocus={ this.hideOverlay } | ||
| /> | ||
| { ! interactive && <div | ||
| className="block-library-embed__interactive-overlay" | ||
| onMouseUp={ this.hideOverlay } /> } | ||
| </div> | ||
| ); | ||
| /* eslint-enable jsx-a11y/no-static-element-interactions */ | ||
| /* eslint-enable jsx-a11y/no-noninteractive-element-interactions */ | ||
|
|
||
| return ( | ||
| <figure className={ classnames( className, 'wp-block-embed', { 'is-type-video': 'video' === type } ) }> | ||
| { ( cannotPreview ) ? ( | ||
| <Placeholder icon={ <BlockIcon icon={ icon } showColors /> } label={ label }> | ||
| <p className="components-placeholder__error"><a href={ url }>{ url }</a></p> | ||
| <p className="components-placeholder__error">{ __( 'Sorry, this embedded content cannot be previewed in the editor.' ) }</p> | ||
| </Placeholder> | ||
| ) : embedWrapper } | ||
| { ( ! RichText.isEmpty( caption ) || isSelected ) && ( | ||
| <RichText | ||
| tagName="figcaption" | ||
| placeholder={ __( 'Write caption…' ) } | ||
| value={ caption } | ||
| onChange={ onCaptionChange } | ||
| inlineToolbar | ||
| /> | ||
| ) } | ||
| </figure> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| export default EmbedPreview; | ||
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.
Minor: For consistency's sake, could you keep methods which extend the
Componentinterface (e.g. constructor, lifecycle,getDerivedStateFromProps, etc) grouped together toward the top of the component definition? While not yet part of a coding standard, it's a useful convention which has developed around component definitions. I'd like to circle around to adding it as a lint rule at some point in the future.