Skip to content
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.

Sign up for GitHub

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

Require an initial click on embed previews for interactivity #12981

Merged
merged 9 commits into from
Feb 11, 2019
Merged

Require an initial click on embed previews for interactivity #12981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1e46e7e
Require an initial click on embed previews for interactivity
notnownikki Dec 18, 2018
ebd88ee
Handle keyboard focus
notnownikki Dec 21, 2018
4053879
Overlay color for demo
notnownikki Dec 27, 2018
6435f68
Merge branch 'master' into update/embed-interactive-content-requires-…
notnownikki Feb 1, 2019
bb1336e
Remove dialog role, make overlay transparent
notnownikki Feb 1, 2019
b2f4481
Merge branch 'update/embed-interactive-content-requires-initial-click…
notnownikki Feb 1, 2019
b37acae
Review fixes
notnownikki Feb 5, 2019
391e7ee
Merge branch 'master' into update/embed-interactive-content-requires-…
notnownikki Feb 5, 2019
a17ede1
Only update state from props when interactive changes
notnownikki Feb 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/block-library/src/embed/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,12 @@
word-break: break-word;
}
}

.block-library-embed__interactive-overlay {
position: absolute;
top: 0;
left: 0;
right: 0;
bottom: 0;
opacity: 0;
}
127 changes: 85 additions & 42 deletions packages/block-library/src/embed/embed-preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,58 +17,101 @@ 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';

const EmbedPreview = ( props ) => {
const { preview, url, type, caption, onCaptionChange, isSelected, className, icon, label } = props;
const { scripts } = preview;
class EmbedPreview extends Component {
constructor() {
super( ...arguments );
this.hideOverlay = this.hideOverlay.bind( this );
this.state = {
interactive: false,
};
}

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' );
static getDerivedStateFromProps( nextProps, state ) {
if ( ! nextProps.isSelected && state.interactive ) {
// 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 };
}

const embedWrapper = 'wp-embed' === type ? (
<WpEmbedPreview
html={ html }
/>
) : (
<div className="wp-block-embed__wrapper">
<SandBox
return null;
}

hideOverlay() {
// This is called onMouseUp on the overlay. We can't respond to the `isSelected` prop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice comment 👍 I might encourage to avoid using "Gutenberg" phrasing, when "block editor" (or just "editor") is sufficient / non-plugin-specific when referring to the editor.

// 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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: You can disable multiple rules in a single eslint-disable statement, but as written here seems like it could be better anyways to prevent the line from being too long (unsure if that was the intention).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, just for readability :)

const embedWrapper = 'wp-embed' === type ? (
<WpEmbedPreview
html={ html }
scripts={ scripts }
title={ iframeTitle }
type={ sandboxClassnames }
/>
</div>
);

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
) : (
<div className="wp-block-embed__wrapper">
<SandBox
html={ html }
scripts={ scripts }
title={ iframeTitle }
type={ sandboxClassnames }
onFocus={ this.hideOverlay }
/>
) }
</figure>
);
};
{ ! 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;
3 changes: 2 additions & 1 deletion packages/components/src/sandbox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ class Sandbox extends Component {
}

render() {
const { title } = this.props;
const { title, onFocus } = this.props;

return (
<FocusableIframe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: It's curious to me that we would have opted to use FocusableIframe here previously without having assigned an onFocus, since... the entire point of the component would be to detect onFocus.

Expand All @@ -207,6 +207,7 @@ class Sandbox extends Component {
className="components-sandbox"
sandbox="allow-scripts allow-same-origin allow-presentation"
onLoad={ this.trySandbox }
onFocus={ onFocus }
width={ Math.ceil( this.state.width ) }
height={ Math.ceil( this.state.height ) } />
);
Expand Down