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

Block API: Support nesting (InnerBlocks) in unknown block types #14443

Merged
merged 4 commits into from Jul 9, 2019

Conversation

@mcsf
Copy link
Contributor

commented Mar 14, 2019

Fixes #13391.
Fixes #15202.

Description

When the block parser finds a block of unknown type, it wraps it in a special block (per getUnregisteredTypeHandlerName) — by default, this will be a block of type core/missing. This new outer block can then offer the user the possibility to extract the wrapped content.

This pull request updates the parsing logic so that the fallback block is fed the entire tree of content of the unknown block — if it has child blocks — and not just its own surface-level content.

Example

When parsing the following unknown block:

<!-- wp:my/unknown -->
  <p>Begin block that contains a list.</p>
  <!-- wp:list -->
  <ul><li>1</li><li>2</li></ul>
  <!-- /wp:list -->
  <p>End a block that contains a list.</p>
<!-- /wp:my/unknown -->

Before, the rescued originalContent would have been:

<!-- wp:my/unknown -->
  <p>Begin block that contains a list.</p>
  <p>End block that contains a list.</p>
<!-- /wp:my/unknown -->

Now, it would be the entire markup for my/unknown.

How has this been tested?

See parent issue for details. In addition, below are some concrete steps:

  1. Start a new post.
  2. Add a Columns block with two columns.
  3. Add some basic content to each column.
  4. Back in the source code, comment out the registration of the Columns block (see diff below).
  5. Refresh the editor.
  • The former Columns block should be singled out as an unknown block.
  • Its in-editor preview should still reveal the former content, though greyed out.
  • Actioning Keep as HTML should replace it with an HTML block reflecting the original content.
  • Not actioning Keep as HTML and previewing the whole post should reflect the original content.
  • Switching to the Code Editor should reveal the whole nested content.
