Add a mechanism to filter out empty theme regions when all blocks inside are empty

If this problem was discovered on a version of Drupal that is not 11.x, add that information in the issue summary and leave the version at 11.x. In Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies. Also mentioned on the version section of the list of issue fields documentation. Thanks.

I have this code in twig :

{% set aside_start_top_rendered = page.aside_start_top|render %}
{% set aside_start_bottom_rendered = page.aside_start_bottom|render %}

{% set aside_exists = (aside_start_top_rendered is real_content or aside_start_bottom_rendered is real_content) and not narrow and container_type != 'fluid' %}

After updating from 11.1.18 to 11.2.2, aside_exists now always returns TRUE because of placeholders

Before 11.2.2, my block had this in it :

{"cfa_navigationprincipale":{"#cache":{"keys":["entity_view","block","cfa_navigationprincipale"],"contexts":["languages:language_interface","route.menu_active_trails:main"],"tags":["block_view","config:block.block.cfa_navigationprincipale","config:system.menu.main"],"max-age":-1},"#weight":0,"#lazy_builder":["Drupal\\block\\BlockViewBuilder::lazyBuilder",["cfa_navigationprincipale","full",null]]},"#sorted":true,"#theme_wrappers":["region"],"#region":"aside_start_top"}

After 11.2.2

 {"cfa_navigationprincipale":{"#cache":{"keys":["entity_view","block","cfa_navigationprincipale"],"contexts":["languages:language_interface","route.menu_active_trails:main"],"tags":["block_view","config:block.block.cfa_navigationprincipale","config:system.menu.main"],"max-age":-1},"#weight":0,"#lazy_builder":["Drupal\\block\\BlockViewBuilder::lazyBuilder",["cfa_navigationprincipale","full",null]],"#create_placeholder":true},"#sorted":true,"#theme_wrappers":["region"],"#region":"aside_start_top"}  

As a workaround for now, I did in custom module :

function mymodule_preprocess_page(array &$variables) {
  if (isset($variables['page']['aside_start_top']['cfa_navigationprincipale'])) {
    $variables['page']['aside_start_top']['cfa_navigationprincipale']['#create_placeholder'] = FALSE;
  }
  if (isset($variables['page']['aside_start_bottom']['cfa_navigationprincipale'])) {
    $variables['page']['aside_start_bottom']['cfa_navigationprincipale']['#create_placeholder'] = FALSE;
  }
}

I was facing the same issue but for merge 12587 works fine

There are also other files this temporary fix needs to be applied to. For example:

core/modules/system/src/Plugin/Block/SystemMenuBlock.php

This stems from this commit: https://git.drupalcode.org/project/drupal/-/commit/bc383a3

As far as I can tell this now makes it impossible to determine if a region should be rendered by the theme layer. I would think this is a fairly major issue.

Static patch based on the MR above, for use in composer workflows.

Ran into this issue with a views block that is empty, but still rendering the HTML for it.
MR / patch resolves this issue.

This is yet another facet (ahem) of #953034: [meta] Themes improperly check renderable arrays when determining visibility

Placeholdering happens for performance reasons but it clashes with theming. It does not even seem possible to fix this but what do I know.

Placeholdering happens for performance reasons but it clashes with theming. It does not even seem possible to fix this but what do I know.

The only thing I can think of is more of a workaround than a solution, and that is to add a configuration option to the affected block plugins that would allow placeholdering to be disabled on a per block basis via the block configuration form. But I don't think it'd be viable long term as the use of placeholdering and/or Fibers/async, etc. expands for better performance during rendering.

We've had this issue on a couple sites now. We've fixed this without patching core by adding something like this to our .theme file for the blocks where we're using the page.sidebar_first|render|striptags|trim pattern :

/**
 * Implements hook_preprocess_HOOK().
 */
function example_preprocess_page(&$variables) {
  if (!empty($variables['page']['sidebar_first']['example_mainnavigation']['#create_placeholder'])) {
    $variables['page']['sidebar_first']['example_mainnavigation']['#create_placeholder'] = FALSE;
  }
}

This way, you can turn off the placeholder functionality in only the places where it's causing this problem.

