Post Comments Loop: Unable to put comment blocks inside rows or columns

jameskoster commented 2 years ago

If you place a Row or Columns block in the Comment Template, it's not possible to place blocks like the comment author avatar inside the Row or Column.

I thought this might be something to do with the component blocks needing to check that the Comments Template is the ancestor (so that they cannot be inserted in other random locations). But it's worth noting that these blocks can be placed inside a Group, so I'm not sure.

DAreRodz commented 2 years ago

Hey @jameskoster, you are right, it's because we are using this in all comment blocks:

https://github.com/WordPress/gutenberg/blob/eaa2e0a91a8ccac43fa2e39f60c37713bff165d7/packages/block-library/src/comment-content/block.json#L7

That limits core/comment-template to be the only possible parent for all comment blocks. As a workaround, I would add core/column, core/group, etc. to the parent property, but this doesn't scale well, as there could be more core blocks to add in the future, or custom blocks, or whatever.

Maybe we can just remove the parent property? 🤔

I know it's not ideal, but it is how blocks inside the Query Loop work ― although those blocks can also be placed outside the Query Loop (e.g., the Single Post template), so it actually makes sense in this case.

jameskoster commented 2 years ago

It's a shame because the result is that comment layouts like this are prohibited:

Could the parent property look at all ancestors?

DAreRodz commented 2 years ago

Could the parent property look at all ancestors?

Yup, it seems possible and not that hard to do. I did a quick tests and everything works as expected (although a bunch of unit tests fail because of mocks missing some properties).

I'll open a PR 👍

DAreRodz commented 2 years ago

After collecting some feedback in #39228, it seems that modifying how parent property works to look all ancestors would not be the best approach. 😅

I’m not aware of all the different APIs to restrict either block parents or children (@ntsekouras mentioned some of them); my impression is that there is not currently any API that allows to define the type of restriction needed here, am I right?

One way that comes to mind, to solve this problem without introducing any new API, would be to restrict blocks with usesContext to be placed inside other blocks only if one of the ancestors has the same property in providesContext (I suppose this was discussed already, right? 😄). This would have some caveats as well, e.g., a block like Generic Avatar could receive postId or commentId from context, but it doesn't require both to be defined at the same time.

So, if we want blocks to have a specific ancestor, we would need to introduce a new API, right? 🤷

In any case, for now, I think we have to choose one of the workarounds I mentioned before. Either just remove the parent property from all the comment blocks or add core/column, core/group, etc. to that property.

jameskoster commented 2 years ago

Thanks for looking in to it @DAreRodz.

I presume that if we remove the parent property then the comment blocks would start to appear in the Inserter regardless of the cursor position? That's probably not ideal.

Likewise if we add things like column and group as parent options then you'd start to see the comment blocks appear in the inserter when you're working in a column or group outside of the comment template? Also not ideal 😅

Honestly I'm not sure if it's worth introducing one of these workarounds – I'd love to hear more thoughts. Overall a new API feels like the proper solution.

DAreRodz commented 2 years ago

Hey @jameskoster, thank you for your insights!

I'm trying to figure out how the process for adding a new API would be. 🤔

I don't want to add new arbitrary properties into the block.json schema ― it would be very convenient TBH, not sure if that's something we can do. Anyway, as this feature would be required for the comment blocks to be released, I was looking for something temporary like passing __experimentalAllowedAncestorBlocks somewhere programmatically, kind of similar to the allowedBlocks prop passed to the useInnerBlocksProps hook.

It would be great to have some thoughts on this. @ntsekouras @gziolo, pinging you as you shared some ideas in https://github.com/WordPress/gutenberg/pull/39228 😊

jameskoster commented 2 years ago

as this feature would be required for the comment blocks to be released

I don't know if that's the case. This issue has very little traction, it may not be a high priority yet.

Agree it would be excellent to get insight from others.

DAreRodz commented 2 years ago

I don't know if that's the case. This issue has very little traction, it may not be a high priority yet.

Oh, I see. Thanks @jameskoster for the clarification. 👍

gziolo commented 2 years ago

Anyway, as this feature would be required for the comment blocks to be released, I was looking for something temporary like passing __experimentalAllowedAncestorBlocks somewhere programmatically, kind of similar to the allowedBlocks prop passed to the useInnerBlocksProps hook.

That would be a good first step 👍🏻

DAreRodz commented 2 years ago

Hi there 👋, it’s been a while since the last time I wrote. I wanted to share a possible API to quickly solve this issue (not meant to be the final one in any way). I’ll greatly appreciate any feedback or comment you may have, so thanks in advance for sharing them. 🙏

I’ve decided first a set of requisites for this API, which I list below. After that, I introduce some possible, slightly different options that can be implemented. The final code should be pretty similar to the experiment I did in #39228, but this time without changing the current parent's behavior and, therefore, not affecting other blocks.

Requisites

As it defines a relation between block types (e.g., core/comment-template → core/comment-content), it should be part of the block type definition.
It should be ideally declarative, as it’s just an immutable characteristic of the block type, not something that could change in response to any logic.
It should introduce fewer changes in the code as possible.
In this case, we want an experimental, not final API: just a temporary solution until we agree on a better and more powerful API for defining block-type relations.

Possible options

__experimentalParentCheckAncestors: boolean

This property would act as a flag intended to change the way parent (in block.json) is interpreted. When it’s defined and true, the function canInsertBlockTypeUnmemoized, which is the one that checks if a block of a certain type can be inserted in a given position, should iterate over all the block’s parents until the type of one of them is included in parent.
```
const parentName = getBlockName( state, rootClientId );
const parents = [ rootClientId ];

if ( __experimentalParentCheckAncestors ) {
    parents.concat( ...getBlockParents( state, rootClientId ) );
}

const hasBlockAllowedParent = some( parents, ( parentClientId ) =>
    checkAllowList(
        blockAllowedParentBlocks,
        getBlockName( state, parentClientId )
    )
);
```

__experimentalRequiredAncestor: string[]

This property would define a list of ancestors, requiring the block at least one of them to be present. Instead of affecting how parent is processed, this property would add a new block-type relation, so both restrictions could be defined at the same time. The block should be allowed to be inserted only if both conditions are met.

This would force changing how canInsert is computed, though, making the code a bit more complex, e.g.:

const requiredAncestor = __experimentalRequiredAncestor;
let doesBlockHaveRequiredAncestor = true;

if ( requiredAncestor ) {
    doesBlockHaveRequiredAncestor = some( requiredAncestor, ( parentClientId ) =>
        checkAllowList(
            blockAllowedParentBlocks,
            getBlockName( state, parentClientId )
        )
    );
}

const canInsert =
    ( hasParentAllowedBlock === null && hasBlockAllowedParent === null ) ||
    ( doesBlockHaveRequiredAncestor &&
            ( hasParentAllowedBlock === true ||
                hasBlockAllowedParent === true ) );

Where to define it

Block metadata (block.json)

If we go with this option, we have to bear in mind that block.json’s schema only allows adding new properties inside supports. It may not be the best place to add such a flag, as it seems to be used for adding visual-related features so far, like settings to change color, typography, spacing, etc.

E.g., for the Comment Content block:

{
    "$schema": "https://schemas.wp.org/trunk/block.json",
    "apiVersion": 2,
    "name": "core/comment-content",
    "title": "Comment Content",
    "category": "theme",
    "parent": [ "core/comment-template" ],
    "description": "Displays the contents of a comment.",
    "textdomain": "default",
    "usesContext": [ "commentId" ],
    "attributes": { ... },
    "supports": {

        "__experimentalParentCheckAncestors": true,

        "color": { ... },
        "typography": { ... },
        "spacing": { ... },
        "html": false
    }
}

In order to get the value of that property, as we already have the block type inside canInsertBlockTypeUnmemoized, we can either read the property directly from supports

const { __experimentalParentCheckAncestors } = block.supports;

or use the getBlockSupport function from @wordpress/blocks:

import { getBlockSupport } from '@wordpress/blocks';

const parentCheckAncestors = getBlockSupport(
    block,
    '__experimentalParentCheckAncestors'
);

Block settings (index.js)