diff --git a/packages/block-library/src/index.js b/packages/block-library/src/index.js
index 81a0e9bec..6a7c4569c 100644
--- a/packages/block-library/src/index.js
+++ b/packages/block-library/src/index.js
@@ -86,7 +86,7 @@ export const registerCoreBlocks = () => {
 		calendar,
 		categories,
 		code,
-		columns,
+		// columns,
 		column,
 		cover,
 		embed,

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@mcsf

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

/cc @diggeddy

@alvaro-ga

This comment has been minimized.

Copy link

commented Mar 15, 2019

Thanks for the effort to fix this. How does it work when there are nested unknown blocks with InnerBlocks (in the test, commenting out also column)?

@mcsf mcsf force-pushed the update/parser-reserialize-unknown-blocks branch from 35688c1 to 64a034e Mar 15, 2019

@mcsf

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Thanks for the effort to fix this. How does it work when there are nested unknown blocks with InnerBlocks (in the test, commenting out also column)?

@alvaro0555: It should work all the same (I re-tested by disabling both core/columns and core/column). This is because the parser stops* at the outermost unknown block and blindly reserialises its inner blocks to produce the core/missing block.

*: After which the parser continues parsing laterally.

@diggeddy

This comment has been minimized.

Copy link

commented Mar 15, 2019

@mcsf thank you, really appreciate the effort.

@mcsf mcsf force-pushed the update/parser-reserialize-unknown-blocks branch from 64a034e to 12607e1 Mar 15, 2019

@mcsf mcsf changed the title Block API: Support nesting in unknown block types Block API: Support nesting (InnerBlocks) in unknown block types Mar 15, 2019

@mcsf mcsf force-pushed the update/parser-reserialize-unknown-blocks branch from 12607e1 to 87c47c3 Mar 15, 2019

@dmsnell dmsnell self-assigned this Mar 17, 2019

@diggeddy

This comment has been minimized.

Copy link

commented Mar 25, 2019

@dmsnell any update on if/when this will be merged?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Something we need to take over as @mcsf will be AFK for a few weeks @WordPress/gutenberg-core

@dmsnell

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@diggeddy I away from the office for about two weeks, so I apologize for my delay. I will try and spend some time on this before the weekend. If not, please ping me again and if I've been unable to address it I'll likely approve it and merge to prevent it being a roadblock.

@diggeddy

This comment has been minimized.

Copy link

commented Apr 1, 2019

@dmsnell any news ? be good to see this put to bed.

@dmsnell

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Thanks for the extra ping @diggeddy.

I would like feedback on what I write here, as I'm a bit skeptical still of the reconstitute-approach for solving the problem.

I'm not sure what we are trying to gain by reserializing content inside the fallback block. **Do we not already have the full tree of innerBlocks and innerContent when we create the fallback block? Well, I think we need to pass along innerContent and innerBlocks (note that innerHTML should not be used anymore. we need to add some official deprecation warning) but as for reserialization I'm confused.

Are we wanting to extract content inside a container as one big blob of potentially-block-containing HTML? Why not provide an option to extract inner blocks as blocks instead?

So my thinking is that if we pass in innerContent and innerBlocks to the fallback then we can tell it to extract the insides and at that time insert new blocks. We shouldn't need to print out and then re-parse or even consider that as a separate kind of thing in these cases.

Not sure if @mcsf is back yet or not. If not, I can update this diff with my suggested changes, or provide another PR for it. I'd like to get a picture of how we want to handle these changes in the UI since this PR seems pretty limited to providing information that's currently missing. Is there already another PR to provide the author-facing functionality?

const blocks = [];
let blockIndex = 0;
for ( let chunk of innerContent ) {
  if ( null === chunk ) {
    blocks.push( innerBlocks[ blockIndex++ ] );
  } else {
    blocks.push( paragraphBlockFromHTML( chunk ) );
  }
}

With this approach we wouldn't need reconstituteBlockNode and we wouldn't have to make a distinction between delimited and undelimited.

As I don't see the updates to the fallback block which provide the extract functionality I'm thinking this may be more of an implementation question than anything else. As I see it though from the perspective of parsing producing a tree it feels more natural to me to extract blocks instead of html. It answers the recursive problem because if we have an inner block we also don't understand we can further extract it too if we want, but in a separate click.

@tomusborne

This comment has been minimized.

Copy link

commented Apr 2, 2019

From the perspective of a user, being able to extract the inner blocks makes more sense than being left with a bunch of HTML.

Maybe it should be an option though? Something like a "Convert to Blocks" button? That will allow them to choose whether to convert the content to blocks or re-activate the plugin with the container.

@diggeddy

This comment has been minimized.

Copy link

commented Apr 6, 2019

@dmsnell I agree with @tomusborne on this... any chance we may see this PR as a near future option?

@tomusborne

This comment has been minimized.

Copy link

commented Apr 6, 2019

The button I mentioned likely isn't even necessary if it complicates things, the user could just leave the page and not save if they decide to re-activate the plugin with the container block.

@diggeddy

This comment has been minimized.

Copy link

commented Apr 12, 2019

@dmsnell any news on this? Sorry for the directness.

dmsnell added a commit that referenced this pull request Apr 19, 2019

Missing Blocks: Allow extracting contents as blocks
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
@dmsnell

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

@diggeddy your patience is exemplary and I'm grateful for your pings - don't ever feel bad about asking me for updates (I'm generally working on several important things at once).

To that end I created #15078 to create a working model for the solution I proposed, which was to extract blocks.

What I'm finding harder to get behind with this PR's solution is creating new concepts and special cases in the parsed block object. I believe that we can accomplish what @mcsf proposed not only by not introducing new things into the system but by removing special cases.

Therefore I'm proposing #15078 as a tiny incremental improvement on the behavior which I hope meets your immediate needs while offering some addition time to fix the broader issue with unsupported blocks holding inner blocks. A parser change may help out but we may not need it or need as much as we think we do.

@Tug Tug referenced this pull request Jul 9, 2019
@Tug

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

I agree we should leave 728f2f3 out of this PR. Although it's a nice addition it might need more work. For instance we could check if all blocks can be inserted first and display a message or hide the button if it's not the case.

In the case of column for instance, we could convert those to group or extract its inner blocks as column cannot live without a parent columns block.

Reverting for now

mcsf added some commits Mar 14, 2019

Block API: Support nesting in unknown block types
When the block parser finds a block of unknown type, it wraps it in a
special block (per `getUnregisteredTypeHandlerName`) — by default, this
will be a block of type `core/missing`. This new outer block can then
offer the user the possibility to extract the wrapped content.

This commit updates the parsing logic so that the fallback block is fed
the entire tree of content of the unknown block — if it has child blocks
— and not just its own surface-level content.

For instance, when parsing the following unknown block:

```html
<!-- wp:my/unknown -->
  <p>Begin block that contains a list.</p>
  <!-- wp:list -->
  <ul><li>1</li><li>2</li></ul>
  <!-- /wp:list -->
  <p>End a block that contains a list.</p>
<!-- /wp:my/unknown -->
```

Before, the rescued `originalContent` would have been:

```html
<!-- wp:my/unknown -->
  <p>Begin block that contains a list.</p>
  <p>End block that contains a list.</p>
<!-- /wp:my/unknown -->
```

Now, it would be the entire markup for `my/unknown`.

@Tug Tug force-pushed the update/parser-reserialize-unknown-blocks branch from 2fc49fe to b5e40f0 Jul 9, 2019

@Tug Tug force-pushed the update/parser-reserialize-unknown-blocks branch from b5e40f0 to c65b3b5 Jul 9, 2019

@Tug

Tug approved these changes Jul 9, 2019

Copy link
Contributor

left a comment

This works for me and also fixes our issue on native wordpress-mobile/gutenberg-mobile#1206

I agree we should deprecate block.innerHTML as it seems redundant with innerContent.
Another solution would be to update it to return the real inner HTML just like with Element on the dom. But blocks being pure objects I don't think it would be a good idea to throw a getter here.
Anyway, this should be done in a follow up PR.

@Tug Tug referenced this pull request Jul 9, 2019
0 of 1 task complete
@mcsf
Copy link
Contributor Author

left a comment

Thanks for the help, @Tug!

</div>
<!-- /wp:column -->
</div>
<!-- /wp:columns -->`.replace( /\t/g, '' );

This comment has been minimized.

Copy link
@mcsf

mcsf Jul 9, 2019

Author Contributor

<3 thanks for cleaning up.

@mcsf mcsf merged commit a7cc76c into master Jul 9, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@mcsf mcsf deleted the update/parser-reserialize-unknown-blocks branch Jul 9, 2019

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

Block API: Support nesting (InnerBlocks) in unknown block types (Word…
…Press#14443)

* Block API: Support nesting in unknown block types

When the block parser finds a block of unknown type, it wraps it in a
special block (per `getUnregisteredTypeHandlerName`) — by default, this
will be a block of type `core/missing`. This new outer block can then
offer the user the possibility to extract the wrapped content.

This commit updates the parsing logic so that the fallback block is fed
the entire tree of content of the unknown block — if it has child blocks
— and not just its own surface-level content.

For instance, when parsing the following unknown block:

```html
<!-- wp:my/unknown -->
  <p>Begin block that contains a list.</p>
  <!-- wp:list -->
  <ul><li>1</li><li>2</li></ul>
  <!-- /wp:list -->
  <p>End a block that contains a list.</p>
<!-- /wp:my/unknown -->
```

Before, the rescued `originalContent` would have been:

```html
<!-- wp:my/unknown -->
  <p>Begin block that contains a list.</p>
  <p>End block that contains a list.</p>
<!-- /wp:my/unknown -->
```

Now, it would be the entire markup for `my/unknown`.

* Add group block in the test template to make sure it works recursively

Props Tug

@youknowriad youknowriad added this to the Gutenberg 6.2 milestone Jul 26, 2019

@@ -416,17 +417,39 @@ export function createBlockWithFallback( blockNode ) {
let blockType = getBlockType( name );

if ( ! blockType ) {
// Preserve undelimited content for use by the unregistered type handler.
const originalUndelimitedContent = innerHTML;
// Since the constituents of the block node are extracted at the start

This comment has been minimized.

Copy link
@mtias

mtias Jul 27, 2019

Contributor

Thanks for all the comments.

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Block API: Support nesting (InnerBlocks) in unknown block types (Word…
…Press#14443)

* Block API: Support nesting in unknown block types

When the block parser finds a block of unknown type, it wraps it in a
special block (per `getUnregisteredTypeHandlerName`) — by default, this
will be a block of type `core/missing`. This new outer block can then
offer the user the possibility to extract the wrapped content.

This commit updates the parsing logic so that the fallback block is fed
the entire tree of content of the unknown block — if it has child blocks
— and not just its own surface-level content.

For instance, when parsing the following unknown block:

```html
<!-- wp:my/unknown -->
  <p>Begin block that contains a list.</p>
  <!-- wp:list -->
  <ul><li>1</li><li>2</li></ul>
  <!-- /wp:list -->
  <p>End a block that contains a list.</p>
<!-- /wp:my/unknown -->
```

Before, the rescued `originalContent` would have been:

```html
<!-- wp:my/unknown -->
  <p>Begin block that contains a list.</p>
  <p>End block that contains a list.</p>
<!-- /wp:my/unknown -->
```

Now, it would be the entire markup for `my/unknown`.

* Add group block in the test template to make sure it works recursively

Props Tug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.