It should be possible to filter out empty regions just before the page is sent via post-processing the HTML. It would obviously require some standardised HTML, maybe a data attribute, to identify regions, and then find all regions with that data attribute and remove them if they have no child elements.

We could add a response listener (not sure exactly where but somewhere like that) for HTML pages, maybe have it conditional operate based on something in theme.info.yml - then themes can add that flag when they remove the non-working Twig attempts.

That approach wouldn't work for big pipe, but we could add a js equivalent in big_pipe.js potentially that does the same thing after all placeholders are replaced.

It wouldn't work at all for no-js big pipe placeholder replacement but by default that is only used for things that aren't HTML elements, or in the case where js is disabled, which probably comes under 'graceful degradation'.

With cached placeholder strategy (and placeholder strategy denylists for special cases), a lot less requests actually use big pipe than they used to, so the no-js placeholder case is really minimal now.

Also this should be marked duplicate of #953034: [meta] Themes improperly check renderable arrays when determining visibility, I'm moving it to 'postponed' for now.

Actually that issue is a meta and includes some other scenarios other than blocks.

Re-titling and moving to a feature request, I might try to get a proof of concept of #15 implemented but if someone else wants to try that would be great.

Needs a response listener that gets the request content for HTML responses, gets all elements with a specific data attribute (TBD), and removes them if they have no children, then puts the updated HTML back on the response. We then need to add that data attribute in region.html.twig - can be done in ::preprocessRegion(). And then make the response listener only do its work if the active theme has a .info.yml key set.

There will be PHP overhead involved here but likely to be considerably less than the PHP and database overhead we've saved via placeholdering blocks.

Added an MR. This needs test coverage but I would love manual testing from anyone experiencing the bug - I tested manually with Umami and the help region.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Switched to str_replace() for the actual replacements, using $dom->saveHtml() resulted in various encoding/escaping weirdness in tests.

Did some quick profiling of the current status and it's coming in about 4ms.

Since the IS mentions render|striptags|trim|length > 0 as empty content detection, I wonder if there can be an option to allow 'data-filter-empty-region' to have a value of "noemptytags" or something, which could be set in a theme (or module's) preprocess, and in the subscriber, would including stripping the tags from the region content to detect "emptiness".

Re stripping tags: on second thought, there's a risk of removing blocks that only have <img /> tags.

Handful more changes to try to get tests passing and avoid triggering notices/warnings.

Still about 4ms time taken and about 1mb memory on the front page of Umami logged in. So it's not free, still think it's better than themes manually undoing the placeholder performance improvements we've added, and we can still make it opt-in in theme.info.yml

Re stripping tags: on second thought, there's a risk of removing blocks that only have tags.

Yeah it seems risky and I'm not sure it's necessary. If a block is rendering empty markup that's a problem with the block plugin or configuration usually.

I wondered a bit if we couldn't use https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/:empty for this instead of PHP, select empty regions with CSS and display: none, and the answer is not really because it doesn't trim whitespace. Maybe we could trim the whitespace in Twig then use it, but feels fragile.

Got to a green test run here so while this still needs test coverage, moving to needs review for the concept.

Added an additional MR here that attempts to achieve the same thing with the CSS :empty pseudo class, it does not work yet because there is one additional bit of whitespace in the region divs that I don't know where it comes from. The advantage of CSS is it would transparently work for javascript big pipe placeholders too (which are replaced or removed after the response is sent).

Tried to update the issue summary for the two options that have MRs and also two options that I vaguely thought about but mostly rejected as either not as good or too hard.

Since both me and @alexpott are in favour of the CSS option, I've hidden the other two MRs for now.

I think the CSS option wins because it works for big pipe too which feels really nice.

Unsure if this is relevant here or not, but I looked into :empty for this kind of thing some time ago, and in addition to whitespace issues mentioned which you can typically fix with {% apply spaceless %} in a custom theme's region templates, (IIRC) it also easily breaks when Twig Debug is on, complicating the theming process entirely.

Hello,

I have tested the MR applying only the changes from:
- core/lib/Drupal/Core/Theme/ThemePreprocess.php
- core/modules/system/css/components/hidden.module.css
- core/modules/system/templates/region.html.twig

See screenshot, even if the component is empty with the data attribute, it is still displayed.

Tested with Brave and Firefox.

Relying on a CSS method (display: none;) to hide unused regions will "break" css psuedoselectors like :nth-child(), :first-child, :last-child. It'd also not work when you'd otherwise want to use immediate sibling selectors - say, if you want to target a main content area immediately preceded by a sidebar with something like .region--left-sidebar + .region--main-content - this would still apply styles to .region--main-content if you used display: none; on the .region--left-sidebar.

For this reason, I don't think the CSS approach ([data-filter-empty-region]:empty { display: none; }) is a good idea.

@seanpb do you actually do those things in themes? Seems like quite a bit of an edge case.

If we do want to support that, then we could add both approaches, but make the response filter opt-in via theme .info.yml as discussed above. If we can do this with zero PHP overhead for most sites, I think we should try to do that as the default behaviour.

If we put the CSS into its own library, we could then conditionally add it in system_page_attachments() based on whether the theme opts into the response filter approach or not too - although it's only one line of CSS it would be consistent with #2880237: [meta] Refactor system/base library, could maybe do that anyway thinking about it.

Discussed with @grimreaper in slack - we were able to figure out why the MR wasn't working with his theme but also went over a couple of other things.

1. There can be use-cases to filter other empty containers, the same mechanism will work for non-regions both with the CSS and response filter approach. I made the data attribute have a more generic name. Other systems wanting to do this like page builders would need to add the data attribute to the relevant container (layout component etc.)

2. To support potentially having a choice of the CSS or response filter approaches, I moved the CSS to its own library in core and attaching it in region preprocess where the data attribute is set. We can make the library attachment depend on theme info.yml if we do end up supporting both ways. Also makes it easier for themes to customise.

Discussed with @catch, I have added some spaceless into the SDC component I used for the help region in ui_suite_bootstrap, then it is working! Got a little flickering effect due to big_pipe, and being in dev mode without cache.

I think to cover more cases, a deeper solution (server side) will be needed but that's a good start.

Could the PHP filtering be made as a config (potentially hidden) or a setting?
I don't think option in the theme.info.yml is the best place. This feels strange to me to have to ask the front developer to know that and it may depend on the site usage, not the code itself.

PS: comment posted at the same moment :)

