Implement HTML comment annotations for Regions, replace HTML wrappers

✨ Leverage HTML comment annotations, remove wrapping div elements Active and ✨ Focus mode for global regions Active should land first.

✨ Leverage HTML comment annotations, remove wrapping div elements Active is in!

I have added a number of todos to the codebase in my work on [#34997648] - whoever works on this should search for #3499364 in the React code. They are places where I've needed to add work arounds for the HTML wrapper being different for regions that are not the content region.

This blocks 🐛 Global region focus spotlight is inaccurate sometimes Active .

Here's the BE pieces — a tiny diff:

diff --git a/src/Controller/ApiLayoutController.php b/src/Controller/ApiLayoutController.php
index 21d1bca7b..7907200ed 100644
--- a/src/Controller/ApiLayoutController.php
+++ b/src/Controller/ApiLayoutController.php
@@ -12,6 +12,7 @@ use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Entity\FieldableEntityInterface;
 use Drupal\Core\Form\FormBuilderInterface;
 use Drupal\Core\Render\Element;
+use Drupal\Core\Render\Markup;
 use Drupal\Core\Theme\ThemeManagerInterface;
 use Drupal\experience_builder\AutoSave\AutoSaveManager;
 use Drupal\experience_builder\ClientDataToEntityConverter;
@@ -388,9 +389,9 @@ final class ApiLayoutController {
     if (isset($renderable[ComponentTreeStructure::ROOT_UUID])) {
       $build = $renderable[ComponentTreeStructure::ROOT_UUID];
     }
-    // @todo Remove/replace this in https://www.drupal.org/project/experience_builder/issues/3499364
-    $build['#prefix'] = '<div data-xb-uuid="content" data-xb-region="content">';
-    $build['#suffix'] = '</div>';
+
+    $build['#prefix'] = Markup::create('<!-- xb-region-start-content -->');
+    $build['#suffix'] = Markup::create('<!-- xb-region-end-content -->');
     $build['#attached']['library'][] = 'experience_builder/preview';
     return $build;
   }
diff --git a/src/Render/MainContent/XBPreviewRenderer.php b/src/Render/MainContent/XBPreviewRenderer.php
index 2941a39ac..6cb93a441 100644
--- a/src/Render/MainContent/XBPreviewRenderer.php
+++ b/src/Render/MainContent/XBPreviewRenderer.php
@@ -11,6 +11,7 @@ use Drupal\Core\Render\AttachmentsInterface;
 use Drupal\Core\Render\AttachmentsResponseProcessorInterface;
 use Drupal\Core\Render\Element;
 use Drupal\Core\Render\MainContent\HtmlRenderer;
+use Drupal\Core\Render\Markup;
 use Drupal\Core\Render\RenderCacheInterface;
 use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\Routing\RouteMatchInterface;
@@ -86,9 +87,8 @@ final class XBPreviewRenderer extends HtmlRenderer {
       if ($region === XbPageVariant::MAIN_CONTENT_REGION) {
         continue;
       }
-      // @todo Remove/replace this in https://www.drupal.org/project/experience_builder/issues/3499364
-      $page[$region]['#prefix'] = '<div data-xb-region="' . $region . '" data-xb-uuid="' . $region . '">';
-      $page[$region]['#suffix'] = '</div>';
+      $page[$region]['#prefix'] = Markup::create("<!-- xb-region-start-$region -->");
+      $page[$region]['#suffix'] = Markup::create("<!-- xb-region-end-$region -->");
     }
     return [$page, $title];
   }

Testing this diff shows it to be working:

FYI: next steps here involves updating the front end pieces as described in the issue summary and making sure 100% of the occurrences of the string data-xb-region are eliminated from the codebase.

This will improve the accuracy of the preview, because the markup structure would no longer be changed. The subtle difference in markup structure can currently cause CSS to not apply, resulting in inaccurate previews!

Yep. See #3507631-11: Page mangled and warning from block components when enabling for global template → for an example.

wim leers → credited larowlan → .

Crediting @effulgentsia and linking for #11, and @larowlan for creating that issue — yay for one fewer bug :D This already is , bumping to and tagging because without this, we won't have reliable previews in XB.

@wimleers

So in my MR I am using the HTML comments to create a map of the region id to it's corresponding <div region region--{regionId}> from the markup returned from the backend.

But when in Olivero, neither <div region region--primary-menu/> nor <div region region--header/> exist in the markup returned by the backend. But it exists for the rest of the regions.

And in Claro, the markup does contain <div region region--header/>

Is this a known bug that the header and primary menu don't come with the

Another thing I noticed is the comment

is after

Is this a known bug that the header and primary menu don't come with the

wrapping element in Olivero? Or is it omitted on purpose for a specific reason?

It's due to some of Olivero's region--*.html.twig files adding a wrapping <div> and some not. I don't know if the ones without a wrapping div had a reason to be like that, but I think in general, whether to add wrapping elements or not is a per-theme decision so we can't rely on themes to follow a single root element for each region rule.

It makes it difficult do the "focus mode" on the header if the header has more than one component since there's no wrapping div I can target.

Is this a unique challenge for regions, or do we run into a similar issue (maybe not "focus mode" specifically but similar issues with selection) for components? We similarly can't assume that components follow a single root element for each component rule. How did we solve it there?

Components don't need to be wrapped by a single element. If there are multiple elements between the start and end comments then the map stores all those elements as an array and the component overlay calculates the total 'area' or size of all those components when drawing the box around them.

Slots on the other hand assume that the comments sit inside the element that is the slot and so look up the tree to find the comments' parent and add that to the map.

I had assumed that regions would behave like slots, but it sounds like a region is more similar to a component that has a slot?

#16:

Another thing I noticed is the comment […]

See the screenshot in #9, it shows that the wrapping is correct? 🤔

The content region is special though, so can you please confirm that the problem only occurs for that region? If so, I'd be happy to look into it!

#18:

I had assumed that regions would behave like slots, but it sounds like a region is more similar to a component that has a slot?

Basically: assume nothing 😅 Theme developers have full freedom to just "print the variable" that contains the region's contents, as described by @effulgentsia in #17.

A huge roadblock with replacing the div wrappers around the region with the HTML comments is that there is no way to initialize SortableJS (drag and drop functionality) for a region without a parent element aka if a twig template only prints out {{content}} or it has no root parent element.

This is because SortableJS requires a parent element to be passed in.

@jessebaker and I talked about this and he said that 📌 Investigate drag-and-drop solution that removes the need to drop items into the preview iFrame Active should unblock this so therefore marking this issue as postponed.

As @hooroomoo explained above, there is currently a blocker on implementing what we want on the front end because of the flexible (read: anything goes) nature of how region templates can/have been built.

I believe there are two steps required to get this working and to meaningfully improve upon what we already have in 0.x and meet the product requirements.

1) The HTML comments for regions need more granularity. So far, the wrapping div has just been replaced by HTML comments. However, the removal of the wrapping div (while that solves some problems) has introduced more problems ("SortableJS requires a parent element" - #20). I propose two sets of comments are introduced. One surrounding the entire template markup and another that just wraps the {{ content}} part.

2) This set of two different comments is needed to give the FE enough information to build/place "dropzones" in the new drag and drop overlay proposed in 📌 Investigate drag-and-drop solution that removes the need to drop items into the preview iFrame Active .

The slight complication is that while the BE work to replace the wrapper div with comments can be done immediately, it cannot be merged until the front end work to create the new overlay is complete.

Examples

Hopefully these examples make my proposed solution clearer:

Example 1

Olivero's region--footer-top.html.twig before

{% if content %}
  <div{{ attributes.addClass(classes) }}>
    <div class="region--{{ region }}__inner">
      {{ content }}
    </div>
  </div>
{% endif %}

Olivero's region--footer-top.html.twig with comments:

<!--xb-region-start-footer-top -->
{% if content %}
  <div{{ attributes.addClass(classes) }}>
    <div class="region--{{ region }}__inner">
      <!--xb-region-content-start-footer-top -->
        {{ content }}
      <!--xb-region-content-end-footer-top -->
    </div>
  </div>
{% endif %}
<!--xb-region-start-footer-end -->

Example 2

Olivero's region--primary-menu.html.twig before (it really is just one line)

  {{ content }}

Olivero's region--primary-menu.html.twig with comments:

<!--xb-region-start-footer-top -->
<!--xb-region-content-start-footer-top -->
  {{ content }}
<!--xb-region-content-end-footer-top -->
<!--xb-region-start-footer-end -->

That's possible if the following is okay:

I believe this screenshot illustrates that the attached patch implements everything in #23, would be good to get confirmed:

(Inserted the image in the issue summary instead of the comment 😅)

This all looks good and I think the .patch in #25 is working as I expect!

The intention on the front end then, once the drag/drop overlay ( 📌 Investigate drag-and-drop solution that removes the need to drop items into the preview iFrame Active ) is ready, is too use the outer comments to draw the region spotlight showing the correct shape/size of the region and then use the inner comments to create the "dropzone" into which components can be dropped.

One thing of note is that in the empty content footer example (where there are content-start and content-end comments) - the front end will possibly need to handle this situation as a special case (when outer comments are found but inner ones are not) and fall back to using the outer comments as the "dropzone".