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

Please add a way to not delete wp-block-group__inner-container when using theme.json. #41124

Open
ddryo opened this issue May 18, 2022 · 16 comments
Labels
[Block] Group Affects the Group Block CSS Styling Related to editor and front end styles, CSS-specific issues. [Focus] Blocks Adoption For issues that directly impact the ability to adopt features of Gutenberg. [Type] Enhancement A suggestion for improvement.

Comments

@ddryo
Copy link
Contributor

ddryo commented May 18, 2022

When using theme.json, the inner Group block (.wp-block-group__inner-container) is forced to be deleted.

I understand why you want to spec it this way, but I don't understand why you can't choose to turn it off or not.

The forced removal makes it impossible to use theme.json in a theme that is already being used by many users.

The presence of .wp-block-group__inner-container also breaks what was possible with styling.

Why force the removal of the DOM while ignoring backwards compatibility?

The evolution of Gutenberg is great.
So I would like to keep up with it even if the changes are too drastic. However, it is unfortunate that there is such a complete disregard for classic theme developers.

At least allow us to choose to keep .wp-block-group__inner-container in theme.json.

Translated with www.DeepL.com/Translator (free version)

@justintadlock justintadlock added the [Block] Group Affects the Group Block label May 24, 2022
@sergiu-radu
Copy link

Totally agree with @ddryo, we need a way to keep the .wp-block-group__inner-container when using the theme.json file in a classic theme.

@eduard-co
Copy link

eduard-co commented Jun 1, 2022

Following this thread and agreeing with the statements above.

There are many use cases in which classic themes are still in use and with very good reason. This new change breaks the flow when using such systems, so it is a regression pretty much. I vote to return to the old behaviour.

@oldrup
Copy link

oldrup commented Jun 1, 2022

As an experienced end user, having created most of my content in Gutenberg for years, the removal of the .wp-block-group__inner-container does mess up my layouts big time. I'm used to markup changes as Gutenberg evolves, and I update my styling accordingly, but this is too big a change.

I know the developers of the classic theme I'm using, are embracing the changes coming with theme.json, as am I, and a good way to "ease into the new ways" would be to ensure critical backwards compatibility, so we don't have to rebuild every single existing page when making the switch.

@skorasaurus skorasaurus added the CSS Styling Related to editor and front end styles, CSS-specific issues. label Jun 1, 2022
@annezazu annezazu added the [Focus] Blocks Adoption For issues that directly impact the ability to adopt features of Gutenberg. label Jun 2, 2022
@andrewserong
Copy link
Contributor

andrewserong commented Jun 2, 2022

I've added this to the layouts tracking issue in #39336. From the discussion here, it sounds like we need a way for themes that have added in a theme.json file, but aren't yet ready to remove the group's inner container, to opt-in to preserving that inner container.

Currently, in the Layout block support, there's a check to see if theme_has_support() that skips adding back in the container. Perhaps we could have a flag or filter that classic themes can use to opt-in to preserve the inner container, even if this check returns true. (Just a note: opting-in to preserving that inner container may break blockGap support provided by theme.json so if we add in the flag, it still could result in some potentially unstable behaviour 🤔)

@carolinan
Copy link
Contributor

carolinan commented Aug 5, 2022

I understand that this is problematic, but I also don't think that adding a flag for keeping the inner div would be a good solution for the long term.

If the theme developer chooses to add a theme.json, they have already decided to make changes to the theme, and they will have new tools to control the layout that allows them to replace / remove the broken CSS.

@sergiu-radu
Copy link

Hi @carolinan, that's a good point but at the same time we have to think about users that already built their websites using the container block that has an inner div.

If we add the theme.json file in to the theme - everything will break for them...

So, we (theme developers) want to get advantage of this theme.json file but at the same time we can't use it because it breaks our users websites...

This told, I really think we need a flag to support somehow those users.

I really hope you can give a chance to this idea :)

Thank you!

@carolinan
Copy link
Contributor

It wont break for them if you also update the CSS.

@carolinan
Copy link
Contributor

carolinan commented Aug 5, 2022

This would be a lot easier to illustrate with complete code examples from theme authors, showing what exactly breaks for them.

@johnfleming
Copy link

@carolinan Following this conversation. I don't have a perspective on the right solution to this issue, but I think it is critical that we enable people who have sites built with Classic Themes to have access to the latest capabilities of all Core blocks without forcing significant updates to those themes or replacement of those themes. This is especially true for theme developers like Astra and Blocksy who have enabled significant adoption of the Block Editor since 2018.

