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

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

Closed
kraftner opened this issue Apr 30, 2024 · 25 comments · Fixed by #67111
Assignees
Labels
[Block] Paragraph Affects the Paragraph Block [Feature] Typography Font and typography-related issues and PRs [Type] Bug An existing feature does not function as intended

Comments

@kraftner
Copy link

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> dc95863#diff-6fffd1b2241c181cc9077eb4508a03fe473b7bf22078a72ba9b2b2dfebd8fb92L101-R134

This makes this a regression in WP 6.5.

Step-by-step reproduction instructions

  1. 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);
});
  1. Insert a paragraph in the block editor.
  2. 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

@kraftner kraftner added the [Type] Bug An existing feature does not function as intended label Apr 30, 2024
@Mamaduka Mamaduka added Needs Testing Needs further testing to be confirmed. [Block] Paragraph Affects the Paragraph Block [Feature] Typography Font and typography-related issues and PRs labels Apr 30, 2024
@t-hamano
Copy link
Contributor

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.

image

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
Copy link
Author

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
Copy link
Member

ramonjd commented May 3, 2024

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

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
Copy link
Member

ramonjd commented May 3, 2024

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:

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
Copy link
Author

kraftner commented May 3, 2024

@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:

if ( ! fills?.length ) {
return null;
}

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
Copy link
Member

ramonjd commented May 3, 2024

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
Copy link
Member

ramonjd commented May 3, 2024

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 --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( {
 				</BlockControls>
 			) }
 			<InspectorControls group="typography">
-				<DropCapControl
-					clientId={ clientId }
-					attributes={ attributes }
-					setAttributes={ setAttributes }
-				/>
+				{ /*
+				 * 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() ? (
+					<DropCapControl
+						clientId={ clientId }
+						attributes={ attributes }
+						setAttributes={ setAttributes }
+					/>
+				) : null }
 			</InspectorControls>
 			<RichText
 				identifier="content"

@t-hamano
Copy link
Contributor

t-hamano commented May 3, 2024

@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
Copy link
Member

ramonjd commented May 3, 2024

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 🤣

@Mamaduka Mamaduka removed [Status] In Progress Tracking issues with work in progress Needs Testing Needs further testing to be confirmed. labels May 14, 2024
@kraftner
Copy link
Author

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
Copy link
Contributor

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 Typography Panel in the Block Inspector for core/paragraph never goes away even with all features disabled #61231 (comment)). 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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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. 😉

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
Copy link
Contributor

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

@briannaneiderman
Copy link

Until a solution is ready, can this at least be added as a notice in the documentation? I spent a solid chunk of time debugging what I thought was a mistake on my end because I didn't know this was a known issue.

Thank you!

@Tmeemu
Copy link

Tmeemu commented Nov 18, 2024

The editor no longer seems to load the component panel in the sidebar when the panel is empty. I.e. when the font size selection has been removed with "fontSizes": [] and there are no other settings enabled either, the panel disappears:

Image

With "lineHeight": true for the same example. The panel is back, as it should be:

Image

So, the somewhat hacky workaround now seems like a way to get rid of it altogether. I'd assume this is a result of a feature/bug fix implemented to the core elsewhere, potentially in 6.7 based on how recent Brianna's comment above is. Can someone confirm?

"fontSize": false still has no effect.

@kraftner
Copy link
Author

@Tmeemu I haven't tried with 6.7. yet, but have you tried with a core/paragraph block? This bug is specific to that block due to the way the drop cap feature is implemented as said in the title and initial bug description.

@t-hamano
Copy link
Contributor

This bug is specific to that block due to the way the drop cap feature is implemented as said in the title and initial bug description.

That's right. This problem hasn't been solved yet.

@youknowriad
Copy link
Contributor

Did something try just moving <InspectorControls group="typography"> within DropCapControl component?

@Mamaduka
Copy link
Member

@youknowriad, that would regress #56967.

@youknowriad
Copy link
Contributor

@Mamaduka What if we extract the condition that we use to render the InspectorControls conditionally to its own component/hook.

<SelectedBlockOnlyControl>
    <DropCapControl />
</SelectedBlockOnlyControl>

SelectedBlockOnlyControl wouldn't render a "Fill" here.

The downside is that condition will be run twice for selected blocks but that is probably ok.

@Mamaduka
Copy link
Member

Mamaduka commented Nov 19, 2024

@youknowriad, I have a similar but simpler solution if we're just going for a one-time fix. I'll create a PR.

Eventually, DropCapControl could be absorbed into main typography controls, but that's a different discussion :)

Update: Here's my proposal - #67111.

@ramonjd
Copy link
Member

ramonjd commented Nov 19, 2024

Eventually, DropCapControl could be absorbed into main typography controls, but that's a different discussion :)

I think I tried that ages ago. Here it is for reference if it helps later:

@Tmeemu
Copy link

Tmeemu commented Nov 25, 2024

@kraftner I can also verify that the Paragraph block does still show the empty panel.

As chance would have it, it's one of the few blocks that we still have the size settings enabled on – while numerous others have all Typography settings disabled. Until "now" (i.e. on 6.5), all the other blocks presented the outward similar issue of showing an empty Typography panel. That issue now appears to be fixed though with a quick search, I couldn't find out more (to link here for anyone else who might be searching for it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Paragraph Affects the Paragraph Block [Feature] Typography Font and typography-related issues and PRs [Type] Bug An existing feature does not function as intended
Projects
None yet
7 participants