Typography Panel in the Block Inspector for `core/paragraph` never goes away even with all features disabled

kraftner commented 5 months ago

Description

If you disable all typography features the corresponding panel should completely disappear. Unfortunately for paragraph blocks this never happens.

The reason for this is that the <DropCapControl> always triggers the rendering of the panel. This comes from the fact that if you add an <InspectorControl> with a group it renders the panel even if the only content is null. So to conditionally render the control you must not even return the <InspectorControl>.

But this PR moved the check inside the <InspectorControl> https://github.com/WordPress/gutenberg/commit/dc95863e510c213d8896dd71843d2c3b62fd260d#diff-6fffd1b2241c181cc9077eb4508a03fe473b7bf22078a72ba9b2b2dfebd8fb92L101-R134

This makes this a regression in WP 6.5.

Step-by-step reproduction instructions

Disable all typography settings in theme.json

"typography": {
"fontSizes": [],
"customFontSize": false,
"defaultFontSizes": false,
"writingMode": false,
"dropCap": false,
"textDecoration": false,
"letterSpacing": false,
"textTransform": false,
"fontWeight": false,
"fontStyle": false,
"lineHeight": false
},

You'll also be fighting against the fact that the font sizes setting doesn't fully go away. I used this workaround to fix this first:

add_filter('wp_theme_json_data_default', function($themeJson) {
    $data = [
        'version'  => 2,
        'settings' => [
            'typography' => [
                'fontSizes' => []
            ]
        ]
    ];
    return $themeJson->update_with($data);
});

Insert a paragraph in the block editor.
You'll still see an empty "Typography" panel.

To further demonstrate that this is the problem you can use this code to trigger the same problem with any other block that would normally not show the Typography Panel if all features are disabled. I used the core/heading block in the following code:

add_action('enqueue_block_editor_assets', function () {
    $script = <<<HTML
    (function () {
        var el = wp.element.createElement;
        wp.hooks.addFilter(
            "editor.BlockEdit",
            "kraftner/non-empty-typography-slot",
            wp.compose.createHigherOrderComponent( function( BlockEdit ) {
                return function( props ) {
                    if(props.name !== 'core/heading'){
                        return el(BlockEdit, props);
                    }
                    return el(
                        wp.element.Fragment,
                        {},
                        el(
                            wp.blockEditor.InspectorControls,
                            { group: "typography" },
                            null
                        ),
                        el(BlockEdit, props),
                    );
                };
            }, 'withInspectorControls' ),
        );
    })();
    HTML;
    wp_add_inline_script('wp-blocks', $script);
});

You'll notice that I added an <InspectorControl> with only null as content but the panel still renders.

Screenshots, screen recording, code snippet

grafik

Environment info

WordPress 6.5 with and without Gutenberg 18.2.0.

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

t-hamano commented 5 months ago

I was able to reproduce it.

I've set all the fields defined in the JSON Schema to empty or false, but I can't completely dismiss the panel. On the other hand, the Color panel and Dimension panel can be deleted.

Below is the theme.json I used for testing.

{
    "$schema": "https://schemas.wp.org/trunk/theme.json",
    "version": 3,
    "settings": {
        "appearanceTools": true,
        "layout": {
            "contentSize": "840px",
            "wideSize": "1100px"
        },
        "color": {
            "background": false,
            "text": false,
            "link": false
        },
        "typography": {
            "fontSizes": [],
            "customFontSize": false,
            "defaultFontSizes": false,
            "writingMode": false,
            "dropCap": false,
            "textDecoration": false,
            "letterSpacing": false,
            "textTransform": false,
            "fontWeight": false,
            "fontStyle": false,
            "lineHeight": false,
            "textAlign": false,
            "textColumns": false,
            "fontFamilies": [],
            "fluid": false
        },
        "spacing": {
            "margin": false,
            "padding": false
        }
    }
}

kraftner commented 5 months ago

As a quick and dirty workaround I've come up with this for now:

$css = <<<HTML
.typography-block-support-panel:has(.components-tools-panel-header + :empty:last-child) {
    display: none;
}        
HTML;

wp_add_inline_style('wp-edit-post', $css);

What it basically does is hide the typography panel if the element following the panel header is empty and at the same time the last child which should only apply with this issue.

ramonjd commented 5 months ago

I optimistically threw up a PR to (hopefully) address this:

https://github.com/WordPress/gutenberg/pull/61342

But only after digested @ellatrix's comments to not call useSettings unconditionally.

