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

Provide parent block as context to render_callback of block #58822

Open
getdave opened this issue Feb 8, 2024 · 14 comments
Open

Provide parent block as context to render_callback of block #58822

getdave opened this issue Feb 8, 2024 · 14 comments
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Package] Blocks /packages/blocks [Type] Enhancement A suggestion for improvement.

Comments

@getdave
Copy link
Contributor

getdave commented Feb 8, 2024

Whilst working on the PR below, Fabian and I both independently realised that you cannot access information about the parent block when rendering the current block using the render_callback in PHP.

We should look to see if we can make this a default part of the $context for all dynamic blocks.

This would be useful in situations where extenders need to adjust the markup of a given block based on the parent block.

This has become more important since we enabled the allowedBlocks setting for blocks which effectively opens up all blocks to allow all other blocks.


@scruffian I looked a bit more into what it would take to accomplish this for a block without the filter.

I was wrong when I said it would be super trivial to find out whether you are nested within a navigation block or not on the server.

This here is the list of contexts that the navigation block provides to inner blocks in the editor:

"providesContext": {
"textColor": "textColor",
"customTextColor": "customTextColor",
"backgroundColor": "backgroundColor",
"customBackgroundColor": "customBackgroundColor",
"overlayTextColor": "overlayTextColor",
"customOverlayTextColor": "customOverlayTextColor",
"overlayBackgroundColor": "overlayBackgroundColor",
"customOverlayBackgroundColor": "customOverlayBackgroundColor",
"fontSize": "fontSize",
"customFontSize": "customFontSize",
"showSubmenuIcon": "showSubmenuIcon",
"openSubmenusOnClick": "openSubmenusOnClick",
"style": "style",
"maxNestingLevel": "maxNestingLevel"
},

But on the server the inner blocks get rendered here:

$inner_block_content = $inner_block->render();

Which doesn't pass any context. So it is not possible to determine whether a block (like the icon block in earlier examples) is rendered inside of the navigation block or not. Therefore dynamic blocks are pretty much out of luck.

And then finally if we don't go with the filter route we will also not have any control over the actual markup of the li element.

If in the future we wanted to add additional classnames to a li element or change the current markup (<li class="wp-block-navigation-item">) that won't be possible because the individual blocks would manually try to replicate that markup 🤔

Because of all of these factors I think the filter approach is the right one at this point. Even so close to a release.

But that is a strong opinion loosely held. So if you or anyone has other ideas how we could allow other blocks to accomplish the same thing without the filter I'm all for it :)

Originally posted by @fabiankaegy in #55551 (comment)

@getdave getdave added [Package] Blocks /packages/blocks [Type] Enhancement A suggestion for improvement. labels Feb 8, 2024
@fabiankaegy fabiankaegy added the [Feature] Extensibility The ability to extend blocks or the editing experience label Feb 8, 2024
@carolinan
Copy link
Contributor

I don't mean to derail but would this also solve passing for example a query context from a query loop through a pattern to a query pagination block?

@talldan
Copy link
Contributor

talldan commented Feb 9, 2024

But on the server the inner blocks get rendered here:
gutenberg/packages/block-library/src/navigation/index.php

Line 133 in d41d67c
$inner_block_content = $inner_block->render();

Which doesn't pass any context. So it is not possible to determine whether a block (like the icon block in earlier examples) is rendered inside of the navigation block or not. Therefore dynamic blocks are pretty much out of luck.

I don't think context is passed to the block render function, instead it's handled by the WP_Block_List and WP_Block classes. It might be a regression if it's not working correctly:
https://github.com/WordPress/wordpress-develop/blob/5c596f30d76169e86de9fddee357dd9ccfd236aa/src/wp-includes/class-wp-block-list.php#L26-L33

WP_Block_List has a magic method that constructs the WP_Block and ensures context is set:
https://github.com/WordPress/wordpress-develop/blob/5c596f30d76169e86de9fddee357dd9ccfd236aa/src/wp-includes/class-wp-block-list.php#L92-L102

@getdave
Copy link
Contributor Author

getdave commented Feb 9, 2024

Thanks @talldan. I also noticed $available_context but from what I saw of WP_Block it appears to be a protected property

https://github.com/WordPress/wordpress-develop/blob/5c596f30d76169e86de9fddee357dd9ccfd236aa/src/wp-includes/class-wp-block.php#L52-L59

Therefore it is not accessible on the $this instance that is passed to the render_callback:

https://github.com/WordPress/wordpress-develop/blob/5c596f30d76169e86de9fddee357dd9ccfd236aa/src/wp-includes/class-wp-block.php#L454

That said I did just notice this code which suggests that if I opt in to "parent" context maybe I will have access to it on $context. L