@grimreaper site usage is a good point, it's really the combination of conditional blocks that may or may not leave an entire region empty in addition to a theme that depends on empty regions not appearing at all (which not all do). If you only have one of these you don't necessarily have a problem.

I don't think I'd want to add a UI option for this, not going to be easy to explain, although wouldn't block it if a product or UX manager thinks we should.

We could add a settings.php flag, or we could add a parameter to the service set to FALSE, and then have an example in the site-specific services.yml setting it to TRUE. That last one would be the least amount of code to implement and most self-contained.

Minor aside: Twig has deprecated the spaceless filter for removal in version 4: https://twig.symfony.com/doc/3.x/deprecated.html#filters. (All usages were removed from Drupal core in #3486170: Remove use of deprecated "spaceless" filter in core templates.)

There's been no announcement about a projected Twig 4 release date, but it might be best to prepare other whitespace control implementations in contrib/custom templates and avoid using spaceless in conjunction with the solution that lands here.

Also another point about NOT putting in theme.info.yml, what about sub-themes? This smells opening doors to more issues than it is closing.

@catch thanks, I forgot the option of service parameter, which seems very good too.

Also #36 is completely big-pipe incompatible - regardless of the solution here, and any removal of the region by a response event is also going to be ineffective. Basically .region--left-sidebar + .region--main-content is not realistic for a theme that is for general use.

Yes the response filter only works for non-big pipe responses, whereas CSS will - so we'd need the CSS for big pipe anyway.

To actually remove the empty regions from the DOM for #36 in a reliable way would require JavaScript. If that's in the header it would be render blocking, so we'd need to make it inline js and compatible with CSPs. Or it could go in the footer, but that would result in Content Layout Shift (at least for regions that the CSS hasn't already hidden).

What I think we should do here is go ahead with the CSS approach because that works reliably for most cases, and open a follow-up to discuss response filters and JavaScript etc. for the remaining edge cases. It would also be possible to implement either or both of those in contrib.