Element / Block API: Add RawHTML component, drop support for HTML string block save #4786

aduth · 2018-01-31T19:40:37Z

Related: #3745 (cherry-picks 57978eb)

This pull request seeks to migrate away from string return from block save implementation toward a new RawHTML component, which serves an identical purpose while also providing additional utility for nesting (#3745) and Editable value (#2750). Further, it reduces the number of code paths to maintain and improves consistency of hook behaviors (e.g. hooks injecting props via blocks.getSaveContent.extraProps are now respected). You can see this demonstrated here in the change introducing supports.className = false to the Classic block, as otherwise a wrapper with the auto-generated class name is applied (a bug resolved by these changes).

Breaking Change: (Deprecation) All existing block save implementations returning raw HTML will continue to work, but a warning will be logged to the browser console recommending the RawHTML component instead. We may want to seek to remove this deprecated code in the future.

Testing instructions:

Ensure unit tests pass:

npm run test-unit

Verify that there are no regressions in the behavior of blocks expecting to save raw HTML, notably the Classic, HTML, and Shortcode blocks. These blocks should also not render any wrapper elements, except in the case that a custom class name is assigned.

aduth · 2018-01-31T19:57:26Z

Some additional tasks I'd like to do here:

Add temporary deprecated backwards compatibility for existing usage
Update documentation to reflect changes

gziolo · 2018-02-01T10:33:45Z

blocks/hooks/deprecated.js

+	return <DangerousHTMLWithWarning children={ element } />;
+}
+
+addFilter( 'blocks.getSaveContent.saveElement', 'core/deprecated/save-props', shimDangerousHTML );


I like how you handled this backward compatibility issue with the hook ❤️

I like how you handled this backward compatibility issue with the hook ❤️

The backwards-compatibility error also had the side benefit of triggering some failing tests which both hinted toward (a) flawed tests and (b) that maybe we want to support some string returns to continue to work unaffected, but not those including raw HTML. This has been improved in the rebased 687af56.

gziolo · 2018-02-05T08:04:20Z

blocks/api/serializer.js

 	}

+	saveElement = applyFilters( 'blocks.getSaveContent.saveElement', saveElement, blockType, attributes );


Unrelated to this PR. In the future, we should also make sure that functional components are treated as React components, too:

saveElement = save( { attributes } );

We are about to introduce new hook blocks.getSaveContent.saveElement. Is it meant to exist only until the saveElement is being able to return a string? If the answer is yes, then we might want to move all the deprecation logic here instead of using a hook.

Unrelated to this PR. In the future, we should also make sure that functional components are treated as React components, too:

Related: #3745 (comment)

It is potentially related to this pull request, as we've simplified all paths which necessitated treating the function component any different than the component class. I think at one point in putting this branch together I'd tried to convert to createElement, but ran into challenges. I'll try to revisit and see if it can be done easily.