https://github.com/WordPress/wordpress-develop/blob/5c596f30d76169e86de9fddee357dd9ccfd236aa/src/wp-includes/class-wp-block.php#L134-L140

Going to try that now.

@talldan
Copy link
Contributor

talldan commented Feb 9, 2024

Thanks @talldan. I also noticed $available_context but from what I saw of WP_Block it appears to be a protected property

Ok, miscommunication here. A block always has two contexts its dealing with:

  • The first is the context it provides to children ($available_context). I was explaning that the navigation block doesn't pass this in via the render function, it instead is provided to inner blocks via the WP_Block_List.
  • The second is the context it consumes (accessed via the $block->context property, see render_block_core_post_title for an example).

The block also has to ensure it opts in to 'use' the context for it to be available (declared in the block.json).

@getdave
Copy link
Contributor Author

getdave commented Feb 9, 2024

  • The first is the context it provides to children ($available_context). I was explaning that the navigation block doesn't pass this in via the render function, it instead is provided to inner blocks via the WP_Block_List.

This was the key! Thanks Dan.

I realised that the Nav block rendering function manual instantiates several WP_Block_Lists to handle its inner blocks. In these it does not provide any context.

@mcsf
Copy link
Contributor

mcsf commented Feb 9, 2024

From a technical standpoint, this could be done for sure, but it's important to understand that it would change long-standing assumptions that we can make about blocks.

Keeping a certain isolation between blocks (siblings and children) was a deliberate choice early on. This is similar to what we've nicknamed the Vatican principle: an outer block cannot fully "see" its inner blocks, and likewise inner blocks have pretty limited knowledge of how/where they are contained. When we later implemented the context API, we made it so that both parties needed to be explicit about sharing a piece of context: the parent provides a certain property, and the child requests to use it.

(In general, not letting a parent fully see its inner blocks has several advantages: it means that there is a clear contract with the block editor that, for blocks, containment is just containment: parents by default don't get to manipulate or filter their children, and in terms of rendering we can be sure that rendering of children happens in exactly one place per parent as an "island" of sorts. This helps the block editor retain more control, which in turn means end users always have a consistent interaction model even in complex nesting scenarios.)

On the other hand, it's clear that there is a value in blocks that are contextual and thus flexible, and we see proof of that in many places, such as the Query—Post Content—Post Template relationships, Navigation, etc. The question is how permissive do we really want to be, because my instinct tells me we're better off with a context system that "undershares" by default.

So, my closing questions are:

  • Is there any way to achieve goals like "find out whether you are nested within a navigation block or not on the server" with the existing system by just adding more data to the context? If so, how reasonable is the solution?
  • If we choose to expand the context feature in the framework, what's the minimum surface expansion we can design?
  • Am I imagining dragons, and actually giving all blocks a "You Are Here" context by default is harmless, and I should shut up?

@getdave
Copy link
Contributor Author

getdave commented Feb 9, 2024

Thanks @mcsf. I have a working prototype of a MVP which I'm going to push up shortly.

Essentially I'm going to make parent a opt in property of context. So it's not automatically provided thereby retaining the contract. However, if you as a developer choose to opt in then you accept the downsides.

Is there any way to achieve goals like "find out whether you are nested within a navigation block or not on the server" with the existing system by just adding more data to the context? If so, how reasonable is the solution?

The Nav block already provides various pieces of context and certain blocks do consume these to work out whether they are within a Nav block.

This is achieved via providesContext and usesContext which parent's specifying which context is made "available" and children opting in to various context that are available.

However, it's flaky as it's based on the attributes of the Nav block. If one of these attributes changes then consumers of context that expect that attribute to be present to determine whether they are nested within a Nav block will see regressions. Now of course we could add a new stable context value of isWithinNavBlock to the block and then that would form a loose contract for consumers of that block who wish to opt into it. That's certain one fix and would require a small tweak to the Nav block code.

This is obviously not a standard API and would be Nav block specific - we're looking to avoid these "hotfixes" whereever possible to ensure the block is aligned to and contributes to wider standardised APIs within the editor.

If we choose to expand the context feature in the framework, what's the minimum surface expansion we can design?

As you suggested we'd need to be careful about introducing this coupling contract. I'm not aware of many use cases other than children of the Navigation block, but that doesn't mean there aren't any. Perhaps @ndiego or @ryanwelcher could provide some insight here as they do a lot of "extending" in their day to day work.

The minimum contract I see is this:

  • parent blocks "expose" themselves on context to inner blocks by default (note we could make this optional as well requiring parent blocks to opt in)
  • child blocks opt in to consume the parent in the context

This would use existing APIs and would require some small changes to WP_Block which I have a PR for locally.

I would also note that the idea of passing the parent block to the child is already outlined in various places such as render_block. However, it's always null suggesting it's an API that's been considered and planned for but never implemented.

My only concern is that some blocks seem to have a parent property available via context if they define a parent or ancestor in their block.json. I've been hunting for where this happens but as yet I can't locate the code path.

Am I imagining dragons, and actually giving all blocks a "You Are Here" context by default is harmless, and I should shut up?

I think it's wise we look for edge cases. We could try introducing the API in Gutenberg during the 6.6 cycle and see how it feels before proceeding.

@scruffian
Copy link
Contributor

Since we have landed the filter in #55551 do we need to worry about this?

@getdave
Copy link
Contributor Author

getdave commented Feb 9, 2024

Since we have landed the filter in #55551 do we need to worry about this?

TBH I was just curious as folks were suggesting this feature already existed when in fact it does not.

Open for discussion. But not a priority.

@fabiankaegy
Copy link
Member

I can think of quite a few use cases where I needed to know whether a block got rendered inside a query loop or not for example. But the only context I get there is the postId and postType. So this would be useful for that.

But again not a prio :)

