Render a placeholder DIV inside empty Regions/Slots in the preview

@jessebaker reported the same problem ~2 hours later: #3473772: Improve UI for empty states - Empty layout. Closed that as a duplicate.

Transfered what he wrote and merged it with @shyam_bhatt's write-up 👍

Related: #3472299-17: Update default config to make a fresh install result in an XB UI with an empty canvas.

I believe at this point all that is left is the backend work to render the empty space inside the iFrame - basically taking the work done in useRenderPreviewEmpty[Slot|Region]Placeholders and doing it on the server side instead.

BE:Because the placeholder is inserted into the preview iFrame after it has loaded it causes a noticeable layout shift every time the preview re-renders so this needs to be refactored and inserted/rendered on the back end.

What is the concrete before vs after that you're looking for? What markup needs to change? What API response needs to change?

When a request to the back end for the preview HTML is made currently the HTML returned contains HTML comments to denote the location of regions and slots. When a slot or the main content "region" is empty, the design calls for an empty space into which users can drop their first component to start making the page. The space is styled to have either a green or purple border. This border is rendered in the overlay. Inside the iFrame we simply need some blank space.

The blank space is created by inserting a div with a height and width. In head, we find the HTML comments and insert the div on the client side. This leads to a "jump" after the page is rendered as the new divs are inserted. If the placeholder div were inserted server side the jump would not happen.

So, when a request returns the preview HTML (e.g. a POST or PATCH to /xb/api/v0/layout/xb_page/1 I'd like the HTML returned to already have placeholder divs in between the HTML comments.

For empty regions:

In the markup an empty region currently looks like

<!-- xb-region-start-content --><!-- xb-region-end-content -->

I would like for it to look like this instead:

<!-- xb-region-start-content --><div class="xb--region-empty-placeholder"></div><!-- xb-region-end-content -->

As soon as there is anything rendered in the region, the xb--region-empty-placeholder should not be rendered.

For empty slots

In the markup an empty slot currently looks like

<!-- xb-slot-start-d42bf579-4554-48e2-9438-b859e00d29d8/content -->
            <!-- xb-slot-end-d42bf579-4554-48e2-9438-b859e00d29d8/content -->

I would like for it to look like this instead:

<!-- xb-slot-start-d42bf579-4554-48e2-9438-b859e00d29d8/content --><div class="xb--slot-empty-placeholder"></div><!-- xb-slot-end-d42bf579-4554-48e2-9438-b859e00d29d8/content -->

As soon as there is anything rendered in the slot, the xb--slot-empty-placeholder should not be rendered.

In addition - the server currently renders the first of a slot's examples from the SDC's yml into the slot. This shouldn't be happening. Currently the front end has to remove this markup.

slots:
  column_one:
    title: Column One
    description: The contents of the first column.
    examples:
      - <p>This is column 1 content</p> <--- this should not be rendered in XB

On the front end

Once the backend is inserting those DIVs the front end can then remove ui/src/hooks/useRenderPreviewEmptySlotPlaceholders.ts and ui/src/hooks/useRenderPreviewEmptyRegionPlaceholder.ts

Part one: needs more clarity 🙏

For empty regions: …

👎 This will inevitably trigger #953034: [meta] Themes improperly check renderable arrays when determining visibility.

🏓 When does XB render an empty region? Only when it's just had its last component instance removed? Because since #3518606: When a global region is empty, don't render HTML comment annotations into it we specifically don't render these!

Part two: clear! 👍

In addition - the server currently renders the first of a slot's examples from the SDC's yml into the slot. This shouldn't be happening. Currently the front end has to remove this markup.

Hah! Should be an easy fix, and easily added test coverage for: in \Drupal\Tests\experience_builder\Kernel\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponentTest::testGetClientSideInfo() tests with isPreview: FALSE. 👍

Part three: needs more clarity 🙏

For empty slots […]

1. We can do this for SDCs, but then this is effectively blocked on #3499352: SDCs should only have get HTML comments injected when `renderComponent(isPreview: TRUE)` — otherwise we'll inevitably render <!-- xb-slot-start-d42bf579-4554-48e2-9438-b859e00d29d8/content --><div class="xb--slot-empty-placeholder"></div><!-- xb-slot-end-d42bf579-4554-48e2-9438-b859e00d29d8/content --> on the live site, too! 😨
2. 🏓 We can't do that for code components … so how do you propose to tackle this for those? 😅 If they don't need this: why not?
3. And that last point is generalizable: all future \Drupal\experience_builder\ComponentSource\ComponentSourceWithSlotsInterface implementations will need to consistently perform the same.

When does XB render an empty region? Only when it's just had its last component instance removed?

This is a good point - I am specifically talking about the Content "region" when a new page is loaded. Other global regions don't need this.

It currently looks like this:

<!-- xb-region-start-content -->
<!-- xb-region-end-content -->

and I'd like to see this instead:

<!-- xb-region-start-content -->
<div class="xb--region-empty-placeholder"></div>
<!-- xb-region-end-content -->

For empty slots [...]

1. Ah, yes. I agree this would be blocked on that. I didn't realise the HTML comments were not already limited to only the XB preview
2. Code components: We are already inserting the HTML comments around slots for code components. I think determining if they are empty is tricky. Perhaps inserting the placeholder can be done at a similar time in the process to the suggested solution here #3520581: [later phase] Unwrap `<astro-island>`s and `<astro-slot>`s
3. It seems that we need bespoke solutions for both SDC and JS components. I don't know if there is a way to generalise this implementation so that the process can happen no matter the ComponentSource.
   

Ah, if this is a concern for the content region only, then this seems solvable.

1. Totally reasonable to assume that #3499352: SDCs should only have get HTML comments injected when `renderComponent(isPreview: TRUE)` would not have been necessary, I was surprised too 😅
2. Just checking: you do confirm that there's no server-side changes needed/expected, right?
3. I don't think it's generalizable, unfortunately.

Do you think this should be a beta blocker rather than a stable blocker?

Do you think this should be a beta blocker rather than a stable blocker?

Obviously the ramifications are not so serious as data integrity or even severe in terms of user impact - however, we have put a lot of time and energy into creating a 'native feeling' to our editing experience and reducing flicker and FOUC has been a large part of that. The impact is on first impressions and the confidence brought from a feeling of robustness.

Here is a gif showing the current state. For clarity it's only an issue when there is an empty slot on the page. This is an SDC. The effect is the same with a code component. Each time a prop is updated and the preview updates the content below the slot appears to jump up and then pop back down again as the page renders without the space and then the space is inserted.

[edit] The embedded gif is playing at a low frames per second and doesn't seem to show the flicker unless you open it in a new tab.

For now, I'm going to remove stable and beta blocker labels and treat this as a "nice to have" because in light of conversations around other priorities this not critical and can be fixed after beta.

I also think that perhaps this CAN be addressed client side if we tackle it in a different way given that it's non-trivial to handle on the server. It will need some thought but there are options to explore.

#3499352: SDCs should only have get HTML comments injected when `renderComponent(isPreview: TRUE)` went in overnight! Time to re-assess.

Thanks for #18, that's super helpful additional context 🙏

Part one: empty "content" region

This part is trivial to implement, so created an MR for it. If you confirm this does what you want/need, we can merge this right away :)