Maybe <DropCapControl /> needs to be in https://github.com/WordPress/gutenberg/blob/ca99cb90ba86483d594296fb6162daf2eb2f1215/packages/block-editor/src/components/global-styles/typography-panel.js?

ramonjd commented 5 months ago

Maybe needs to be in https://github.com/WordPress/gutenberg/blob/ca99cb90ba86483d594296fb6162daf2eb2f1215/packages/block-editor/src/components/global-styles/typography-panel.js?

This is the direction I was thinking:

https://github.com/WordPress/gutenberg/pull/61343

Might be way off as I didn't get time to look at all angles. Hope it helps someone though.

If I get more time I can circle back.

kraftner commented 5 months ago

@ramonjd Thanks for the attempts! I think your second attempt, while interesting, also isn't an ideal solution. In contrast to the the other settings in there DropCap is a paragraph-specific setting and hence should live with the block and not the Typography Panel if you ask me.

I thought about it some more and maybe the solution lies in stepping back and looking at the big picture:

Why is the panel rendering in the first place if all it contains is a slot with null?

So I looked into where this seems to be happening. It seems that there is a check for an empty slot, but it only checks for an empty array but doesn't care about what is in that array:

https://github.com/WordPress/gutenberg/blob/0612581c89f1c1e408168bc6669abc724e5ee33f/packages/block-editor/src/components/inspector-controls/slot.js#L58-L60

So maybe we should just deepen that check and also bail out if the slot only contains null values?

I then tried to go down the rabbit hole some more and see if that was a general problem with Slots that they do not filter out null. But this is were I've hit a dead end with my React (debugging) skills for now. It seems to happen somewhere here. Maybe somebody else knows where to look.

ramonjd commented 5 months ago

I think your second attempt, while interesting, also isn't an ideal solution. In contrast to the the other settings in there DropCap is a paragraph-specific setting and hence should live with the block and not the Typography Panel if you ask me.

Fair point.

There's an argument that other blocks could still use it, eg. Pullquote, but as it stands it's just for paragraph.

If there's a way to handle it in the ToolsPanel, then it sounds like neater way! Good one!

So maybe we should just deepen that check and also bail out if the slot only contains null values?

I had a quick look and I think the array is populated with ref items, which are the InspectorControls themselves 🤔

~But it looks like we could test for undefined so maybe something like the following would work:~

    if ( ! fills?.length && ! fills.every( ( fill ) => !! fill ) ) {
        return null;
    }

Nope, I'm having a great day 😆 Checking for children on the fill doesn't appear to yield anything either.

Might come back when my mind is fresher.

ramonjd commented 5 months ago

Something like this might work, but I'm not sure.

Basically checking for null on a React component being called as a function before rendering the JSX.

I think it's bad practice to render a component this way, but maybe only to check for null first?

I'm out of ideas 🤣

Call DropCapEnabled as a function to check for `null` before rendering DropCapControl

