Render generic fallback block for unknown block type #335

aduth · 2017-03-27T22:16:13Z

This pull request seeks to start handling of blocks which cannot be associated with a known registered block type. The current implementation is very basic and unfinished; merely setting HTML of a div (dangerously) and displaying an overlay with messaging atop the rendered content.

Caveats:

Again, this is temporary / WIP. I highly doubt we'd want "Unknown Block" messaging shown atop the block, but at least this gives a better sense that those blocks were detected by the parser.

Open questions:

How do we expect these sorts of blocks to behave from a UX perspective? (cc @jasmussen)
Is a "generic" block a good approach as the fallback?
Does it make sense to expose isVisible as an option to blocks? In this case it was something of an internal flag to prevent the generic block being made available as an option in the inserter, but I could potentially see this being useful for other cases (discontinuing blocks) and aligns reasonably well to isVisible control behavior.

Implementation notes:

Moved parser logic back into base blocks/index.js . Primarily to enable attribute parsing to wait until block has been determined. The fallback logic occurs in the Editor class, not in the parsing, since the block parser should not be aware of this generic block type. (cc @youknowriad for thoughts)
Now treating settings.edit as a component, which has a few nice advantages around avoiding wrapper elements and enabling the plugin author to define edit and save as Component classes (e.g. if needing to trigger network request upon mount)

jasmussen · 2017-03-28T07:53:37Z

Nice work, really good discussion to be having.

How do we expect these sorts of blocks to behave from a UX perspective? (cc @jasmussen)

It also ties into the fact that every post published by WordPress so far, will contain un-hinted markup that we somehow have to parse as blocks if you go and edit an old post. Whatever we do there should inform what we do here.

Given that, it seems like much of what exists today should ideally not be treated as an "unknown block". I don't know what prompts the "unknown" message from appearing in this PR, but it probably shouldn't be basic headings, paragraphs, images, working embeds, even shortcoded galleries. Ideally we'd be able to find a way to parse all those when reading the old post the first time, and upgrade each old block as you interact with them.

Way back in the kickoff days, this discussion came up briefly, which prompted the following mockup:

The middle block here is "unparseable", which was in the same discussion also referred to as an "I know what I'm doing block". This one definitely deserves special UI.

However we're likely to have many many edgecases where a block that should be easy to identify might have custom stuff or even broken or garbled syntax, that renders right by the browser but is still technically borked. This is also related to #214. Would a custom CSS class on an image, say, pull-right, cause our parser to not understand it? Or would we be able to "upgrade" it so you can add a caption and get the delicious HTML5 markup with figcaptions and whatnot?

Was this helpful?

youknowriad

the block parser should not be aware of this generic block type.

Good point, I think we should rename the html block that results from the grammar to unknown block which allows us to keep the output of the grammar consistent even when we have unknown blocks (without comments).

Moved parser logic back into base blocks/index.js . Primarily to enable attribute parsing to wait until block has been determined.

I don't know, I don't feel this is a good move, I think having something like that is good:

const parse = ( postContent ) => {
  grammarParse( postContent ).parse()
    // Converter could happen here
    .map(fallbackUnknownBlocks)
    .map(getBlockSettings)
    .map(getBlockStateWithAttributes) 
}

We could compose those map's of course ;)
In the above, the fallback still happens before the attributes parsing, but I think this logic is good separated in a parser module instead of the editor rendering.

youknowriad · 2017-03-28T09:14:46Z

blocks/README.md

@@ -129,6 +129,7 @@ Registers a new block provided a unique slug and an object defining its behavior
 - `save( attributes: Object ): WPElement` - Returns an element describing the markup of a block to be saved in the published content. This function is called before save and when switching to an editor's HTML view.
 - `controls: string[]` - Slugs for controls to be made available to block. See also: [`wp.blocks.registerControl`](#wpblocksregistercontrol-slug-string-settings-object-)
 - `encodeAttributes( attributes: Object ): Object` - Called when save markup is generated, this function allows you to control which attributes are to be encoded in the block comment metadata. By default, all attribute values not defined in the block's `attributes` property are serialized to the comment metadata. If defined, this function should return the subset of attributes to encode, or `null` to bypass default behavior.
+- `isVisible: boolean` - Whether the block is to be made available as an option in the block inserter overlay. Defaults to `true`.


I would prefer isDeprecated for deprecating blocks and I don't think we should make this block invisible in the inserter (so maybe the isVisible is not worth it for now). IMO the fallback block should be something close to the current editor which will serve as a backwards compatibility solution (think current editor inside as a block).

youknowriad · 2017-03-28T09:16:23Z

blocks/index.js

+	// Block attributes by hpq
+	if ( blockSettings.attributes ) {
+		return { ...attrs, ...query.parse( rawContent, blockSettings.attributes ) };
+	}


Now that I think about it, we probably need a fallback return attrs

jasmussen · 2017-03-28T09:39:54Z

Here's a draft mockup of an "unparseable block":

Can you think of a better code example of something that would be unparseable? Is there anything missing from this mockup? Any changes you'd like to see?

youknowriad · 2017-03-28T09:46:42Z

@jasmussen I may be wrong, but I think there are way more unparseable blocks especially since we're trying to keep the structure as flat as possible, nesting complex HTML tags could lead to other sort of unparseable blocks. I was thinking we leave the "current editor" (or tweak its UI a bit) for unparseable blocks, that way we have a good backwards compatibility strategy that handles all the use cases. And for people complaining about the new editor, they could still use this unparsable block for everything.

jasmussen · 2017-03-28T09:49:24Z

That sounds pretty good to me. Sounds like much larger sequences of markup can count as the contents of an "unparseable block", correct? Perhaps that you could even choose to insert an "unparseable block"? In case of the latter, we might want to rename it :) — an "I know what I'm doing block" has been suggested. Perhaps "Generic", as Andrew suggested in this very PR is a good term.

youknowriad · 2017-03-28T09:52:54Z

Sounds like much larger sequences of markup can count as the contents of an "unparseable block", correct?

Yep

Perhaps that you could even choose to insert an "unparseable block"? In case of the latter, we might want to rename it :)

I was think of "HTML block" :)

jasmussen · 2017-03-28T09:56:03Z

I was think of "HTML block" :)

That works too, though it would be rendered, right? I.e. you wouldn't see the markup itself? If yes, we should dig up the past discussions around labelling the two previous editor modes "Visual" and "Text", seems like we should sync up with that somehow.

jasmussen · 2017-03-28T09:56:53Z

Also CC: @mtias

joyously · 2017-03-28T15:22:05Z

I don't think a <script> block is unparseable, but it is unrenderable. Some HTML with mismatched tags or other syntax errors is unparseable. A [shortcode] is not exactly unparseable, but not HTML. HTML entities used in text should still be parseable. Plain text with no markup is still parseable.

folletto · 2017-03-29T10:49:30Z

Here's a potential approach that might help us get closer to a solution to this issue:

We should never have exceptions to the concept of "everything is a block". Invalid blocks are blocks, they just "fall back" to a specific example.
This would allow extensions to later provide alternatives, and even better parsing that could solve issues (this works on the assumption that the block engine should tokenize, but it's up to the block module to parse and display).
This would also allow the user to "switch" an invalid block to another to fix it ("This was a checklist that got broken, let's switch to checklist...").
We have a HTML block that provides "render HTML" and "show HTML code".
Valid blocks that are invisibles like script get assigned "render HTML".
Proper invalid blocks get assigned the same block, but "show HTML code": the evidence of the code showing up on the front-end too is a way to communicate that something is broken... but still render it.
Note that the above isn't a "code" block: it's not made to SHOW code (i.e. no highlighting, no line number, nothing). It's meant to render it in two different ways.

As an aside: the sooner we get to the point people can "code blocks" as modules separate from the actual block engine, the better, as we can quickly prototype any of the above.

anna-harrison · 2017-03-29T11:24:32Z

Hi! We (Ephox) have been thinking about this issue and experimenting with some ideas...

Imagine that in addition to all the block types that we are building ( #16 ) we create a TinyMCE Block Type. So, when you click on the inserter ( #323 ), TinyMCE Block Type would pop up as one of the choices.

The idea behind this block type would be to leverage TinyMCE to:

satisfy the edge cases i.e. the people who want to type up complex documents with tables, lists, and fancy formatting, spellchecking, MS Word cut and paste, etc
provide backwards compatibility
provide accessibility and
leverage TinyMCE mobile for editing on tablets and mobile phones

The UI on the TinyMCE Block Type would look just like the rest of the WP blocks, so it would appear seamless in the context of the whole page.

A really rough sketch: blue are all the WP block types; red is the TinyMCE Block Type

( cc @intronic @Afraithe )

aduth · 2017-03-30T15:00:25Z

I've rebased and pushed a new approach. Instead of registering a generic block, the logic changes now boil down to:

The parser will return all detected nodes, even if not a block (raw HTML) or not a registered block (deactivated plugin)
The Visual editor attempts to use the most applicable render logic, falling back to simply rendering (dangerously) the raw HTML of the block node

The latter case is like the idea of the "unparseable block" in @jasmussen's earlier comment.

I think eventually we'll want to move out the fallback rendering behavior. It's not clear yet whether this is just containing it in a separate component or perhaps integrating this into some transformations API. Just like we'll need to support blocks initializing from other blocks, we might be able to extend this so that a generic block could express wildcard ability to render any block which has no other applicable handlers. This is similar to a middleware architecture like how an Express application handles 404 pages as simply the last available handler for a route. Obviously ordering (or priority) would be a requirement to supporting this.

Proper invalid blocks get assigned the same block, but "show HTML code": the evidence of the code showing up on the front-end too is a way to communicate that something is broken... but still render it.

@folletto Is the thinking here that if a block's render behavior isn't known, to display the raw HTML (the escaped text, not rendered HTML) of the parsed node to clearly demonstrate that something's broken?

provide backwards compatibility

@annaephox Are you proposing that the TinyMCE block would be the fallback handler for unknown blocks? This could potentially align with my idea above of "a generic block expressing wildcard ability to render any block" (except in this case that generic block is the TinyMCE block).

youknowriad

I think this is way better. Good job here. I prefer the blockType fallback (see bellow), but It could be done later too.

youknowriad · 2017-03-30T15:10:56Z

blocks/parser.js

@@ -37,16 +39,14 @@ export function getBlockAttributes( blockNode, blockSettings ) {
 * @return {Array}          Block list
 */
 export default function parse( content ) {
-	return grammarParse( content ).reduce( ( memo, blockNode ) => {
+	return grammarParse( content ).map( ( blockNode ) => {
 		const settings = getBlockSettings( blockNode.blockType );


What do you think if we fallback here to a blockType = "html" (or tiny, generic or whatever, just pick one) whenever we get falsy settings, we could call, getBlockSettings again with the default block after that.

This would allow us to leverage the TinyMCE block everyone is talking about (me included) as a fallback block.

How does the parser know that the generic block even exists? Would this be a pre-registered block maybe?

For me it's ok to have a "special" block defined in all use cases (auto-registered) in the blocks module

The grammar already returns and html blockType if no comment is found

The grammar already returns and html blockType if no comment is found

Yeah, I wasn't sure how I felt about that, vs. simply omitting the property if it isn't a block.

For me it's ok to have a "special" block defined in all use cases (auto-registered) in the blocks module

This could work. It will likely add a dependency to wp.element from blocks and blocks would now include accompanying CSS.

A subtle side-effect to the logic in the visual editor render is that even if a block is registered, it's also gracefully handled if it defines neither edit nor save. I don't think we'd care to support this, so it probably shouldn't need to be graceful.

Yeah, I'm thinking more an more that the distinction between blocks and editor is superficial. We could merge everything IMO. We can keep two different global variables, but the module separation is not really important.

youknowriad · 2017-03-30T15:12:20Z

editor/editor/mode/visual.js

-					<div key={ index }>
-						{ wp.blocks.getBlockSettings( block.blockType ).edit( block.attributes, onChangeBlock( index ) ) }
-					</div>
-				) }


If we fallback to a different blockType as suggested above, all the changes here are not needed anymore.

If we fallback to a different blockType as suggested above, all the changes here are not needed anymore.

I kept some changes mostly for consideration of:

What if a block decides it doesn't need to be editable, and only assigns a save implementation? Should we support this? With updated logic it will simply show the front-end UI within the editor.

We currently allow a block to register itself with neither edit nor save implementations, which will throw if left unchecked

and only assigns a save implementation?

Yep, could be good for separators and stuff like that.

We currently allow a block to register itself with neither edit nor save implementations, which will throw if left unchecked

I think we should add some validation to the registerBlock (category, save, title, icon are all mandatory to me) (but granted this is a separate task)

androb · 2017-03-30T17:44:34Z

@aduth see #349 for more discussion about falling back to TinyMCE for unparseable content. It is very tolerant of bad HTML and would help people progressively "upgrade" their content to add blocks or convert existing content to newer, smarter blocks (e.g. through the Type Indicator to switch a blockquote to a New Quote).

Afraithe · 2017-03-30T18:11:12Z

Might not want to fall back to tinymce on everything though, like script tags or such things?

androb · 2017-03-30T19:22:32Z

@Afraithe we could have a Script Tag Block... in that way it is not unknown, it is known :)

anna-harrison · 2017-03-31T07:14:16Z

@aduth Hi and yes, I think I am proposing exactly what you wrote up there!

Are you proposing that the TinyMCE block would be the fallback handler for unknown blocks? This could potentially align with my idea above of "a generic block expressing wildcard ability to render any block" (except in this case that generic block is the TinyMCE block).

@mimo84 : can you take a look at the above and translate the hard bits for me, I think this aligns with what we are planning on doing next week :-)

aduth · 2017-03-31T14:41:19Z

Last few commits introduce a wp.blocks.setUnknownTypeHandler function and a new stub for a freeform block type to handle these unknown types.

youknowriad

I left some small notes but this is looking good! I really like the setUnknownTypeHandler 👍

youknowriad · 2017-03-31T14:47:01Z

editor/editor/mode/visual.js

+				const settings = wp.blocks.getBlockSettings( block.blockType );
+
+				let BlockEdit;
+				if ( settings ) {


I think we can assume this is always defined, right!

I think we can assume this is always defined, right!

I guess this depends if the editor implementation should trust itself to know that elsewhere it has assigned the unknown type handler. 😄 I might be erring too far on the side of safety here. Would certainly be more convenient to be able to assume that settings is always truthy.

aduth · 2017-03-31T15:12:44Z

I agree we should have more validation for registerBlock to check that we can have some usable render implementation. I'll create a task issue to track this (Edit: #362).

I'm inclined to keep the safety check on settings. Mainly because the return type of getBlockSettings is a nullable object, and I'd rather safety follow the expected type than assumptions about behavior.

Let's merge this and continue iterating in subsequent pull requests.

aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Mar 27, 2017

youknowriad reviewed Mar 28, 2017

View reviewed changes

This was referenced Mar 29, 2017

Add Block: List #346

Closed

Idea: Smart Text Block #349

Closed

This was referenced Mar 30, 2017

Serializer: Adding a block state serializer (blocks => postContent) #348

Merged

Find tests in nested directories #355

Merged

aduth added 4 commits March 30, 2017 10:24

Render blocks as components

716811f

Add missing test for fallback parsed on undefined attributes

67adf4d

Disable react/display-name ESLint rule

ba9635b

Include unknown blocks in parse return value

2b61305

aduth force-pushed the update/unknown-blocks branch from 68ce6e9 to 55c6ffc Compare March 30, 2017 14:39

Fall back to dangerous div rendering with undefined edit

a4e2994

aduth force-pushed the update/unknown-blocks branch from 55c6ffc to a4e2994 Compare March 30, 2017 14:43

youknowriad reviewed Mar 30, 2017

View reviewed changes

Allow assignment of unknown block type handler

9bf268c

aduth added 2 commits March 31, 2017 10:36

Register freeform block as unknown block handler

8ff84e9

Clarify possible undefined return value from unknown type getter

eaa5dfc

youknowriad approved these changes Mar 31, 2017

View reviewed changes

Test block type of parsed unknown blocks

15db036

aduth merged commit 92102dd into master Mar 31, 2017

aduth deleted the update/unknown-blocks branch March 31, 2017 15:18

This was referenced Mar 31, 2017

Plugin: Quote Block Initial Attempt #327

Closed

Update block API docs with edit/save argument as props #363

Merged

anna-harrison mentioned this pull request Apr 3, 2017

Make Core TinyMCE Block Type #365

Closed

aduth mentioned this pull request Apr 10, 2017

Suppress freeform block contenteditable warning #395

Merged

anna-harrison mentioned this pull request May 4, 2017

Freeform Block: Consider more descriptive name #645

Closed

Verdinjoshua26 mentioned this pull request Nov 27, 2023

[Snyk] Fix for 5 vulnerabilities Verdinjoshua26/gutenberg#93

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render generic fallback block for unknown block type #335

Render generic fallback block for unknown block type #335

aduth commented Mar 27, 2017

jasmussen commented Mar 28, 2017

youknowriad left a comment •

edited

Loading

youknowriad Mar 28, 2017

youknowriad Mar 28, 2017

jasmussen commented Mar 28, 2017

youknowriad commented Mar 28, 2017

jasmussen commented Mar 28, 2017

youknowriad commented Mar 28, 2017

jasmussen commented Mar 28, 2017

jasmussen commented Mar 28, 2017

joyously commented Mar 28, 2017

folletto commented Mar 29, 2017

anna-harrison commented Mar 29, 2017

aduth commented Mar 30, 2017

youknowriad left a comment

youknowriad Mar 30, 2017

aduth Mar 30, 2017

youknowriad Mar 30, 2017

youknowriad Mar 30, 2017

aduth Mar 30, 2017

youknowriad Mar 30, 2017 •

edited

Loading

youknowriad Mar 30, 2017

aduth Mar 31, 2017

youknowriad Mar 31, 2017

androb commented Mar 30, 2017

Afraithe commented Mar 30, 2017

androb commented Mar 30, 2017

anna-harrison commented Mar 31, 2017

aduth commented Mar 31, 2017

youknowriad left a comment

youknowriad Mar 31, 2017

aduth Mar 31, 2017

aduth commented Mar 31, 2017 •

edited

Loading

Render generic fallback block for unknown block type #335

Render generic fallback block for unknown block type #335

Conversation

aduth commented Mar 27, 2017

jasmussen commented Mar 28, 2017

youknowriad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Mar 28, 2017

youknowriad commented Mar 28, 2017

jasmussen commented Mar 28, 2017

youknowriad commented Mar 28, 2017

jasmussen commented Mar 28, 2017

jasmussen commented Mar 28, 2017

joyously commented Mar 28, 2017

folletto commented Mar 29, 2017

anna-harrison commented Mar 29, 2017

aduth commented Mar 30, 2017

youknowriad left a 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

youknowriad Mar 30, 2017 • 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

androb commented Mar 30, 2017

Afraithe commented Mar 30, 2017

androb commented Mar 30, 2017

anna-harrison commented Mar 31, 2017

aduth commented Mar 31, 2017

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Mar 31, 2017 • edited Loading

youknowriad left a comment •

edited

Loading

youknowriad Mar 30, 2017 •

edited

Loading

aduth commented Mar 31, 2017 •

edited

Loading