The index.js file is the entry point for the block representation in the block editor. In this case, the property could be defined inside settings. It may look a bit weird at first, but there is already some blocks using an __experimentalLabel property there, so we have a precedent. 😃
```
export const settings = {
    icon,
    edit,
    __experimentalParentCheckAncestors: true,
};
```
Inside canInsertBlockTypeUnmemoized, the property would be extracted directly from the block object.
```
const { __experimentalParentCheckAncestors } = block;
```

Other options considered (and discarded)

I don't want to extend much more. Just mention that I’ve tried to look for other APIs, some of them programmatic (similar to allowedBlocks inside useInnerBlocksProps()), and in the end they had more drawbacks than advantages.

Well, this is it. As I said before, any feedback is more than welcome. 😊

luisherranz commented 2 years ago

I don't have much experience with these APIs, so I hope others chime in, but these are my two cents:

It doesn't make sense to put it in a different place than where "parent" is:
```
{
  "parent": "...",
  "ancestors": "..."
}
```
We should update the schema.
It should be independent of "parent", because both can be used at the same time (so your second option):
- I want this block to appear only if "xxx" is the parent block and "yyy" is an ancestor.
It doesn't make sense to include words like required or check if "parent" doesn't include them as well. So I'd call it "ancestors" and not "requiredAncestors".
It should be possible to define AND and OR relations:
- I want this block to appear only if "xxx AND yyy" blocks are ancestors.
- I want this block to appear only if "xxx OR yyy" blocks are ancestors.
It's unclear to me how such an API could be designed in a JSON format. Maybe with nested arrays:
- "xxx AND yyy": ["xxx", "yyy"]
- "xxx OR yyy": [["xxx"], ["yyy"]]
- "(xxx AND yyy) OR zzz": [["xxx", "yyy"], ["zzz"]]
Probably a bit confusing for the OR case, but the simple cases (single ancestor or AND) are straightforward. Maybe there are better ideas.

Other than that, I think it's a great proposal, and very well explained! 🙂

gziolo commented 2 years ago

@DAreRodz, thank you for sharing all your ideas. It looks like you explored a few different interesting ideas which make the decision process only simpler 😄

I echo most of what @luisherranz commented. I think we have enough historical data to make the new API stable from the beginning. We simply didn't have so pressing requirements to allow ancestors for core blocks before. For the initial implementation it might be enough just go with two top-level fields:

the existing parent
the new ancestor (to follow the singular form parent) or ancestors

It gives a lot of flexibility already and covers many interesting combinations based on how it works today for parent:

parent [ 'A', 'B' ] works as parent A OR parent B
parent [ 'A' ] and ancestor [ 'C' ] would work as parent A AND ancestor C operator
parent [ 'A', 'B' ] and ancestor [ 'C', 'D' ] would work as parent (A OR B) AND ancestor (C OR D)

luisherranz commented 2 years ago

@gziolo: how would you do ancestor A AND B?

gziolo commented 2 years ago

@gziolo: how would you do ancestor A AND B?

Yes, it would be an edge case we can’t express with the API proposed for block.json. Do you have an example where this type of support would be applicable? We can continually iterate and introduce an additional function alternative handled in JS registration that offers complete flexibility.

michalczaplinski commented 2 years ago

Nicely summarized @DAreRodz 👏 Some of my comments were already mentioned by others, but I'll repeat them here for show of support.

Option 2. (__experimentalRequiredAncestor: string[]) seems like a better choice.
- probably renaming it to simply ancestor or __experimentalAncestor if we really need to mark it as experimental for now.
Like Luis said, we should change the schema and add the property to the block.json
I just wanna give support to what David said:

In this case, we want an experimental, not final API: just a temporary solution until we agree on a better and more powerful API for defining block-type relations.

I feel the right thing to do would be to add the simple API now to solve the problem at hand and only when we have more use cases think about generalising the API to allow more complex relations.
Disclaimer: I don't think this is something that we should pursue, but just documenting it as an option that I considered.

I was thinking whether we should consider creating the API that is a bit more restrictive and only consider certain blocks as "pass-through" when using the ancestor property. The "pass-through" blocks might be column, columns and group for now.

So, imagine this block and inserting it in the editor:
```
// block.json
{ 
 "name": "core/comment-author-name",
 "ancestor": "comment-template",
 ...
}
```
- Comment Template
  - Columns
  - Column
    - Comment-Author-Name ✅
    - Some Other Block
    - Comment-Author-Name ❌
My reasoning was that a certain ancestor block might be added to the tree by a third party outside of the control of the block X author and end up causing the block X to appear in the inserter when it should not.

On the other hand, this would necessitate maintaining an allowlist of "pass-through" blocks which is probably not feasible either... Simple counterexample: What if someone uses a third-party or "column(s)" block?

DAreRodz commented 2 years ago

Thanks a lot for all your insights, I really appreciate them. 🙌

I'm already working on a PR to add the ancestor property to block.json.

luisherranz commented 2 years ago

My reasoning was that a certain ancestor block might be added to the tree by a third party outside of the control of the block X author and end up causing the block X to appear in the inserter when it should not.

@michalczaplinski: I don't understand the problem. Could you please elaborate?

Yes, it would be an edge case we can’t express with the API proposed for block.json. Do you have an example where this type of support would be applicable?

@gziolo: Imagine a totals block that can be inserted either on the cart or checkout blocks:

{
  "name": "totals",
  "ancestors": ["cart", "checkout"] // Cart OR Checkout
}

And another block that needs to be inside of the totals block, but only in the cart:

{
  "name": "some-cart-custom-total",
  "ancestors": ["totals", "cart"] // It doesn't work
}

I didn't know that parent arrays means OR, which means that changing ancestors array to mean AND (and subarrays to mean OR) is confusing.

Another option could be to use strings:

{
  "name": "totals",
  "ancestors": "cart OR cart" 
}

{
  "name": "some-cart-custom-total",
  "ancestors": "totals AND cart" 
}

And parenthesis if needed:

{
  "name": "some-cart-custom-total",
  "ancestors": "totals AND (cart OR mini-cart)"
}

EDIT: The string syntax could support NOT operations as well. And if it's standardized, it could be used among other properties where it makes sense. I've found a couple of npm packages that parse that syntax: 1, 2.

{
   // Inside the query-loop, but only outside of the comments-loop
  "ancestors": "query-loop AND NOT comments-loop"
}

I don't love the idea of the strings, though, and I don't have any other ideas right now, but we can keep thinking. I think it'd be a shame to introduce an ancestors property that only supports OR operations.

I also wonder if, at some point, we should also introduce types of blocks, like columns or query-loop instead of core/columns and core/query-loop, so people can create their own implementation of those blocks and still accept all the blocks that are designed to be used inside.

gziolo commented 2 years ago

@luisherranz thank you for sharing some good examples. It definitely shows that ancestors can quickly become very complex to express and need some more advanced API. The truth is that even in the most simplistic way (where you define only a list of ancestor blocks) it's still a huge improvement. I believe @DAreRodz can land the initial API while we discuss how to cover the use cases you shared.

Aljullu commented 2 years ago

I wanted to share another use-case that we will face in WooCommerce once we start using the Query Loop block to display products, and which will complicate things a bit further. :upside_down_face:

The challenge is that we not only need to change which blocks are available depending on the parent/ancestor, but also depending on its attributes. Let me explain: we would like to use the Query Loop block do display products. For that, we will be creating Product-specific blocks (ie: Product Price). Ideally, those blocks should only be available if the parent/ancestor block is Query Loop block and has postType: product as an attribute.

I know that's not exactly what's being discussed here, but I wanted to raise this use case because I think at some point it might be easier (at least, for plugin devs like us) to programmatically check the ancestors than to write these kinds of rules in JSON.

luisherranz commented 2 years ago

Interesting.

Would it make sense to leverage block variations for that? Or does it not fit the use case?

Similar to the way Group/Row/Stack work.

const variations = [
    {
        name: 'product-query-loop',
        title: __( 'Product Query Loop' ),
        attributes: { postType: 'product' },
        scope: [ 'transform' ],
        isActive: ( blockAttributes ) => blockAttributes.postType === 'product',
    }
]

{
  "name": "product-price",
  "ancestors": [ "product-query-loop" ]
}

Aljullu commented 2 years ago

Ah, good idea @luisherranz. I didn't invest a lot of time thinking on this yet, given that we didn't start the exploration phase yet. But a priori, I guess your suggestion to create a block variation could work for us. :+1:

WordPress / gutenberg