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

Fix focus issue on embed block #517

Merged
merged 3 commits into from
Apr 26, 2017
Merged

Fix focus issue on embed block #517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c4d6e78
Fix focus issue on embed block
ellatrix Apr 26, 2017
2f9d767
Add invisible iframe overlay to prevent clicking iframed controls
ellatrix Apr 26, 2017
2c52363
Sync pot file
ellatrix Apr 26, 2017
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
10 changes: 7 additions & 3 deletions blocks/library/embed/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,21 @@ registerBlock( 'core/embed', {
caption: children( 'figcaption' )
},

edit( { attributes, isSelected, setAttributes } ) {
edit( { attributes, setAttributes, focus, setFocus } ) {
const { url, title, caption } = attributes;

return (
<figure>
<iframe src={ url } title={ title } />
{ caption || isSelected ? (
<div className="iframe-overlay">
<iframe src={ url } title={ title } />
Copy link
Member

@aduth aduth Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Per our Slack conversation, perhaps in order to show an icon in the overlay, we could render the overlay within the relative wrapper, only if the block has not received focus:

<figure>
	<div className="embed-wrapper">
		{ ! focus && (
			<div className="embed-overlay">
				<Dashicon icon={ /* ... */ } />
			</div>
		) }
		<iframe { /* ... */ } />
	</div>
	{ /* ... */ }
</figure>

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, this could work too. I'm just wondering how you know how high the iframe is.

Copy link
Member

Choose a reason for hiding this comment

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

The height? I think it should work just the same as you have now, if you apply position: relative; to .embed-wrapper and position: absolute; top: 0; right: 0; bottom: 0; left: 0; to .embed-overlay.

Copy link
Member Author

Choose a reason for hiding this comment

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

How is .embed-overlay the same as iframe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sorry @aduth, I did not see .embed-wrapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of a setting, they could just add a wrapper to place it where they want. Maybe this could be a separate React component like <IframeOverlay>.

Copy link
Member

@aduth aduth Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think we still could. In the above example, there's little difference between the .embed-overlay and the :before pseudo-element.

The problem with @rileybrook / @jasmussen's mockups at #483 (comment) is that it assumes we know that the embed is a video type. How we get this information is still a little unclear at this point. The embed block's implementation might be able to determine its own content type through proposed APIs like the oembed/1.0/proxy endpoint, which could return the provider and let us compare against a "known" mapping of provider -> type. This is not great either, though even server-side we don't really have a good sense of the type of a supported embed provider.

Otherwise we could default to something generic.

Copy link
Member

Choose a reason for hiding this comment

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

Are we talking about having many different block types for various sorts of embeds? I'm not sure why this would be a common behavior across blocks, at least to the point of wanting to create an abstraction.

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, I thought this invisible overlay was a good start for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

To your last comment, this overlay is now just to prevent clicks going through iframes, so I named it like that. Not sure if it's intended for wider use.

</div>
{ caption || !! focus ? (
<Editable
tagName="figcaption"
placeholder={ wp.i18n.__( 'Write caption…' ) }
value={ caption }
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { caption: value } ) } />
) : null }
</figure>
Expand Down
18 changes: 18 additions & 0 deletions editor/modes/visual-editor/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,24 @@
&.is-selected:before {
border-color: $light-gray-500;
}

.iframe-overlay {
position: relative;
}

.iframe-overlay:before {
content: '';
display: block;
position: absolute;
top: 0;
right: 0;
bottom: 0;
left: 0;
}

&.is-selected .iframe-overlay:before {
display: none;
}
}

.editor-visual-editor__block-controls {
Expand Down
2 changes: 1 addition & 1 deletion languages/gutenberg.pot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ msgstr ""
msgid "Embed"
msgstr ""

#: blocks/library/embed/index.js:31
#: blocks/library/embed/index.js:33
#: blocks/library/image/index.js:81
msgid "Write caption…"
msgstr ""
Expand Down