@carolinan
Copy link
Contributor

I would rather that we explored ways to help the users update their block markup without having to open every single post/page/post type manually, than support these inner divs and a soup of setting combinations "forever".

@mateuswetah
Copy link
Contributor

mateuswetah commented Aug 5, 2022

The problem here is that the container update its acting as blocker to several other theme.json v2 features that would benefit enormously this themes. Seriously, using the block editor with Blocksy for example is amazing, but once you learn that you cannot take advantage of new spacing controls gaps, it really feels limited.

I am certainly not aware of the internals, but I would like to believe that the inner container change is only one of the many things that theme.json brings and not all of them are so interdependent to a point that it would be prejudicial to have a flag disabling it for backwards capability sake.

In defense of these theme authors, changing containers logic is not something easy to do. I have a classic theme and I gave up adopting theme.json. It is one of those things that, no matter how much css you add, you may interfere with client content or other features that the classic theme may have built around it (lets remember many of these guys were making powerful layout edition solutions while Gutenberg wasn't ready, which certainly helped increase the adoption of it).

@mateuswetah
Copy link
Contributor

mateuswetah commented Sep 5, 2022

Am I right to say that the new add_theme_support( 'appearance-tools' ); introduced in Gutenberg 14.0, solves a good portion of this by allowing us to have all those nice features without opting-in to the new containers structure?

@MatzeKitt
Copy link
Contributor

By the way, if you need a quick solution for this, you can use this PHP code:

/**
 * Add group block inner container.
 * The inner container has been removed in WordPress 5.8 if a theme.json
 * is available in the theme.
 * 
 * @param	string	$block_content The block content
 * @return	string The updated block content
 */
function my_add_block_group_inner( $block_content ) {
	libxml_use_internal_errors( true );
	$dom = new DOMDocument();
	$dom->loadHTML(
		mb_convert_encoding(
			'<html>' . $block_content . '</html>',
			'HTML-ENTITIES',
			'UTF-8'
		),
		LIBXML_HTML_NOIMPLIED | LIBXML_HTML_NODEFDTD
	);
	
	foreach ( $dom->getElementsByTagName( 'div' ) as $div ) {
		// check for desired class name
		if (
			strpos( $div->getAttribute( 'class' ), 'wp-block-group' ) === false
			|| strpos( $div->getAttribute( 'class' ), 'wp-block-group__inner-container' ) !== false
		) {
			continue;
		}
		
		// check if we already processed this div
		foreach ( $div->childNodes as $childNode ) {
			if (
				method_exists( $childNode, 'getAttribute' )
				&& strpos( $childNode->getAttribute( 'class' ), 'wp-block-group__inner-container' ) !== false
			) {
				continue 2;
			}
		}
		
		// create the inner container element
		$inner_container = $dom->createElement( 'div' );
		$inner_container->setAttribute( 'class', 'wp-block-group__inner-container' );
		// get all children of the current group
		$children = iterator_to_array( $div->childNodes );
		
		// append all children to the inner container
		foreach ( $children as $child ) {
			$inner_container->appendChild( $child );
		}
		
		// append new inner container to the group block
		$div->appendChild( $inner_container );
	}
	
	return str_replace( [ '<html>', '</html>' ], '', $dom->saveHTML( $dom->documentElement ) );
}

add_filter( 'render_block_core/group', 'my_add_block_group_inner' );

@Soean
Copy link
Member

Soean commented Sep 12, 2022

@MatzeKitt or use a copy of wp_restore_group_inner_container without the JSON support check.
https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/block-supports/layout.php#L246-L273

@ddryo
Copy link
Contributor Author

ddryo commented Oct 5, 2022

@carolinan

carolinan > It wont break for them if you also update the CSS.

No no no. It should not be that simple.

As for the styles provided by the theme, you are right, it may be possible to handle it by simply editing the CSS in the theme.

However, for a theme with tens of thousands of users, many users will be styling their sites with .wp-block-group__inner-container on their own sites.

The problem for the theme developer is that they want to introduce theme.json and use the new functionality, but they are destroying the content of many users to do so.

Translated with www.DeepL.com/Translator (free version)

@michaelbourne
Copy link

This is a DOM issue and backwards compatibility issue, saying "just change the CSS" feels a bit too curt for WordPress, aside from the fact it's technically just bad advice.

The function @Soean referenced will help us hack it in the meantime. Would love for this to get some more visibility.

@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block CSS Styling Related to editor and front end styles, CSS-specific issues. [Focus] Blocks Adoption For issues that directly impact the ability to adopt features of Gutenberg. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests