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.

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

Inner Blocks are lost if Parent Container Block is removed. #13391

Closed
diggeddy opened this issue Jan 20, 2019 · 13 comments · Fixed by #14443
Closed

Inner Blocks are lost if Parent Container Block is removed. #13391

diggeddy opened this issue Jan 20, 2019 · 13 comments · Fixed by #14443
Labels
[Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f [Type] Bug An existing feature does not function as intended

Comments

@diggeddy
Copy link

There are many plugins available that create a Container ( Section or Row ) block that allow Inner Blocks to be added. Examples: Atomic Blocks, CoBlocks, Ultimate Addons for Gutenberg etc. Deactivating any of the plugins tested results in the following warning within the Editor.

screenshot 2019-01-20 at 18 47 58

If the user chooses to Keep as HTML or switches to the Code Editor then all of the Inner Block content is lost. There is no ability to extract the Inner Blocks as a Common Block or as HTML.

Replicate this issue

NOTE: I have chosen the Atomic Block plugin for this example, but as mentioned it effects all of these types of plugin.

WordPress 5.0.3
Gutenberg 4.8
Atomic Blocks 1.4.23

  1. Install Atomic Blocks plugin from WordPress repository
  2. Add a AB Container Block.
  3. Populate the AB Container Block with a single Title Block.
  4. Switch to Code Editor and this is the resulting output:

screenshot 2019-01-20 at 19 19 29

  1. Deactivate Atomic Blocks Plugin.
  2. Check Page Preview to see that Inner Block Content remains intact.
  3. Within Editor switch to Code Editor and this is the resulting output:

screenshot 2019-01-20 at 19 19 40

This ultimately leads to 'Block Lock'. The only way the user can retain the content added to a Container Block is to extract them before the plugin is deleted. This could lead to loss of user content within the Editor.

Proposed solution: Option to extract all blocks as separate HTML or Common Blocks

@swissspidy swissspidy added Needs Testing Needs further testing to be confirmed. [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P Needs Technical Feedback Needs testing from a developer perspective. labels Jan 22, 2019
@rchipka
Copy link

rchipka commented Jan 30, 2019

I've encountered this before as well.

Replace container with InnerBlocks

It seems intuitive to me that if we can parse the HTML for the inner blocks they should be able to be retrieved automatically, so perhaps there could be an option to just "Remove containing block".

Transformable containers

Why not make all block types that accept InnerBlocks available as a transform option?

This would be a pretty cool feature to have since we could simply choose to transform a missing container to another container type.

It would also allow for quick experimentation between different container blocks.

For example, a user could quickly see what their InnerBlocks layout looks like inside of "Columns" vs inside of a "Slider", etc. without these blocks having to explicitly allow or define a transformation.

Static substitute container

Why not (upon identification of a missing block type that accepts InnerBlocks), transform the block into a static container that replicates and "freezes" the original wrapping layout and drops the InnerBlocks into the appropriate location.

If we could make this happen, then users would impacted to the minimum degree should a heavily used block be removed site-wide.

They could still edit the inner contents, but they would lose the original block settings controls since it's being emulated by a generic layout-emulating container.

And, most importantly, the user wouldn't get a big ugly warning where their only option forces them to lose data.

@tomusborne
Copy link

This is definitely a pretty major issue. If a user removes any third-party block which contains inner blocks, all of their content is lost.

@talldan
Copy link
Contributor

talldan commented Feb 8, 2019

Following up on the ping from @diggeddy in #13391

@talldan would you mind casting your eye over this issue - it would be good to get your input with the need for a Core Container block to provide a 'fall back' state for 3rd party container blocks.

I had a quick look into how missing blocks are handled. At the moment:

  1. The post content is parsed. During this phase, inner block markup is extracted from the outer block's markup and mapped into a block data structure. The parser's responsibility here is to map the post's markup into a block data structure, but it only gives each block enough knowledge to do a shallow render:

    const createParse = ( parseImplementation ) =>
    ( content ) => parseImplementation( content ).reduce( ( memo, blockNode ) => {
    const block = createBlockWithFallback( blockNode );
    if ( block ) {
    memo.push( block );
    }
    return memo;
    }, [] );

  2. Gutenberg tries to recreate the block, and for missing blocks uses a core/missing block that's registered as a fallback:
    https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/index.js#L113

  3. The 'missing' block displays the RawHTML, but this is the version with the inner block markup extracted by the parser (at step 1). The 'missing' block has access to the innerBlock data, but at this point there's no way of knowing how to restructure the inner blocks into something meaningful (where would it insert those inner blocks?):
    https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/missing/index.js#L88

It seems as though this would need to be fixed at the parsing stage (step 1), since that's where knowledge of the inner block markup is taken out of the block creation process.

@talldan
Copy link
Contributor

talldan commented Feb 8, 2019

@rchipka A couple of interesting ideas. I had some thoughts about what the challenges might be with an implementation of those ideas:

Why not make all block types that accept InnerBlocks available as a transform option?

The difficulty here is that a block with inner blocks can contain other blocks with inner blocks that could also be missing. This transform would have to be recursive, I'm not sure how that would be fashioned in a user interface given the user might have limited knowledge about the original markup of the missing block. Also worth noting that with each transform, data can be lost since rarely is a transform a perfect match. The more nesting === the more data lost.

Why not (upon identification of a missing block type that accepts InnerBlocks), transform the block into a static container that replicates and "freezes" the original wrapping layout and drops the InnerBlocks into the appropriate location.

A couple of things that are challenging about this. First, as mentioned in my comment above, currently there's no knowledge of where to drop the inner blocks. Second, each block has it own HTML that can wrap inner blocks. For example, in the columns block the columns are wrapped in divs. This markup would have to be kept as static html, but somehow still accept the dynamic inner blocks as children.

One other idea I had is that we could attempt a raw transform of the missing block—this could work in some cases, maybe mapping images that are inner blocks into a gallery block. It'd likely still not be a perfect transform.

@diggeddy
Copy link
Author

@talldan thanks for looking into this. Reading between the lines it seems like in its current state GB has a major issue with managing out this problem. Especially trying to cover every eventuality.

If a basic core container block existed, and was registered as a transform state in 3rd party container blocks, would this provide a manual fallback for converting blocks before the plugin was removed ?

@diggeddy
Copy link
Author

diggeddy commented Mar 4, 2019

So revisiting the issue, and the only solution i have found is the Classic Editor. From my ( not conclusive ) tests the markup remains intact if you Edit a page using the classic editor. This is whether the Container Block plugin is active or not. Would the simplest solution be to allow a block to be transformed to a Classic Editor Block if parent is missing?

@youknowriad youknowriad added the [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f label Mar 7, 2019
@youknowriad
Copy link
Contributor

This looks like a parsing issue, I wonder if @mcsf @dmsnell have thoughts about how we could keep the inner blocks HTML in the originalUndelimitedContent attribute of the missing block.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended and removed Needs Technical Feedback Needs testing from a developer perspective. Needs Testing Needs further testing to be confirmed. labels Mar 7, 2019
@mcsf
Copy link
Contributor

mcsf commented Mar 7, 2019

Thanks for bringing this up. There's room to improve this, yes. As a basic concept, this works:

-const originalUndelimitedContent = innerHTML;
+const originalUndelimitedContent = reserializeBlock( { innerBlocks, innerContent } );
function reserializeBlock( blockNode ) {
	const { innerBlocks, innerContent } = blockNode;
	let i = 0;
	return innerContent.map( ( item ) => item === null ?
		reserializeBlock( innerBlocks[ i++ ] ) : 
		item
	).join( '' );
}

@dmsnell
Copy link
Member

dmsnell commented Mar 9, 2019

Thanks all for raising this issue. I'd like to refer to a deeper (open) problem we all know needs addressing which is: what do we do when we don't understand a block in the document.

I don't think that this is a special case here but rather one of many cases where we don't have a strong fallback mechanism.

To this point I'd like to note that not all is lost:

This is definitely a pretty major issue. If a user removes any third-party block which contains inner blocks, all of their content is lost.

The content remains and this is why we want to provide a meaningful static fallback render and avoid things like this…

<!-- wp:latest-posts /-->

…because truly when the plugin disappears the content is gone. If, however, we do provide that render as static HTML we at least have a graceful fallback if not the fully-intended HTML structure of the block. We may not be able to edit it, but as noted, the preview still works.

With shortcodes we have a very similar problem when the plugin is removed, but blocks allow us to provide a sane fallback whereas shortcodes render what should remain an internal detail to the site's admin. On that notion I think the Gutenberg behavior is a step above what has been in WordPress traditionally.


How could we improve this further?

we can parse the HTML for the inner blocks they should be able to be retrieved automatically, so perhaps there could be an option to just "Remove containing block".

I kind of like the idea of having "Extract inner blocks" as an option for blocks with the parent, even if they are known and validated. That is, imagine we have a column block with five columns. If we extract the inner blocks we get a linear list of all the blocks at the top-level.

If we do this it won't resolve the problem that some of the inner blocks will be unknown - that's fine for the author to figure out. I really want to frame this issue in the context of using functionality whose required plugin is missing - this is already a kind of failure condition when we're starting, so anything we do will help even if not completely.

Though the produced markup may have additional surrounding <div>'s or other elements, the innerBlocks is nothing more than an array that we can remove, or an interspersed array. An extract function could swap all of that out.


Concerning Transformable Containers I don't have strong thoughts at the moment other than it appears to be solving a slightly different problem than this issue seems to be directly addressing. That is, we can build transformable containers and still have the problem noted in the original description.


@youknowriad I'm a bit confused by your comment. can you try and elaborate what you mean by it?

@mcsf how is your solution different than converting an unknown block to raw HTML?

@youknowriad
Copy link
Contributor

As I understand it we're not capable of transforming the block into an HTML block because when parsing the blocks, we lose the "full HTML" content of the missing block (the HTML of the inner blocks is removed).

So yeah, I was wondering how we could enhance the block parsing to provide the full HTML instead of the "remaining HTML after innerBlocks removal" in the originalUndelimitedContent attribute of the missing block.

@mcsf
Copy link
Contributor

mcsf commented Mar 11, 2019

Back when we originally discussed nesting support in the parser, we called this the Vatican principle. 😄 Let me explain:

Rome is a city, the capital of Italy. In Rome is enclaved the Vatican, an independent city-state. Though the reality is a lot more nuanced than the principle, the principle was that, when one is in the Vatican, one isn't in Italy. Therefore, the Italian state does not "see" what goes on in the Vatican.

The analogy, then, is that the parser is responsible for building a tree in which each block node is aware of all of its contents only up until the point where its "enclaved" (nested) blocks start.

To reply to:

how is your solution different than converting an unknown block to raw HTML?

Converting an unknown block to raw HTML will yield a block of HTML that excludes the nested content. So if you have a Columns block with two columns of content, and suddenly you no longer had the Columns block type registered, converting to HTML would yield:

<div class="wp-block-columns has-2-columns">

</div>

@mcsf
Copy link
Contributor

mcsf commented Mar 11, 2019

If we want to preserve the fundamental model for our parser (which includes the Vatican principle, and the fact that we operate in two stages: raw parsing to delineate blocks, and a second stage to ascribe meaning to blocks), then I believe the place to tackle this is around createBlockWithFallback, where we already have exceptions for a number of things, including if ( ! blockType ).

That means that we're already working with the output of the first stage of parsing, and so we have to work with block nodes as input in order to produce HTML trees for e.g. originalUndelimitedContent. Hence that very quick prototype in #13391 (comment).

@mcsf
Copy link
Contributor

mcsf commented Mar 14, 2019

Fix in #14443

dmsnell added a commit that referenced this issue Apr 19, 2019
Resolves #13391
Alternate approach to #14443

When the editor encounters a block without a registered block type
it transforms that unknown block into a generic "missing block."

A post's author is free to convert that unknown block into a kind
of static view from the original saved HTML of the block.

Unfortunately this model breaks down when the original block
includes nested blocks. Up until this point the editor has failed
to account for those inner blocks.

---

In this patch we're adding a new option to the missing plugin to
allow "extracting" the inner contents. That is, instead of simply
converting to static HTML the extract option splits the inner
contents into a new list of blocks as if simply removing the
outer wrapper.

This approach doesn't completely solve the editor's problem because
parts of the extracted contents may be wrong, but it provides a
reasonable fallback behavior to recover what is possible in the
situation where the editor can't understand a block.

---

 - extracts inner blocks and content when a block is unsupported
 - creates `core/html` blocks out of non-block content inside
   unsupported blocks

 - solve the issue of missing inner block HTML when converting
   to static HTML. this should be resolved in another patch.
 - improve the messaging for unsupported blocks during an edit session
 - fully resolve issues surrounding unsupported blocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants