Umami theme ignores placeholders and HTML replaced elements when checking for empty regions

This is desirable for accessibility.

Empty DIVs don't really matter much, as they don't pollute the semantics at all.
However an empty <aside> 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.

It took me a while to figure this out, when there is a block in the sidebar that will be replaced with bigpipe, the output is something like:

    <span data-big-pipe-placeholder-id="callback=Drupal%5Cblock%5CBlockViewBuilder%3A%3AlazyBuilder&amp;args%5B0%5D=recipecategory&amp;args%5B1%5D=full&amp;args%5B2%5D&amp;token=pcva2mpXQyxRrGwdCofsGdjk4ljkqESK_R9WGvxjSgQ"></span>

This means that the |striptags|trim makes it empty, but there is a block there.

I'm taking a look at this now at DrupalCon code sprint.

I was looking through https://www.drupal.org/node/953034.

There do not seem to be any solutions for both hiding the markup of a region if it's truly empty and displaying the region if it contains content added by BigPipe.

There was a suggestion to use |render|strip_comments|trim as an improvement. But I don't believe this will solve the problem with content loaded from BigPipe.

Hello,

We faced up with this situation when creating a new role in a demo site. Editor user was not able to view the node tabs because the block was filtered in a very aggressive way.

I fixed it passing some parameters to the striptags call in this patch.

What do you think?

Thank you.

This patch is working as expected in our search demo site. We had similar issues that prevented bigpipe from working correctly.

Please get this committed to umami so we can inherit correctly from umami and use this awesome demo-content for more than 1 purpose :)

Would be great if we could get the exact steps it takes to reproduce this. Also tagging for review from a framework manager since they originally raised this problem.

Sure - here are the steps:

git clone git@github.com:nickveenhof/drupal8-umami-search.git
git checkout 2937640
composer install
mkdir docroot/themes/contrib
cp -R docroot/core/profiles/demo_umami/themes/umami docroot/themes/contrib/
cp -R docroot/core/profiles/demo_umami/modules/demo_umami_content docroot/modules/contrib/
rm .ht.sqlite || :
sudo rm -rf web/sites/default/settings.php
vendor/bin/drush site-install --db-url=sqlite://.ht.sqlite config_installer config_installer_sync_configure_form.sync_directory=../config/sync/ --yes
vendor/bin/drush ev '\Drupal::classResolver()->getInstanceFromDefinition(Drupal\demo_umami_content\InstallHelper::class)->importContent();'
vendor/bin/drush ev '\Drupal::classResolver()->getInstanceFromDefinition(Drupal\demo_umami_search_content\InstallHelper::class)->importContent();'
vendor/bin/drush search-api:reset-tracker
vendor/bin/drush search-api:index
vendor/bin/drush cr

Set $settings['config_readonly'] = FALSE; in your settings.php
Go to http://mysiteijustinstalled/admin/structure/block/manage/type?destinatio...
Change the page restrictions and leave this section empty. So the block could load on all pages

Go to the homepage and you should see the empty region:

You can see in the HTML the following empty region:

<aside class="layout-sidebar" role="complementary">
  <div class="region region-sidebar">
    <div></div>
  </div>
</aside>

Patch now applies cleanly. There was a slight rework to allow for the factor the {% if %} had been extended to include both the header and the preheader region.

Refactored and rerolled.

+++ b/core/profiles/demo_umami/themes/umami/templates/layout/page.html.twig
@@ -44,27 +44,28 @@
+  {% if page.pre_header|render|striptags('<drupal-render-placeholder>')|trim is not empty or
+     page.header|render|striptags('<drupal-render-placeholder>')|trim is not empty %}

I think we need a new twig filter for this, as littering our templates with this makes them harder to maintain, but also will result in more updates if things changes.

Having a filter means we can update it in one place

Thoughts?

If that's not a breaking change to existing themes (I guess it's not as long as it's not added to them)

I'm wondering how would the addition of a new filter break backwards compatibility?

I would prefer adding the filter here, since then we could say that this is the way empty regions are supposed to be checked. Also, fixing any potential side effects of the solution would be easier once we have a centralized place to control that.

I think it would be also useful to get a review from the render API subsystem maintainers.

+++ b/core/profiles/demo_umami/themes/umami/templates/layout/page.html.twig
@@ -44,27 +44,28 @@
-  {% if page.tabs %}
+  {% if page.tabs|render|striptags('<drupal-render-placeholder>')|trim is not empty %}

This is not possible, for complex reasons explained in #953034: [meta] Themes improperly check renderable arrays when determining visibility.

I see that this is explicitly doing a complete render to sort of work around it. But it then still strips placeholder tags, meaning that it may be concluding that something is empty even though it won't be (once the placeholder is rendered).

See #953034-200: [meta] Themes improperly check renderable arrays when determining visibility.

I see now that the IS already mentioned #953034, but it didn't link to it. Hence I didn't notice.

If we just print

{% if page.tabs %}
{{ pagetabs }}
{% endif %}

We get empty regions printed on every page that there is no block in that region for. So on a views page, we'll have <div class="region-tabs"></div>

We should instead ensure that we don't have emptiness-based conditions. It would be okay to check for pre-rendering emptiness (i.e. "this region has no blocks placed in it"), but not to check for post-rendering emptiness (i.e. "this region has >=1 blocks, but maybe all of them render to nothing for this request").

I'm working on this

I tried to rework the page template so that we don't need to do the if checks. Any thoughts on this approach?

So I found a way to do this in twig without the dance around with the |trim|strip|makemeasandwich

{% set some_region %}{% spaceless %}{{ page.some_region }}{%endspaceless %}{% endset %}
{%if some_region %}
  <!-- markup goes here -->
  <div class="some-region">
    {{ some_region }}
  </div>
{% end if %}

Basically, set also is a block, if you combine that with spaceless, you render the element in advance without whitespace.
If it yields nothing, your if fails.
Thoughts?

I updated the demo site as mentioned in comment #16 and this is working as expected. @laurii, what are the next steps we can take to get this committed? I'm reluctant to set this to RTBC as I am not aware enough of the standards this has to meet before we can commit this.

Discussed in person with @laurii at Drupal Dev Days and he is in agreement we can mark this as RTBC. If for whatever reason someone objects, please reopen/comment/add suggestions how to improve.

Re-rolled 2937640-30.patch against core 8.8.x

Twig coding styles look good to me.

So another reason why this is a very good thing to do is that the current approach in HEAD results in the final page having markup like
<div id="block-umami-disclaimer--2" class="block-type-disclaimer-block block block-block-content block-block-content9b4dcd67-99f3-48d0-93c9-2c46648b29de">
instead of
<div id="block-umami-disclaimer" class="block-type-disclaimer-block block block-block-content block-block-content9b4dcd67-99f3-48d0-93c9-2c46648b29de">

This is because the rendering but not using the output causes the \Drupal\Component\Utility\Html::getUniqueId() to add --2 etc to IDs! So this is definitely a good thing to fix.

The resulting fix is changing the HTML in other ways though that we might not want. For example...

Before

<header class="layout-header" role="banner">
      <div class="container">
                    <div class="region region-pre-header">
    <div class="language-switcher-language-url block block-language block-language-blocklanguage-interface" id="block-languageswitcher" role="navigation">

After

<header class="layout-header" role="banner">
      <div class="container">
          <div class="layout-pre-header">
          <div class="region region-pre-header">
        <div class="language-switcher-language-url block block-language block-language-blocklanguage-interface" id="block-languageswitcher" role="navigation">

We've inserted a new div <div class="layout-pre-header"> into the design. I'm not sure that we should be doing that here.

Also the issue summary needs updating with the solution and why it is appropriate for Umami.

I've updated the issue summary because it was difficult to reconcile what was being discussed in the comments and what is done in the patch. There's bits that still need to be updated by someone else if they understand the issue and the patch better.

As for the priority of this bug, I'd like to point out:

That means the current code removes A LOT of valid content. If Umami were a regular theme (and not a demo theme for a specific set of demo content), this would be a major bug.

Having this code in core means developers are definitely copying this out of core and putting it in contrib and custom themes. And THOSE copies are major bugs.

Does that mean this issue is major?

I think this most closely aligns with https://www.w3.org/TR/UNDERSTANDING-WCAG20/content-structure-separation-...

This was partly changed in #3439017: Umami page.tpl.php breaks block placeholders which was similar but I think more problems were identified here.

@berdir just opened #3516523: Umami page template renders header regions multiple times too.