Add iframe raw handling (e.g. Google Maps) #4660

ellatrix · 2018-01-24T17:18:05Z

Description

This will allow the pasting of iframes when the user can publish unfiltered HTML. When frames are pasted, they are put in an HTML block. We can iterate on this later by creating a Google Maps embed block that would accept the embed code as well for discoverability. See Add google maps as an embed #3970.
It will extract iframes and images from paragraphs so that they can be converted to blocks. Previously an aligned image in the middle of a paragraph would not be converted to an image block.

How Has This Been Tested?

E.g. go to Google Maps, click the share icon, then the embed tab, and copy the embed code (an iframe). Pasting this in Gutenberg as an admin should now work. Ensure that it does not as an author or less.
Put an iframe in a classic text block (through edit HTML). Then edit visually, and go back to edit HTML. The iframe will be wrapped in a paragraph because it is an inline element. Now convert into blocks. The iframe should be put in a separate block. Previously the iframe would be stripped from the paragraph tag.

jaswrks · 2018-01-24T23:12:17Z

blocks/editable/index.js

+
+export default withContext( 'editor' )(
+	( settings ) => ( {
+		unfilteredHTML: settings && settings.unfilteredHTML,


@iseulde Do you think it would be better to use the withAPIData component and check user.data.capabilities.unfiltered_html? example here

Good point, didn't think of that. Will try tomorrow.

jaswrks · 2018-01-24T23:13:43Z

E.g. go to Google Maps, click the share icon, then the embed tab, and copy the embed code (an iframe). Pasting this in Gutenberg as an admin should now work.

I tested this in latest Chrome on macOS and it works! :-)

Ensure that it does not as an editor or less.

I think you mean author, right? If so, this is working correctly. Editors can post unfiltered HTML.

ellatrix · 2018-01-25T12:02:12Z

I think you mean author, right? If so, this is working correctly. Editors can post unfiltered HTML.

Yes, sorry. Didn't realise editors have the cap as well.

I updated the PR, but since the Editable doesn't need to be aware of the post type, it now loads duplicate user data... This is more of a REST API/caching concern though. Not sure how to best fix this.

ellatrix · 2018-01-30T14:08:10Z

If someone could review, that'd be great! :)

jaswrks · 2018-01-30T22:14:45Z

blocks/api/raw-handling/inline-block-corrector.js

+
+	let wrapper = node;
+
+	while ( wrapper.nodeName !== 'P' ) {


I think you might need while( wrapper && wrapper.nodeName ) here, because the final iteration might be a node with parentElement = null.

Thanks! I changed it from while ( wrapper.nodeName !== 'P' && ( wrapper = wrapper.parentElement ) ) {} and forgot to add the check. 🙈 Nice catch.

ellatrix · 2018-02-01T17:11:08Z

Rebased after Editable rename.

aduth · 2018-02-02T13:50:45Z

lib/client-assets.php

@@ -909,6 +909,10 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
 		'colors'             => $color_palette,
 		'blockTypes'         => $allowed_block_types,
 		'titlePlaceholder'   => apply_filters( 'enter_title_here', __( 'Add title', 'gutenberg' ), $post ),
+		'alignWide'        => $align_wide || ! empty( $gutenberg_theme_support[0]['wide-images'] ), // Backcompat. Use `align-wide` outside of `gutenberg` array.


PHPCS warnings from misaligned arrows here (causing the build failure)

Actually, I think these are duplicated and need to be removed. Maybe from a bad rebase?

aduth · 2018-02-02T13:53:18Z

editor/components/block-settings-menu/unknown-converter.js

+		onReplace( block.uid, rawHandler( {
+			HTML: serialize( block ),
+			mode: 'BLOCKS',
+			allowIframes: get( user, 'data.capabilities.unfiltered_html', false ),


Minor: String argument to get requires additional parsing and is slightly less performant than passing as an array, e.g.:

allowIframes: get( user, [ 'data', 'capabilities', 'unfiltered_html' ], false ),

Makes me wonder if we should have some equivalent of selectors for withAPIData too, rather than working with the REST properties directly, something like a currentUserCan

aduth · 2018-02-02T13:58:26Z

editor/components/block-list/block.js

@@ -122,6 +123,12 @@ export class BlockListBlock extends Component {
 		};
 	}

+	getChildContext() {
+		return {
+			unfilteredHTML: get( this.props.user, 'data.capabilities.unfiltered_html', false ),


The name unfilteredHTML isn't entirely clear to me: At first, I thought this was passing some HTML string, not a true / false permissions thing. Maybe allowUnfilteredHTML ? isUnfilteredHTMLAllowed ?

canUserUseUnfilteredHTML? It's a mouthful, but a recognizable phrasing.

aduth · 2018-02-02T14:04:00Z

blocks/api/raw-handling/test/inline-block-corrector.js

+import inlineBlockCorrector from '../inline-block-corrector';
+import { deepFilterHTML } from '../utils';
+
+describe( 'inlineContentConverter', () => {


Name doesn't match the folder.

aduth · 2018-02-02T14:04:50Z

blocks/api/raw-handling/test/inline-block-corrector.js

+import { deepFilterHTML } from '../utils';
+
+describe( 'inlineContentConverter', () => {
+	it( 'should move inline-block content from paragraph', () => {


It would be useful to have some of these behaviors documented (README.md and/or JSDoc on the functions themselves). By the name inlineBlockCorrector, I had no idea what to expect from its behavior.

I'm coming to this mostly unfamiliar with the raw handlers, so I expect it'd be a common sentiment.

aduth · 2018-02-02T14:09:50Z

blocks/api/raw-handling/utils.js

@@ -110,6 +115,10 @@ export function isClassWhitelisted( tag, name ) {
 	);
 }

+export function isInlineBlock( node ) {


Fun with JavaScript 😄

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/prototype

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty

mcsf · 2018-02-01T17:10:02Z

blocks/api/raw-handling/index.js

 		piece = deepFilterHTML( piece, [
 			listMerger,
 			imageCorrector,
 			// Add semantic formatting before attributes are stripped.
 			formattingTransformer,
 			stripAttributes,
 			commentRemover,
+			...iframeUnwrapper,


Since we're already dealing with an array (the one provided to deepFilterHTML), we can simplify this:

deepFilterHTML( piece, compact( [ … allowIframes && unwrapper, inlineBlockCorrector, … ] ) )

Alternatively, we could use .filter( Boolean ).

mcsf · 2018-02-01T17:10:37Z

blocks/api/raw-handling/inline-block-corrector.js

+ */
+const { ELEMENT_NODE } = window.Node;
+
+export default function( node ) {


This needs a comment and a high-level explanation of what and why.

mcsf · 2018-02-01T17:24:13Z

blocks/library/image/index.js

 					const hasImage = node.querySelector( 'img' );

-					return tag === 'img' || ( hasImage && ! hasText ) || ( hasImage && tag === 'figure' );
+					return tag === 'img' || ( hasImage && tag === 'figure' );


Do we know this to be an improvement? For instance, pasting <p>Behold my picture: <img src="..."> Isn't it amazing?</p> yields Image<…> + Paragraph<Behold…amazing?>.

I ask because it change seems tangential to the iframe support. Should we try this separately?

Yes, the same fix is needed for iframes as well as images. Otherwise the image just ends up inside a paragraph. Most of the time, this structure goes together with alignment, so it is not visually clear where the image was anchored.

mcsf · 2018-02-02T15:47:12Z

blocks/api/raw-handling/utils.js

@@ -31,6 +31,11 @@ const inlineWhitelist = {
 	br: {},
 };

+const inlineBlockWhiteList = {


I'm a bit concerned that all these lists and moving parts in raw-handling grow hard to keep track of, e.g. "inline" vs "inline wrapper" vs. "inline block". Docs in this file would help.

mcsf · 2018-02-02T15:53:24Z

editor/components/block-list/block.js

@@ -122,6 +123,12 @@ export class BlockListBlock extends Component {
 		};
 	}

+	getChildContext() {
+		return {
+			unfilteredHTML: get( this.props.user, 'data.capabilities.unfiltered_html', false ),


canUserUseUnfilteredHTML? It's a mouthful, but a recognizable phrasing.

ellatrix · 2018-02-05T13:48:59Z

I think I addressed all concerns.

ellatrix · 2018-02-07T12:11:47Z

Rebased.

ellatrix · 2018-02-07T12:34:06Z

This PR also indirectly fixes a bug where multiple images in a paragraph are all removed except for the first.

aduth · 2018-02-08T00:16:46Z

blocks/api/raw-handling/test/inline-block-corrector.js

+/**
+ * External dependencies
+ */
+import { equal } from 'assert';


Curious why we're using the built-in assert lib here rather than Jest?

It's been used all over this paste module since the beginning. I can change this.

Not a fan of the globals :|

aduth · 2018-02-08T00:18:10Z

blocks/api/raw-handling/utils.js

@@ -63,7 +68,7 @@ const whitelist = {
 };

 export function isWhitelisted( element ) {
-	return !! whitelist[ element.nodeName.toLowerCase() ];
+	return !! whitelist.hasOwnProperty( element.nodeName.toLowerCase() );


The type coercion is unnecessary (hasOwnProperty will certainly return a boolean).

aduth · 2018-02-08T00:18:13Z

blocks/api/raw-handling/utils.js

@@ -99,7 +104,7 @@ function isInlineForTag( nodeName, tagName ) {

 export function isInline( node, tagName ) {
 	const nodeName = node.nodeName.toLowerCase();
-	return !! inlineWhitelist[ nodeName ] || isInlineForTag( nodeName, tagName );
+	return !! inlineWhitelist.hasOwnProperty( nodeName ) || isInlineForTag( nodeName, tagName );


The type coercion is unnecessary (hasOwnProperty will certainly return a boolean).

aduth · 2018-02-08T00:18:17Z

blocks/api/raw-handling/utils.js

+ * @return {boolean} True if embedded content, false if not.
+ */
+export function isEmbedded( node ) {
+	return !! embeddedWhiteList.hasOwnProperty( node.nodeName.toLowerCase() );


The type coercion is unnecessary (hasOwnProperty will certainly return a boolean).

aduth · 2018-02-08T00:18:20Z

blocks/api/raw-handling/utils.js

 export function isInlineWrapper( node ) {
-	return !! inlineWrapperWhiteList[ node.nodeName.toLowerCase() ];
+	return !! inlineWrapperWhiteList.hasOwnProperty( node.nodeName.toLowerCase() );


The type coercion is unnecessary (hasOwnProperty will certainly return a boolean).

aduth · 2018-02-08T00:25:56Z

blocks/rich-text/index.js

@@ -371,6 +371,7 @@ export default class RichText extends Component {
 			plainText: this.pastedPlainText,
 			mode,
 			tagName: this.props.tagName,
+			allowIframes: this.context.canUserUseUnfilteredHTML,


Are we ever expecting allowScript, allowInput, allowStyle?

Maybe we just pass the canUserUseUnfilteredHTML verbatim as the property name?

ellatrix · 2018-02-08T12:34:46Z

Thanks @aduth, feedback addressed. :)

ellatrix · 2018-02-08T13:13:15Z

After reviews from 3 people in the last two weeks, I think this is ready for merge.

ellatrix added the [Feature] Paste label Jan 24, 2018

jaswrks reviewed Jan 24, 2018

View reviewed changes

ellatrix mentioned this pull request Jan 25, 2018

Add google maps as an embed #3970

Closed

ellatrix changed the title ~~Allow pasting iframes (e.g. Google Maps)~~ Allow pasting/convert from iframes (e.g. Google Maps) Jan 25, 2018

ellatrix changed the title ~~Allow pasting/convert from iframes (e.g. Google Maps)~~ Add iframe raw handling (e.g. Google Maps) Jan 25, 2018

ellatrix requested a review from mtias January 26, 2018 12:39

ellatrix mentioned this pull request Jan 30, 2018

Remove wrapper from image raw transform; Added test cases; #4740

Merged

ellatrix requested review from mcsf, youknowriad, aduth and jorgefilipecosta January 30, 2018 14:07

jaswrks reviewed Jan 30, 2018

View reviewed changes

ellatrix force-pushed the add/paste-iframe branch from e901e18 to 43a94ba Compare February 1, 2018 17:10

aduth reviewed Feb 2, 2018

View reviewed changes

mcsf reviewed Feb 2, 2018

View reviewed changes

ellatrix force-pushed the add/paste-iframe branch 2 times, most recently from 1c29b3e to 6d56c2d Compare February 5, 2018 13:47

ellatrix requested review from mcsf and aduth February 5, 2018 18:38

ellatrix force-pushed the add/paste-iframe branch from 6d56c2d to 2f30eeb Compare February 7, 2018 12:11

Allow pasting iframes (squashed 7 commits)

6a54e94

ellatrix force-pushed the add/paste-iframe branch from 2f30eeb to 6a54e94 Compare February 7, 2018 12:13

ellatrix requested a review from a team February 7, 2018 12:14

aduth reviewed Feb 8, 2018

View reviewed changes

Polish

fedb8d0

ellatrix force-pushed the add/paste-iframe branch from bbb2e78 to fedb8d0 Compare February 8, 2018 12:53

ellatrix merged commit 13b181b into master Feb 8, 2018

ellatrix deleted the add/paste-iframe branch February 8, 2018 13:13

aduth mentioned this pull request Jul 10, 2018

Block Edit: Remove legacy context API child context #7869

Merged

MadtownLems mentioned this pull request May 12, 2023

Add iframe raw handling to embed block (ie Google Maps) #50599

Open

Add iframe raw handling (e.g. Google Maps) #4660

Add iframe raw handling (e.g. Google Maps) #4660

Conversation

ellatrix commented Jan 24, 2018 • edited Loading

Description

How Has This Been Tested?

jaswrks Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaswrks commented Jan 24, 2018

ellatrix commented Jan 25, 2018

ellatrix commented Jan 30, 2018

jaswrks Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Feb 5, 2018

ellatrix commented Feb 7, 2018

ellatrix commented Feb 7, 2018

Choose a reason for hiding this comment

ellatrix Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Feb 8, 2018

ellatrix commented Feb 8, 2018

ellatrix commented Jan 24, 2018 •

edited

Loading

jaswrks Jan 24, 2018 •

edited

Loading

jaswrks Jan 30, 2018 •

edited

Loading

ellatrix Feb 8, 2018 •

edited

Loading