@carolinan
Copy link
Contributor

There is a request to enable using the Home block outside the navigation, too. In that case we would need to know if it is in the navigation block or not, to change the wrapping element.

@mcsf
Copy link
Contributor

mcsf commented Feb 15, 2024

Thanks for the reply.

Now of course we could add a new stable context value of isWithinNavBlock to the block and then that would form a loose contract for consumers of that block who wish to opt into it.
[…] would be Nav block specific - we're looking to avoid these "hotfixes" wherever possible

I have to say: I don't see what you're describing as a hotfix, but as the system working as intended: the Nav block has complex needs and can take advantage of an API that's precisely meant for this sort of data sharing.

Essentially I'm going to make parent a opt in property of context. So it's not automatically provided thereby retaining the contract.

That said, I'm of course open to trying a prototype and debating. :)

As you suggested we'd need to be careful about introducing this coupling contract. I'm not aware of many use cases other than children of the Navigation block, but that doesn't mean there aren't any.

See, that's where I start to get suspicious. If the only use cases that we can think of are that of particularly complex and idiosyncratic blocks (Nav & Query), and for which there already is an explicit solution (providesContext: isWithinNavBlock | queryId), then do we have grounds?

My only concern is that some blocks seem to have a parent property available via context if they define a parent or ancestor in their block.json. I've been hunting for where this happens but as yet I can't locate the code path.

Hm, can you illustrate this? I'm not sure I understood ("have a parent property available via context").

@getdave
Copy link
Contributor Author

getdave commented Feb 18, 2024

Hm, can you illustrate this? I'm not sure I understood ("have a parent property available via context").

I later realised this was a different property. Basically it's so you can tell a block that it should only appear as a child of a given block.

See, that's where I start to get suspicious. If the only use cases that we can think of are that of particularly complex and idiosyncratic blocks (Nav & Query), and for which there already is an explicit solution (providesContext: isWithinNavBlock | queryId), then do we have grounds?

I agree with you and tend towards the cautious approach here. That is why I previously asked if there are indeed any additional concrete use cases - we have only a few so far.

We can let this sit for a while and if nothing comes up close it out as "not required" and illustrate the current methods available for achieving a similar result.

@mr-gorjan
Copy link

mr-gorjan commented Jun 21, 2024

<label for="textInput">Enter Text:</label>
<input type="text" id="textInput" name="textInput">
<p id="errorMessage" class="error">Input must be at least 3 characters long.</p>
<button id="submitButton" type="button">Submit</button>
<script>
    document.getElementById("submitButton").addEventListener("click", function(event) {
        var textInput = document.getElementById("textInput").value;
        var errorMessage = document.getElementById("errorMessage");

        if (textInput.length < 3) {
            errorMessage.style.display = "block";
        } else {
            errorMessage.style.display = "none";
        }
    });
</script>

How can I make something like this into Separate Blocks and still let them allow to access each other when they are inside Form Block? I thought of extending exisiting Blocks such as Paragraph which would act as Text Block but ran into dependency issues.

I was searching for a way to access the Parent Block and its Child Block, landed on this ticket. Have been trying to get my head around with the whole block editing and building blocks which would accept input values outside of editor. Thanks to @carolinan for suggesting on Slack, to look at Isolated Guternberg Editor.

I ended up choosing a simple path first and asked myself why not build few blocks which could be placed anywhere. Like a Text Block which can have an attribute of "editable", that when set true would display the Text Block as an Input Block for others.

The same problem occured to me when I needed to access the child values for a Submit Button Block. If anyone here can shed some light on a way I can achieve it then it would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Package] Blocks /packages/blocks [Type] Enhancement A suggestion for improvement.
Projects
Development

No branches or pull requests

7 participants