[meta] Themes improperly check renderable arrays when determining visibility

Created on 26 October 2010, about 14 years ago
Updated 1 February 2023, almost 2 years ago

Problem/Motivation

In certain situations regions that contain empty blocks still render.

The original design goal of regions was that the region itself not render when it contains "empty" content.

Determining emptiness is a challenge under the existing Render API implementation.
People expecting a region to dissapear may be surprised to find this feature non-functional.

<!--break-->

@Eclipsegc

Dispite the fact that core community members are very aware of this issue, unfortunately there's really very little to be done about it until we decide to really overhaul this particular subsystem (Render API).
There are potentially a lot of people interested in doing that work, it's just a matter of figuring out the right solution, and building consensus.

The message you should take from this is that core developers are generally very aware of this issue, but it's not a simple as building a fix. It's more like we need to revisit the underlying paradigm that drives this section of Drupal.
It'll happen eventually, and until then, we'll all have to limp around this issue together.

Several approaches to solving this have been considered. All of which come with unacceptable drawbacks.

The core issue is that there is no possible way to definitely identify an 'empty' block without first
rendering it.

This is the standard code for detecting empty regions and results in wrapping divs showing regardless of content existing,

<?php
if ($page['region']) {...
?>

There are a number of symptoms of this issue.

It's a challenge to add class such as "has sidebar" to the body tag when it's impossible to determine if it's empty (there are some is empty() type workarounds in the thread)

Many possible workarounds have been proposed. None of which are suitable for implementation in core.

There are a number of existing child issues about broken layout, and more keep coming.

Considerations

Placeholders

Placeholders only have partially to do with BigPipe - They allow our dynamic page cache to work at all for complex scenarios. It is a pre-requisite for authenticated user caching.

Access

@fabianx

Access is not affected in core right now. If you use block visibility access conditions you are fine => with or without placeholders. (because it happens _before_ any rendering takes place). The only thing that there is, is that there is cache data in the region, so !empty() won't work. A helper function by Wim will filter that there are no renderable children. This needs a helper function, however at least the cacheable meta data needs to be put to the upper level.

We could also for empty regions just remove it and render it on the render stack instead. I think that would be safer, too and is exactly what happens when people using work-arounds to pre-render things.

This would make block visibility condition blocks work nicely - e.g. the user login block and is a really simple fix - just render an empty region before returning.

Empty blocks

@fabianx

Empty blocks happen just after rendering, so we cannot know if the region is empty before rendering it actually. Pre-rendering work-around works, but is ugly especially because it doubles the time to render the region.

Placeholdered blocks

@fabianx

In most cases you are by default not affected by this

Empty blocks happen just after placeholder replacement, with BigPipe even just on the client. => Pre-rendering work-around will not work. A late event subscriber or for BigPipe a JS solution will work.

Accessibility:

@andrewmacpherson

Empty DIVs don't really matter much for a11y, as they don't pollute the semantics at all.

However an empty is a problem for some assistive tech, as it will be included in their "landmark regions" navigation tools. An empty landmark region is potentially confusing. Same goes for header, footer, nav elements.

Proposed resolution

Suggestions

The best solutions that have been suggested over the years this issue has existed just don't get
the job done in an acceptable way.

The ideal result is the ability to add this to a template:

{# D8 #}
{% if region %}
  <div>{{ region }}</div>
{% endif %}

There have been some suggestions of adding twig filters for D8, for example:

{# D8 #}
{% set region = page.region|render %}
{% if region %}
  <div>{{ region }}</div>
{% endif %}

and calling render directly in the twig template:

// D7
if ($region = render($page['region'])):
  <div>$region</div>
endif;

These implementations suffer from the fact that placeholders are not yet replaced in the render array, using
this method will break features such as big pipe.

Design a new architecture for the render system.

It's clear from the existing thread that with the current implementation of the render system that was
introduced with drupal 7 that the issue steps from a fundamental flaw in the render architecture.

@pheneproxima has a good idea:

Render arrays already support a little-known property called #access_callback. If set, it is supposed to contain a callback function (or callable, I believe) which is called before its part of the array is rendered. And if it returns FALSE, that part of the array is skipped by the rendering system.

It would take a pretty large-scale effort across core, but we could take advantage of this property to quickly recurse through a render array and remove parts that should not, according to the access callbacks, be rendered. For example, an empty item_list could have an access callback which returns FALSE if there are no items in the list. A renderable array section that will contain the results of a view could have an access callback which returns FALSE if the view has no results (and there is no empty text). And so forth. Each part of a render array could implement lightweight logic to prevent rendering of that specific part of the array. Basically, we would pre-compute the #access property by recursively calling the lightweight access callbacks.

The downside is that this would still involve recursing through a gigantic renderable array. However...it'd be a great deal faster than rendering out the array and checking if there was any output. Also, we could take advantage of cacheability by returning AccessResult from the access callbacks (if, indeed, that's not what they already return).

Limitations

@markcarver

Simply rendering a variable in Twig does not guarantee the variable will be "empty".

If a render array contains properties for render caching/big pipe/lazy loading, then it can potentially generate placeholder markup.

This placeholder content may or may not be empty, but that will not be determined until it is replaced at a later time in the request... well after said Twig template is invoked, ran through "if empty" statement and subsequently render cached using the incorrect logic.

Known workarounds

// @todo audit the suggested work arounds and update this description.
// There a numerous workarounds and approaches, some general some for specific usecases.

@danbohea

OK, so there's a fourth option for people to consider for a default emptiness definition:

A: Empty means there must be text other than normal whitespace or tags or HTML comments (e.g. render|strip_tags|strip_comments|trim)
B: Empty means that there is no whitespace, but HTML tags or HTML comments are considered non-empty (render|trim)
C: Empty means there is absolutely no content in there not even whitespace (render)
D: (**NEW**) Empty means that there is no whitespace, HTML tags are considered non-empty, HTML comments are ignored and considered empty (I guess that's render|strip_comments|trim ?)
Option D is my preference: it's compatible with Twig debugging and shouldn't accidentally hide non-text HTML (e.g. tags).

assign render output in twig to determine "emptiness"

Add

{% set output = render_var(content) %}

to your twig

Remaining tasks

[ ] Escalate this issue to an plan or initiative
[ ] Audit the workarounds
[ ] Document the known limitations - it doesn't look like this is going away any time soon.

User interface changes

None from an admin perspective.
Sitebuilder's will now have regions with empty blocks not being rendered, as expected.

API changes

[ ] To be determined

Data model changes

// @ todo

Release notes snippet

// @ todo

🌱 Plan
Status

Needs work

Version

10.1

Component
Theme 

Last updated about 24 hours ago

Created by

🇺🇸United States eclipsegc

Live updates comments and jobs are added and updated live.
  • needs profiling

    It may affect performance, and thus requires in-depth technical reviews and profiling.

  • Accessibility

    It affects the ability of people with disabilities or special needs (such as blindness or color-blindness) to use Drupal.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇩🇪Germany sic

    I know this isnt helping but this problem has existed for over 13 years now (!) and still is the reason, besides quite many, we still cannot wholeheartedly start any new Drupal projects. The lack of a fundamental function as to check if a region is empty or not, if a sidebar shall be rendered or not, is a joke in 2023. When I started web development I was nothing short but astounded by this community. So many contributed modules and functions and all of that open source. Where did it go wrong? Look at the current state. Buggy caching, this issue, no proper way of an actually, working migration, no way of editing translated content without your whole backend being in that language, etc etc.

    @Anybody as great and commendable as your effort is, the real problem is that gets replaced with either something empty or not.

  • 🇺🇸United States bayousoft

    @jmcerda this solution works here

  • 🇩🇪Germany sachbearbeiter

    @sic "So many contributed modules and functions and all of that open source. Where did it go wrong? Look at the current state."

    You're romancing a bit here ;) It was never different ... the multilingual system, media management, update system ... all inadequate over long periods of time ...

    @jmcerda Thanks!

  • 🇨🇦Canada mgifford Ottawa, Ontario

    Empty elements can be a problem, looking at this the most likely WCAG SC failure would be tied to links so WCAG 4.1.2 https://www.w3.org/WAI/WCAG22/Understanding/name-role-value

    Looking here;
    https://stackoverflow.com/questions/68281369/does-the-wcag-address-empty...

    Are there chances that titles, labels, headings, tables, table headings, lists or will be empty? If so then there are going to be some other SC which may apply.

  • 🇺🇦Ukraine Brolad

    Drupal 10 compatibility

  • 🇳🇱Netherlands jurriaanroelofs

    I know this isnt helping but this problem has existed for over 13 years now (!) and still is the reason, besides quite many, we still cannot wholeheartedly start any new Drupal projects.

    @sic we have a workaround committed to dxpr_theme that probably works in 99.9% of real-world applications, feel free to use our theme or implement our patch in your custom theme:

    https://git.drupalcode.org/project/dxpr_theme/-/commit/7810b0c78d866e705...

  • 🇩🇪Germany sic

    @JuriaanRoelofs Thanks for the input, but wasnt the original problem that the drupal placeholder gets replaced with something empty or not after the rendering process that we are provided with? How does your patch fix that?

  • 🇫🇷France dqd London | N.Y.C | Paris | Hamburg | Berlin

    I had no time yet to re-read this issue again (last time is some years ago) ...

    ... but does it has been already discussed as possible idea/solution to give regions a default key "used" set to 0 with them by default, which changes its value from 0 to 1 or whatever only if blocks get moved into the region and saved? (This could be extendable from boolean to a list of keys of course, if we want to check for more.)

    It would be an additional key|value set named "used" or "filled" on each region getting filled with 1 if a block gets placed into and only changes back to 0 if no block is in this region.

    In a twig template it would look like this then

    {% if region.used %}
    

    Just a glimpse popping up in my mind...

Production build 0.71.5 2024