Automattic / vip-governance-plugin

WordPress plugin that adds additional governance capabilities to the block editor.
https://wpvip.com
GNU General Public License v3.0
17 stars 1 forks source link

Problems allowing core/list nested in a block #30

Closed alecgeatches closed 1 year ago

alecgeatches commented 1 year ago

While reviewing the changes in #24, I ran into an issue allowing a user to edit a core/list block within another block. The screenshots in this issue reference PR 24, but that's likely not relevant to the problem.

core/list at the root level

First, let's start with a basic set of locked-down rules that allow an editor to edit a handful of root blocks including core/list and core/list-item:

{
    "$schema": "./governance-schema.json",
    "version": "0.2.0",
    "rules": [
        {
            "type": "default",
            "allowedFeatures": [],
            "allowedBlocks": [ "core/paragraph" ]
        },
        {
            "type": "role",
            "roles": [ "editor" ],
            "allowedBlocks": [ "core/media-text", "core/heading", "core/list", "core/list-item" ],
            "blockSettings": {}
        }
    ]
}

This works fine! Lists can be inserted easily at the root level:

root-level-list

core/list in a core/media-text

Next, let's change the editor role rule. We'll move core/list so that it's the only allowed block in a core/media-text instead. The goal is that we allow core/media-text blocks at the root level, and inside core/media-text blocks we only allow lists.

"rules": [
    {
        "type": "default",
        "allowedFeatures": [],
        "allowedBlocks": [ "core/paragraph" ]
    },
    {
        "type": "role",
        "roles": [ "editor" ],
        "allowedBlocks": [ "core/media-text", "core/heading" ],
        "blockSettings": {
            "core/media-text": {
                "allowedBlocks": [ "core/list" ]
            }
        }
    }
]

This unfortunately doesn't work because the content portion of core/media-text is a core/paragraph, and that's not editable:

core-list-no-paragraph

It seems strange that the default rule core/paragraph doesn't apply here. It may be worth evaluating whether default blocks should be included automatically in allowedBlocks, since I'll need to re-add core/paragraph to any nested block settings. But for now, that's fine, I'll add the paragraph block it to core/media-text's allowedBlocks:

core/list in a core/media-text - Attempt 2

{
    "type": "role",
    "roles": [ "editor" ],
    "allowedBlocks": [ "core/media-text", "core/heading" ],
    "blockSettings": {
        "core/media-text": {
            "allowedBlocks": [ "core/list", "core/paragraph" ]
        }
    }
}

This fixes the content paragraph issue but unfortunately I can't interact with the list item:

core-list-item-blocked

This appears to be because core/list-item isn't allowed. This may be another potential area of improvement. Some blocks like core/list or core/columns are only usable with their child blocks enabled. Potentially we can hardcode some core blocks to support necessary child items to remove this responsibility from the rules file creator.

Anyway, let's add core/list-item support to core/media-text and try that:

core/list in a core/media-text - Attempt 3

{
    "type": "role",
    "roles": [ "editor" ],
    "allowedBlocks": [ "core/media-text", "core/heading" ],
    "blockSettings": {
        "core/media-text": {
            "allowedBlocks": [ "core/paragraph", "core/list", "core/list-item" ]
        }
    }
}

Nope, this still won't let me interact with the list item:

list-item-in-media-text

This is surprising - this same set of allowedBlocks works fine at the root level for inserting core/list-items, but not in core/media-text. I'll try explicitly making a nested rule for core/list:

core/list in a core/media-text - Attempt 4

{
    "type": "role",
    "roles": [ "editor" ],
    "allowedBlocks": [ "core/media-text", "core/heading" ],
    "blockSettings": {
        "core/media-text": {
            "allowedBlocks": [ "core/paragraph", "core/list" ],
            "core/list": {
                "allowedBlocks": [ "core/list-item" ]
            }
        }
    }
}

Still doesn't work, same result as above. Maybe I need to add core/paragraph to the core/list rules too?

core/list in a core/media-text - Attempt 5