```diff diff --git a/packages/block-editor/src/components/inspector-controls/fill.js b/packages/block-editor/src/components/inspector-controls/fill.js index 456b33af91..c8cc86cb7b 100644 --- a/packages/block-editor/src/components/inspector-controls/fill.js +++ b/packages/block-editor/src/components/inspector-controls/fill.js @@ -37,6 +37,11 @@ export default function InspectorControlsFill( { } const context = useBlockEditContext(); + + if ( ! children ) { + return null; + } + const Fill = groups[ group ]?.Fill; if ( ! Fill ) { warning( `Unknown InspectorControls group "${ group }" provided.` ); diff --git a/packages/block-library/src/paragraph/edit.js b/packages/block-library/src/paragraph/edit.js index 061cb672ae..c1706467c6 100644 --- a/packages/block-library/src/paragraph/edit.js +++ b/packages/block-library/src/paragraph/edit.js @@ -47,7 +47,7 @@ function hasDropCapDisabled( align ) { return align === ( isRTL() ? 'left' : 'right' ) || align === 'center'; } -function DropCapControl( { clientId, attributes, setAttributes } ) { +function DropCapEnabled() { // Please do not add a useSelect call to the paragraph block unconditionally. // Every useSelect added to a (frequently used) block will degrade load // and type performance. By moving it within InspectorControls, the subscription is @@ -58,6 +58,10 @@ function DropCapControl( { clientId, attributes, setAttributes } ) { return null; } + return <>; +} + +function DropCapControl( { clientId, attributes, setAttributes } ) { const { align, dropCap } = attributes; let helpText; @@ -132,11 +136,19 @@ function ParagraphBlock( { ) } - + { /* + * Check return value of function before rendering JSX element to avoid rendering + * the inspector control, which checks for React children. + * Ideally we shouldn't call a React function component directly, + * so only render component if it passes. + */ } + { null !== DropCapEnabled() ? ( + + ) : null }

t-hamano commented 5 months ago

@ramonjd Thank you for all your ideas🙇

If I understand correctly, the root cause of the empty Typography panel appearing is because the InspectorControls is always rendered regardless of whether the Paragraph block supports DropCap or not. In order to conditionally render the InspectorConrols component, we need to run useSettings() inside the Edit component and check if DropCap is supported. However, from a performance perspective, this is not recommended. Is this correct?

I'm also trying to find a way to solve the problem without making changes to the InspectorControl component itself, but I haven't come up with a good idea yet 😅

ramonjd commented 5 months ago

I'm also trying to find a way to solve the problem without making changes to the InspectorControl component itself, but I haven't come up with a good idea yet

Thanks for looking. 🍺

This does work, but I haven't checked for side-effects, e.g., if the parent component were to call any of the same hooks so I don't think it's safe according to the warnings in that link I cited.

Maybe the typography hook could mutate the props/attributes for the block? I'm getting desperate 🤣

kraftner commented 3 months ago

Having talked to multiple people at contributor day at WCEU about this issue and nobody feeling capable of finding a clean solution I now do what I was recommended to do: @youknowriad can you have a look at this issue please?

youknowriad commented 3 months ago

I went through the discussions here and I'd say that if we're trying to solve a solution in terms of React and Slot/Fill, we're probably going in the wrong direction to be honest. Slots in general shouldn't actually care about what the Fills render. Especially important for performance as well as otherwise we would risk re-rendering a lot more than needed when updating just a small area (fill).

There are options to solve this issue though:

I don't think it's out of the question to move DropCap to the typography panel. But to do this properly, we need to build a way to generate its styles dynamically (like other block hooks)...
That said, it's not the perfect solution because plugins can actually inject stuff that do the same thing as DropCapControl within the typography panel as well (or any other tools panel for that matter).
So for me, the CSS solution is actually a good one (something like this https://github.com/WordPress/gutenberg/issues/61231#issuecomment-2085060309). I think we could make it generic within the ToolsPanel component, :has is something that is widely available to us now, so we could just leverage it.

That said, in terms of extensibility and DevX, it's not really a great DevX to have to disable all typography controls one by one if we want to remove the panel entirely. Maybe we should think about having a way to disable it all at once with a typography.enabled setting or something (that obviously defaults to true) and same for all the other panels.

kraftner commented 3 months ago

Thanks for your fast response @youknowriad!

To be honest I am stunned that if I read that right you're seeing the CSS solution as the only viable one. To me this sounds really messy and somewhat like an architectural flaw. But if you say there isn't a clean/perfect solution I'll takes this as a fact since there is probably nobody with better insight into the guts of Gutenberg.

Concerning the batch removal I think your proposal for a general off-switch is nice, but to be honest probably beyond the scope of this issue. I also think that it may be a nice addition but not necessary a general solution for the issue at hand: Since there are e.g. multiple ways some of the features could be disabled beyond theme.json it is well possible that ending up with everything disabled isn't necessarily the result of turning it off all at once but turning different features off at different places and then just ending up with all disabled as the final result.

youknowriad commented 3 months ago

But if you say there isn't a clean/perfect solution I'll takes this as a fact since there is probably nobody with better insight into the guts of Gutenberg.

In this particular case, there's an easy solution (to revert to the previous check) before InspectorControls but this will trade performance for "code guidelines" or "nice code". I would pick nice code or guidelines if the performance impact was negligible but in this particular case, the performance impact is something meaningful IMO and I would rather over optimize, typing performance and things like that in an editor.

I don't think there's any architectural flaw here. There's just competing requirements:

Performance
Extensibility (adding slots)
Control (disabling UI that is potentially extensible)

kraftner commented 3 months ago

Well I am not going to waste anybody's time here arguing about whether such a CSS fix should ever be an acceptable solution to something that should never be rendered, I guess you can read my point of view between the lines. :wink:

Returning to constructively moving this issue forward:

I can try crafting a more general CSS selector, but I'm not really sure where those should then be best added in the overall codebase so this can move on towards a PR. Any pointers?

youknowriad commented 3 months ago

Somewhere in the styles of this component probably https://github.com/WordPress/gutenberg/tree/trunk/packages/components/src/tools-panel

WordPress / gutenberg