Global regions containing only "dynamically missing blocks" (due to emptiness or access) cause malformed pages

Will this get solved by #3491021: Leverage HTML comment annotations, remove wrapping div elements or is there a different cause here for an extraneous HTML element existing that CSS is targeting?

Even if the class and CSS is coming from Olivero, why would an empty region be rendered differently in the preview than how it's rendered for real on the site, other than due to #3 or some other bug with what the preview inserts that affects how emptiness is determined?

AFAICT this is completely unrelated to Olivero's CSS or even any generated markup: it appears to be entirely caused by the overlay that was introduced in #3475759: Implement a different approach to the iFrame preview interactions:

IOW: AFAICT this critical UX bug surfaced by the recently added PageTemplate editing (#3494114: Implement auto-save of the page template config entity) support can be traced back to the overlay not computing height correctly. @jessebaker, Can you investigate? 🙏

I'm struggling to reproduce this.

I created a new node and added an SDC (hero) to the Content region.

In the iFrame there is a just one div.region that contains my hero SDC. There are no empty div.region elements. Any region that does not contain a component is not rendered at all.

Looking at your screenshot @larowlan it looks like you have selected a .xb--sortable-item. That div should be wrapping a component which would suggest that the region you are looking at is not in fact empty.

Looking at your issue @wim leers if you can reproduce it can you confirm that the div.xb--overlay-region__higlighted div has a height that is different to the height of the div.region inside the iFrame? (also looking at the markup in your screenshot I can see inside the region there are multiple div.componentOverlay elements which would suggest that your highlight region is not empty either.

Can either of you share a screenshot of the left hand layers panel when this issue is occuring - I'd like to see what components that UI thinks are inside the highlight region.

Are you sure you performed this step, @jessebaker?

* Visit theme settings and enable XB for global regions

Yeah I have the regions all showing up and working in the layers panel - but they are all empty.

In your screenshot (although it's mostly cropped off) it looks like you have a number of components inside several of your regions.

In your screenshot (although it's mostly cropped off) it looks like you have a number of components inside several of your regions.

Ah, you're running into this other bug: #3497744: If an autosave entry exists before enabling global regions for a theme, theme regions cannot be seen. Either wipe the autosave or reinstall Drupal+XB to bypass that bug for now.

Ah, that was it, I can now replicate. Thanks, I'll take a look

I think there are multiple factors at play here.

1. Each component is wrapped in a div.xb--sortable-item even if it has CSS to make it display: none. This means it is receiving the extra margin-block-end that @larowlan mentioned in the issue summary. I believe this will be solve once things are wrapped with HTML comments instead of divs.
2. Each region is wrapped in a div.xb--sortable-list. I'm not sure what the intent of that is at the moment, but in future you will only ever be dragging/dropping into one "focused" region at a time. #3497648: Focus mode for global regions

I have just checked on my branch where the wrapper divs have been replaced with HTML comments the spacing appears the same in XB as it does looking at the front end of the same node as a logged out user:

I think that means, this is not an XB issue, but perhaps an issue with the theme?

#13.1: Great!

#13.2: That is added by\Drupal\experience_builder\Render\MainContent\XBPreviewRenderer::prepare(), introduced in #3489899: Add support for global regions. Are you saying that's not necessary? 🤔

That screenshot of the actual frontpage looks wildly broken too — it should look like this:

— i.e. far less whitespace between the header and the content.

That suggests this is caused by something in XB, that even runs outside of the XB preview! 😱😬

Investigating…

Explanation found: this happens not when just XB is installed (left), but after you enabled using XB's PageTemplate for the current theme (right):

Now working to determine the root cause.

This is caused by the \Drupal\experience_builder\Plugin\DisplayVariant\PageTemplateDisplayVariant::build() logic that I introduced in #3485917: `ComponentTreeMeetRequirements` constraint validator must be updated now that Blocks-as-Components are supported + make PageTemplate render main content!.

Unlike BlockPageVariant, it does not use \Drupal\block\BlockRepository::getVisibleBlocksPerRegion(). Which means that it renders all placed Block-sourced XB Components, always.

This is basically caused by this @todo not being addressed yet:

// @todo access checking and everything in \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray::onBuildRender

This issue is not about access checking. Nor is it about context support — that will be handled in #3485502: [needs design] Handle block contexts (aka implicit inputs vs `settings` aka explicit inputs) — note that the only reason that blocks like LocalTasksBlock etc work is that they don't rely on Context plugins, but on dependency injection (services).

It is about handling a block being empty.

This is ridiculously hard to debug because Xdebug was crashing 🥲 (PHP 8.3.8, Xdebug 3.3.0alpha3), and even re-executing the same code endlessly 🤯 I'd upgrade to PHP 8.4, but Xdebug is not yet available for it. Upgrading to Xdebug 3.4.1 (stable!) did not fix it.

I'm currently updating to the latest PHP 8.3 to hopefully get Xdebug to work again…

Got it working 😬 After painful debugging (still crashing, I had to debug it another way), figuring out how to write the test coverage was also painful due to layers of magic in the Block module. 🪄

Result

Before

After

👆 The MR fixes the problem I described in #15 + #16, which explains why @jessebaker in #13 thought that the XB preview actually did match what was on the front end!

Next steps

#13.2: removing those means that none of the Component instances in any of the regions other than the special "content" region work. So, that's necessary.

While I could argue that the non-zero height for all of the regions that contain only empty-for-the-current-context is the remaining cause of the "after" screenshot above showing it's still not working. And there's not a clear answer for it either: Layout Builder struggled with that too, plus the XB designs do not account for this at all. So, created #3497990: [later phase] Support Block's PreviewAwarePluginInterface (and similar for other component sources), to allow this to land ASAP.

🏓 to Lee

Also: fails on D10 only because the Local Actions block is not fully validatable there AFAICT. IMHO we should spend zero minutes on that and just exempt that test from D10; XB should be able to require D11 long before a stable release anyway.

I haven't reviewed this MR so don't know it's full scope or how close it is to landing. In case it helps with landing all of or parts of this, or descoping all of or parts of this, I just want to add some background that our long-term goal (actually, short-term goal, but possibly not in time for 0.2.0) is for regions without any blocks or components in them to not be shown in either the canvas or the layers panel. Instead, the only way to move a block into a region that doesn't already have blocks in it would be via a right-click. However, since that might not be feasible in time for 0.2.0, the MR here might still be a good improvement worth merging. Also, what I just wrote is just about regions without any blocks/components in them; that wouldn't address issues with regions that contain blocks with empty content.

This issue title is misleading — let's break it down:

1. ✅ statically empty global regions — this is what you're referring to in #22. Those actually already do not show up! In fact — you can't even drag-and-drop a component instance into an empty region in the preview, only in the Layers panel! 😄👍
2. ❌ dynamically empty global regions: regions that have components in them, but those components are not visible. For example: the higlighted region in the #19 screenshot: all 4 of those Block-sourced Component instances render to the empty string, in this context (i.e. on the previewed route)
3. ❌ dynamically empty global regions: regions that have components in them, but those components are not accessible — not solved here, but will have to be solved eventually

The last 2 of those situations can lead to the visual symptoms seen in the issue summary and #19.

The first one already works fine, proof:

After having written #23, I figured I might as well … just add \Drupal\Core\Block\BlockPluginInterface::access() support.

It turned out to be trivial to add failing test coverage for that (https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...), as well as to fix it (https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...) 🥳

So I wrote in #23, but now that is solved!

Overhauled the issue summary. Moving the bits @larowlan wrote about the problems in the preview to #3497990: [later phase] Support Block's PreviewAwarePluginInterface (and similar for other component sources).

IMHO it's very much worth it to land this first — it's a simple but essential part.

Fascinating, that last failure!

PropSourceEndpointTest fails because the expected cache tag config:system.site is now missing. Root cause: block access. Because the /xb-components controller that generates the list of available Components on the site also renders a preview, for use upon hovering a Component.

Turns out the user_login_block happens to be inaccessible to authenticated users! Fortunately, that's easily fixed: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...

Ready for review.

• @larowlan (overall)
• @f.mazeikis (ComponentTest only)
• @tedbow (ComponentTreeHydrated only)

One nit to appease PHPStan but otherwise this looks good to me.

Thanks for that PHPStan improvement!

Double approval by 2 Drupal core committers + #3455753: Milestone 0.2.0: Early preview deadline is near ⇒ not waiting for approval of other code owners for trivial changes in their areas.