{
    "type": "role",
    "roles": [ "editor" ],
    "allowedBlocks": [ "core/media-text", "core/heading" ],
    "blockSettings": {
        "core/media-text": {
            "allowedBlocks": [ "core/paragraph", "core/list" ],
            "core/list": {
                "allowedBlocks": [ "core/paragraph", "core/list-item" ]
            }
        }
    }
}

That didn't do it either, same result as before:

media-text-list-item

I can't find a way to make the original rule idea work:

The goal is that we allow core/media-text blocks at the root level, and inside core/media-text blocks we only allow lists.

Conclusion

There are a few potential issues here:

  1. First and most importantly, I'm not sure how to make a ruleset work where I allow core/list nested in a block and can interact with it's children core/list-item blocks. I can only apply these rules at the root level, but not in a nested block. This seems like a bug, or I'm missing a configuration step.

  2. From "Attempt 1" above:

    It seems strange that the default rule core/paragraph doesn't apply here. It may be worth evaluating whether default blocks should be included automatically in allowedBlocks, since I'll need to re-add core/paragraph to any nested block settings.

    Our default ruleset makes it easy to avoid repeating rules like core/paragraph everywhere, but it only applies to the root set of blocks. Once we start making inner allowedBlocks, we're still required to repeat that. We should consider solutions where that isn't necessary.

  3. From "Attempt 2":

    This may be another potential area of improvement. Some blocks like core/list or core/columns are only usable with their child blocks enabled. Potentially we can hardcode some core blocks to support necessary child items to remove this responsibility from the rules file creator.

    This would ease some of the difficulty of dealing with fiddly inner allowedBlocks rules for some block types.

  4. From "Attempt 3":

    This is surprising - this same set of allowedBlocks works fine at the root level for inserting core/list-items, but not in core/media-text.

    The surprising bit here is that this set of rules works fine, and allows me to insert core/list-items in core/media-text:

    {
        "type": "role",
        "roles": [ "editor" ],
        "allowedBlocks": [ "core/media-text", "core/list", "core/list-item" ]
    }

    However, this set doesn't work, with the same rules applied at the core/media-text block level:

    {
        "type": "role",
        "roles": [ "editor" ],
        "allowedBlocks": [ "core/media-text" ],
        "blockSettings": {
            "core/media-text": {
                "allowedBlocks": [ "core/paragraph", "core/list", "core/list-item" ]
            }
        }
    }

    Root-level allowedBlocks apply to children blocks, but inner allowedBlocks appear to not. This feels inconsistent.

ingeniumed commented 1 year ago

First and most importantly, I'm not sure how to make a ruleset work where I allow core/list nested in a block and can interact with it's children core/list-item blocks. I can only apply these rules at the root level, but not in a nested block. This seems like a bug, or I'm missing a configuration step.

This applies not only here, but also to attempt 3 and attempt 4.

This is definitely a bug. It's actually a TODO in the block-utils.js code but wasn't documented in the limitations in the README. For block searches within a child we only go one level in the parent-child hierarchy in the governance rules. This means that, while attempt 4 is correct it's not being parsed correctly.

Our default ruleset makes it easy to avoid repeating rules like core/paragraph everywhere, but it only applies to the root set of blocks. Once we start making inner allowedBlocks, we're still required to repeat that. We should consider solutions where that isn't necessary.

This one has been tricky to come up with a solution because while we do want to make it easy to use, we also want to be restrictive. The root level allowed blocks should be separate from the parent-child allowed blocks in case someone does want to keep them different.

This may be another potential area of improvement. Some blocks like core/list or core/columns are only usable with their child blocks enabled. Potentially we can hardcode some core blocks to support necessary child items to remove this responsibility from the rules file creator.

This was how the original code was, but the challenge is that we have to maintain this list and that can get tedious. That's also not going to work for custom blocks, which is the onus was changed to be on the rule maker. I'm trying to see if we can use the allowedBlocks property within the innerBlocks of a block to determine this, but that's not exposed AFAIK.

Hope that makes sense as to why this is happening. I'll open up a bug for that undocumented limitation.

ingeniumed commented 1 year ago

Opened #31 for this