It currently looks like this:

<!-- xb-region-start-content -->
<!-- xb-region-end-content -->

and I'd like to see this instead:

<!-- xb-region-start-content -->
<div class="xb--region-empty-placeholder"></div>
<!-- xb-region-end-content -->

Part two: empty slots

This actually should be fairly doable too:

1. \Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated::renderify() knows when it's rendering a preview ($isPreview === TRUE), and so it can do something different (like inserting that div) when rendering a preview
2. which gives us two choices:
   1. either comparing the value of a slot with its example value/default, and if that exists, replace it
   2. or updating GeneratedFieldExplicitInputUxComponentSourceBase::hydrateComponent() to also receive a $isPreview parameter, and letting it omit slot examples — it currently looks like this:
      

        public function hydrateComponent(array $explicit_input): array {
          $hydrated[self::EXPLICIT_INPUT_NAME] = $explicit_input['resolved'];
      
          if ($slots = $this->getSlotDefinitions()) {
            // Use the first example defined in SDC metadata, if it exists. Otherwise,
            // fall back to `"#plain_text => ''`, which is accepted by SDC's rendering
            // logic but still results in an empty slot.
            // @see https://www.drupal.org/node/3391702
            // @see \Drupal\Core\Render\Element\ComponentElement::generateComponentTemplate()
            $hydrated['slots'] = array_map(fn($slot) => $slot['examples'][0] ?? '', $slots);
          }
      
          return $hydrated;
        }
      

   I think the first choice is preferable because it keeps the component sources simpler, and hence more reliable, which is important for long-term DX/ecosystem of XB. I'll get a sample MR for that up too.

   That still won't solve it for code components (as described in #15.three.2) though (the server side can't change that), but this should leave it in a much better spot at least :)

I don't really understand how but, @wim leers, your change in !1055 to add the slot placeholder on the server side is working for both SDC and Code Components. I've tested and double checked and the div rendered in ComponentTreeHydrated.php is showing for both types of component.

Those two MR's combined have 100% resolved this issue as far as I am concerned. I've removed the now redundant JS (and 1 other function I found that was already redundant from function-utils.ts (!1055 diff).

🥳

Yes, I realized actually while writing that code that due to slot population even for code components happening server-side (see \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::setSlots()), my statements about that have been wrong all along! 🙈

The 3473761-empty-slot MR has phpunit test failures I will look into.

I re-running the failed e2e tests on the other MR

@tedbow are you still actively working on the failing phpunit tests for 3473761-empty-slot MR?

Both MRs are now passing tests and ready for review.

Back-end changes look good

Given FE lead @jessebaker made the FE changes, going ahead here 🚢

Both MRs need to get upstream changes merged in 🙏

Both MRs have been rebased.

Thanks! First one is in. Will merge the second after a new CI pipeline passes.

After merging the first one there was a conflict in the remaining MR, I have rebased that one again.

Thanks!