Yes, I tried the similar in the past but ended with splitting into two methods getSaveContent and getSaveElement only :(

We are about to introduce new hook blocks.getSaveContent.saveElement. Is it meant to exist only until the saveElement is being able to return a string? If the answer is yes, then we might want to move all the deprecation logic here instead of using a hook.

I don't have a problem with it sticking around. Related to #3800, it can serve as useful in cases like this one where we want to override the entire behavior of save, not just inject some additional props.

Of course, this opens questions of when and why we add filters. Also feel that we should be proactive about documenting them (expecting that, as with PHP, we'd want hook documentation pages.

gziolo · 2018-02-05T08:18:33Z

blocks/hooks/deprecated.js

+	return element;
+}
+
+addFilter( 'blocks.getSaveContent.saveElement', 'core/deprecated/save-props', shimDangerousHTML );


'core/deprecated/save-props' -> 'core/deprecated/save-element'?

'core/deprecated/save-props' -> 'core/deprecated/save-element'?

Aside: I still don't like that we name filters 😄 The number of times I've edited this line just to change the name serves as one reason.

Yes, this namespace thing (2nd param) seems obsolete at the moment.

gziolo · 2018-02-05T08:38:31Z

blocks/hooks/deprecated.js

+	// Still support string return from save, but in the same way any component
+	// could render a string, it should be escaped. Therefore, only shim usage
+	// which had included some HTML expected to be unescaped.
+	if ( typeof element === 'string' && includes( element, '<' ) ) {


includes( element, '<' ) - this behavior might lead to some errors. I will comment next to the code which might cause issues.

includes( element, '<' ) - this behavior might lead to some errors. I will comment next to the code which might cause issues.

Which comment expanded on the issue here?

#4786 (comment)

Talked with @gziolo about this some more. Basically: We want to support string returns from save, but in the same way that string is a valid React element, it should be unescaped. The shim here specifically targets HTML which previous block implementations may have been returning. The one case where this won't work well is if a block had returned a shortcode string, which will now be escaped. We could similarly add support for shortcode patterns, or alternatively just allow them to break (it's unclear to me how common this would have been).

gziolo · 2018-02-05T09:17:06Z

blocks/api/serializer.js

@@ -45,13 +45,10 @@ export function getSaveElement( blockType, attributes ) {
 		saveElement = createElement( save, { attributes } );
 	} else {
 		saveElement = save( { attributes } );
-
-		// Special-case function render implementation to allow raw HTML return
-		if ( 'string' === typeof saveElement ) {


As noted in here https://github.com/WordPress/gutenberg/pull/4786/files#r165907203, we are slightly changing behavior with this PR. When the saveElement is going to be represented as a plain string without any HTML tags, this function won't return the value immediately. It will be wrapped with Children.map, but addExtraContainerProps won't apply filters because it will return immediately after check if an object was passed. In the case where the saveElement would contain HTML tags it is going to be recognized as an object by the inner function, therefore all filters will be applied. This might introduce some confusion as the same block can have a different markup depending on the content.

Please also note that there is another function in this file:

export function getSaveContent( blockType, attributes ) {

which performs check against the return value from getSaveElement. It won't return string anymore after this refactoring is applied so we can remove the check in lines 78-81. Children.map makes sure that return value of the function is always object.

We might also want to explore in another PR the possibility of turning getSaveElement in SaveBlock component as suggested in #3800 by @andreiglingeanu.

In the case where the saveElement would contain HTML tags it is going to be recognized as an object by the inner function, therefore all filters will be applied.

If I understand correctly: This is different, yes, but arguably more consistent. Example of this being how the Freeform block could get away with avoiding supports.className: false previously only because it returned a string from save and so it was previously skipped in the generated class name hook. While in the case of freeform we don't want the generated class name, it seems to me wise to reduce the number of variations in how save results are handled.

gziolo

This looks good and is almost ready to merge. I have one concern where we need to clarify what is the expected behavior because there might be an unexpected change in the output HTML depending on the content of RichText component. I also left some comment where I would like to discuss further refactorings.

aduth · 2018-02-05T16:13:22Z

I've pushed a rebased branch which should address most all of the feedback. I've pushed for more refactoring of the getSaveElement function, specifically eliminating explicit support for Component class, which was flawed anyways (see fixed "Component save" serializer test case).

I have some stashed changes which pushed even to deprecate blocks.getSaveContent.extraProps altogether, instead recommending the newly-introduced blocks.getSaveElement which can accomplish the same via the filter handler's own cloneElement. However, since this requires the plugin author to accommodate all element shapes (including null), I was worried about this being error-prone with implementations trying to call e.g. element.props.foo (where element is null). For now, I've kept it around.

cc @youknowriad I know you'd mentioned on at least one occasion removing support for save components.

youknowriad · 2018-02-05T16:18:03Z

blocks/api/registration.js

 *
- * @type {Object}
+ * @typedef {WPBlockType}


We should just use flow or typescript

Yes, that would even more useful :)

youknowriad · 2018-02-05T16:19:10Z

blocks/api/serializer.js


+	// Component classes are unsupported for save since serialization must
+	// occur synchronously. For improved interoperability with higher-order
+	// components which often return component class, emulate basic support.


Good idea 👍

youknowriad · 2018-02-05T16:22:10Z

blocks/api/test/serializer.js

@@ -129,7 +117,7 @@ describe( 'block serializer', () => {
 					{ fruit: 'Bananas' }
 				);

-				expect( saved ).toBe( '<div>Bananas</div>' );


Do you know why the className was missing here?

Do you know why the className was missing here?

Yep, this was a tricky one. Alluded to in #4786 (comment) with the "fixed test case" mention, the issue was specifically in how createElement is treated differently from calling save as a function directly. It's the same reason why we couldn't just refactor to createElement:

...which is that, when cloning the element produced by createElement, it's the same as modifying props of what is passed into save, not what is returned by save. The filtering expects to be applied on whatever is produced by save, e.g. the div here.

We don't really have a way to hook into the render behavior without some awful machinery to render into a temporary DOM, so instead we drop support for save as a component and just treat it as a function (i.e. it's still not a "stateless function component", just a function returning an element(s)).

youknowriad · 2018-02-05T16:25:18Z

blocks/library/freeform/test/__snapshots__/index.js.snap

@@ -8,7 +8,7 @@ Array [
    style="display:none"
  />,
  <div
-    class="wp-block-freeform blocks-rich-text__tinymce"


Wondering why this change, you didn't touch the edit function right?

Wondering why this change, you didn't touch the edit function right?

Because I added the supports.className: false, it's no longer adding the generated class in BlockEdit:

gutenberg/blocks/block-edit/index.js

Lines 28 to 32 in 0cbe106

// Generate a class name for the block's editable form

const generatedClassName = hasBlockSupport( blockType, 'className', true ) ?

getBlockDefaultClassname( name ) :

null;

const className = classnames( generatedClassName, attributes.className );

...though this makes me think that this is a bit hard to follow, and that we might want to consolidate class name handling into blocks/hooks/generated-class-name.js, particularly since BlockEdit is already filterable anyways.

Oh right I was confused it with customClassName

gziolo · 2018-02-06T13:42:41Z

blocks/api/serializer.js

+		 * @param {Object}      attributes Block attributes.
+		 */
+		const props = applyFilters(
+			'blocks.getSaveContent.extraProps',


Should we rename to blocks.getSaveElement.extraProps to match with another filter? Can be another PR.

Should we rename to blocks.getSaveElement.extraProps to match with another filter? Can be another PR.

I think we should, yes, but it is a breaking change, one which we can deprecate with a hook, but probably best left for a separate pull request.

gziolo · 2018-02-06T13:45:38Z

element/index.js

+	let rendered = renderToStaticMarkup( element );
+
+	// Drop raw HTML wrappers (support dangerous inner HTML without wrapper)
+	rendered = rendered.replace( /<\/?wp-dangerous-html>/g, '' );


Nit: this function can return here instead of updating variable's value.

Nit: this function can return here instead of updating variable's value.

I'm going to leave this as-is, as it distinguishes / sets a pattern for transformations which would be applied to the default rendered output.

mtias · 2018-02-06T13:59:47Z

blocks/library/html/index.js

@@ -70,6 +71,6 @@ export const settings = {
 	] ),

 	save( { attributes } ) {
-		return attributes.content;
+		return <DangerousHTML>{ attributes.content }</DangerousHTML>;


I think this component should be called RawHTML, it doesn't make a lot of sense to call it dangerous in the context of an editor that deals with raw html in valid and regular cases.

Works for me.

Yeah, one potential downside is it's basically a substitute for dangerouslySetInnerHTML (sans wrapper), not just for serialization, so it could turn into a bit of a foot-gun.

gziolo

Code looks good, there are still some minor comments which might need some code updates, but everything looks good. I did a quite long testing session and I didn't discover any regressions when using updated blocks: Classic, Custom HTML, Shortcodes. 👍

Explicit support by element static string renderer.

Unused anyways, misleading to imply it is.

aduth · 2018-02-06T15:44:02Z

Renamed from DangerousHTML to RawHTML.

Also added deprecated compatibility support for shortcodes via a regular expression which is prone to false positives (better to capture and warn than break any plugin which had expected to return a shortcode text from its save).

aduth added [Feature] Block API API that allows to express the block paradigm. Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Breaking Change For PRs that introduce a change that will break existing functionality labels Jan 31, 2018

aduth added the [Status] In Progress Tracking issues with work in progress label Jan 31, 2018

gziolo reviewed Feb 1, 2018

View reviewed changes

aduth mentioned this pull request Feb 1, 2018

Framework: Assign preferred JSDoc tags and types #4785

Merged

aduth force-pushed the add/dangerous-html branch from b310d0e to ed02d56 Compare February 1, 2018 14:27

aduth removed the [Status] In Progress Tracking issues with work in progress label Feb 1, 2018

aduth force-pushed the add/dangerous-html branch 4 times, most recently from 79061cb to e80fb68 Compare February 2, 2018 17:25

gziolo reviewed Feb 5, 2018

View reviewed changes

gziolo requested changes Feb 5, 2018

View reviewed changes

aduth force-pushed the add/dangerous-html branch from e80fb68 to 402d4a3 Compare February 5, 2018 16:06

youknowriad reviewed Feb 5, 2018

View reviewed changes

gziolo reviewed Feb 6, 2018

View reviewed changes

mtias reviewed Feb 6, 2018

View reviewed changes

gziolo approved these changes Feb 6, 2018

View reviewed changes

aduth added 5 commits February 6, 2018 10:28

Element: Add support for RawHTML component

cc90d1e

Explicit support by element static string renderer.

Element: Render dangerous wrapper if props passed

55d8da3

Blocks: Update blocks to return RawHTML

f354daf

Block API: Drop support for raw HTML save return

12a98e9

Block API: Document WPBlockType type definition

caf1ca7

aduth added 3 commits February 6, 2018 10:33

Block API: Add hook for deprecated raw HTML support

a3cd23b

Serializer: Remove save return from more block test

c061592

Unused anyways, misleading to imply it is.

Serializer: Simplify serialize support to function

aca8f72

aduth force-pushed the add/dangerous-html branch from 402d4a3 to aca8f72 Compare February 6, 2018 15:39

aduth changed the title ~~Element / Block API: Add DangerousHTML component, drop support for HTML string block save~~ Element / Block API: Add RawHTML component, drop support for HTML string block save Feb 6, 2018

aduth merged commit 69830a1 into master Feb 6, 2018

aduth deleted the add/dangerous-html branch February 6, 2018 15:54

aduth mentioned this pull request Feb 6, 2018

Blocks: Support block nesting (Add Columns block, InnerBlocks component) #3745

Merged

aduth mentioned this pull request Feb 16, 2018

Extensibility: Introduce BlockSave which would work similar to BlockEdit #3800

Closed

iandunn mentioned this pull request Jan 1, 2019

Document proper use of RawHTML, escape-html, decodeEntities, etc #13156

Open

		}

		saveElement = applyFilters( 'blocks.getSaveContent.saveElement', saveElement, blockType, attributes );

	// Generate a class name for the block's editable form
	const generatedClassName = hasBlockSupport( blockType, 'className', true ) ?
	getBlockDefaultClassname( name ) :
	null;
	const className = classnames( generatedClassName, attributes.className );

Element / Block API: Add RawHTML component, drop support for HTML string block save #4786

Element / Block API: Add RawHTML component, drop support for HTML string block save #4786

Conversation

aduth commented Jan 31, 2018 • edited Loading

aduth commented Jan 31, 2018

Choose a reason for hiding this comment

aduth Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

aduth Feb 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

aduth commented Feb 5, 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

aduth Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

gziolo Feb 6, 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

gziolo left a comment

Choose a reason for hiding this comment

aduth commented Feb 6, 2018

aduth commented Jan 31, 2018 •

edited

Loading

aduth Feb 1, 2018 •

edited

Loading

gziolo Feb 5, 2018 •

edited

Loading

aduth Feb 5, 2018 •

edited

Loading

gziolo Feb 5, 2018 •

edited

Loading

aduth Feb 5, 2018 •

edited

Loading

youknowriad Feb 5, 2018 •

edited

Loading

gziolo Feb 6, 2018 •

edited

Loading