Placeholders and real render, in Canvas and Preview

Thanks for taking care of this.

A better placeholder is a consolation prize;

To be able to finish #3542796: Preview of view sources we need placeholders with the name of the source in the label, instead of '[Placeholder] No preview'.

Such huge MR is putting an undue burden on the reviewer. Let's try to keep topics well defined and well scoped.

Placeholders scope

Code review

I don't say this is necessarily wrong, maybe the work done here is broader than my initial understanding, but I was naively expecting a change covering only:

• PreviewPanel with the replacement of $this->buildPlaceholder($this->t('[Placeholder] No preview') by a dynamic value with the label of the source plugin, and the removal of alterPreviewPlaceholder()
• maybe similar changes BuilderPanel
• and not much more

So, I am surprised this change is needing so much additions:

• DisplayBuilderHelpers::markSampleEntity(), used in EntityView and Instance.
• RegionPlaceholderSourceTrait with ::buildBlockRegion() ⚠️ The code, like other parts of the module, is using both "region" and "slot" terms. Not a big deal, but we may talk about terminology in a future ticket

Specific to blocks:

• Override of BlockSource from UI Patterns. ⚠️ Those source overrides are not mean to stay so can we avoid adding more? ⚠️ Why are we manipulating context_requirements (soon to be deprecated) here?
• BlockRegionPlaceholderTrait used in BlockPlaceholderTrait and BlockSource
• BlockPlaceholderTrait used in PreviewPanel and BuilderPanel

Specific to display_builder_page_layout:

• PageRegionSourceBase using RegionPlaceholderSourceTrait and extended by MainPageContentSource and PageTitleSource

I need to continue the review, but it is a bit intimidating.

getPropValue() outside of Display Builder

⚠️ PageRegionSourceBase::getPropValue() and ViewsUiPatternsSourceBase::getPropValue() directly return a placeholder instead of the expected value. I guess they are replaced later by the real value if possible.

However, once the UI Patterns contexts will be tidied, some of those source plugins will show up outside of Display Builder, in the broader UI Patterns ecosystem. Will they work as expected?

Feature review

Just starting:

• ⚠️ On Page Layout, the footer menu block is now displayed with a placeholder ("Filled by the System module when this display runs on a real page."), it was rendered normally before.
• ... in progress ...

#3542796: Preview of view sources scope

The other MR content has been put here with some modifications. There may be a reason, but splitting the works (this ticket only for placeholders fix, the other ticket for the source previews), and validating changes one by one is usually helpful to avoid taking shortcuts.

With those main changes:

• views_pre_execute hook in CountViewExecutions
• ViewsUiPatternsSourceBase is now using RegionPlaceholderSourceTrait

Review in progress.

Third scope?

Are those changes related to placeholders? Part of #3615194: Dynamic rules for wrapping in PreviewPanel? Or something else?

• Implementations of DisplayBuildableInterface::getPreviewPagePath() in EntityViewOverride and ViewDisplay
• PreviewRenderCache service decorating render_cache ⚠️ active on every request when the module is activated. Is it not too "bold"? Do we need specific tests?
• DisplayBuilderHelpers::previewedInstanceId(), used in DenyPreviewSubRequest, DisplayBuildablePluginBase, ApiPreviewController and PreviewRenderCache
• DisplayBuildableInterface::getSourcesForRender(), implemented in getSourcesForRender() used instead of ::getSources(), once of each module: in EntityViewDisplayTrait, PageLayoutPageVariant and PreprocessViewsView

Got the complaint on the size, will split the MR in 3 to ease review.

Thanks you 🤗

Thanks for the split, there are now +1659/−311 lines to review instead of +3347/-467 👍

Note to myself: The reviews must be done following this order:

1. #3618072: Placeholders and real render, in Canvas and Preview
2. #3542796: Preview of view sources, because the MR is rebasing the previous one
3. #3618618: Preview an entity view override and a view page display on their own page, because the MR is rebasing the previous one

So starting with this one.

First feedback: just thinking out loud, in the flow of the review, I may write silly things here. I keep the ticket on my side until the review is finished.

Placeholders directly managed by sources

What stays here: a placeholder is named by whoever knows best. A source that can never resolve in a builder answers for itself at any depth

That sounds cool. I will try to write here my understanding, just to be sure.

There was 2 different placeholders logic in Display Bucodeilder:

• in BuilderPanel::buildSingleBlock(), we render a placeholder:
  • for every source rendered as empty or with failed rendering
  • and always for system_messages_block and local_tasks_block block plugins
• in PreviewPanel::alterPreviewPlaceholder(), we render a placeholder only (and always) for a curated selection of sources:
  • some blocks: local_tasks_block & system_messages_block
  • from display_builder_page_layout: main_page_content & page_title
  • and some from display_builder_page_views, but it is a temporary situation until #3542796: Preview of view sources

So, by introducing RegionPlaceholderSourceTrait::buildRegionPlaceholder(), we moved the management of placeholders to the source plugins themselves:

• BlockSource with FORCE_PLACEHOLDER
• MainPageContentSource & PageTitleSource (through PageRegionSourceBase)

That's cool. ⚠️ It was initially a bit complicated to understand because of the use of the word "region" here.

💡 By the way, does PreviewFallbackInterface::getPreviewFallbackString() (implemented in Core Block plugins and used in Layout Builder) would help us here?

Follow-up for the footer menu: an empty block nested in a slot gets a placeholder in Canvas and nothing in Preview, because Preview only judges root nodes. Separate issue.

⚠️ It may be bigger than that. I will check again.

DisplayBuilderHelpers::markSampleEntity()

Used in EntityView::getRuntimeContexts() and Instance::refreshContexts(), that's a nice idea to reuse in_preview from Core:

      // Undeclared on purpose, by core: `in_preview` is a plain dynamic
      // property that NodeForm and CommentForm set the same way.
      $entity->in_preview = TRUE;

Even if I don't really know what Core is doing with this information :)

⚠️ It is not directly related to the current work, but we are altering the logic here, so I am asking: Why do we have Instance::refreshContexts() by the way? Is it a leftover of previous architecture (before the buildable plugins) ?

There is nothing related to, or own by, the instance data in this method. It seems the logic can move to EntityView, alongside ::markSampleEntity().

IslandPluginManagerInterface::BUILDER_SURFACE

context_requirements in BlockSource is read, not manipulated: inBuilderSurface() checks whether RequirementsContext carries BUILDER_SURFACE. It needs a migration answer when contexts are tidied, but it adds no new write.

Indeed. ⚠️ I will have a little look.

💡 By the way, just saying, there is also PreviewAwarePluginInterface for plugins, which seems to cover similar needs in Layout Builder.

Placeholders from ::getPropValue()

PageRegionSourceBase::getPropValue() returning a placeholder outside Display Builder is a real gap. ViewsUiPatternsSourceBase guards it properly, page layout does not.

If it is OK for ViewsUiPatternsSourceBase, I belive we are good for now, because the recent addition of page context (type: url), in PageLayoutSource, MainPageContentSource and PageTitleSource, will act as a safeguard once context_requirements: ['page'] will be "disabled" (and later removed) in UI Patterns side.

Instead of being context free and show up on every source selectors, those source plugins will be available nowhere because page context (type: url) is never injected in UI Patterns.

Placeholders directly managed by sources

Tested with Page Layout in a fresh install and UI Suite DaisyUI:

It seems"[Page] Title" is the problematic one here, for 2 reasons:

• it is rendered differently in Canvas/Builder while being managed by RegionPlaceholderSourceTrait
• if I move the first "[Page] Title" outside the "Navbar" component, it is rendered OK in Preview
• if I move the second "[Page] Title" inside the "Grid 1 region" component, the component is now rendered as a placeholder and its content hidden inside this placeholder. See next screenshots.

This is not related with having 2 "[Page] Title" in a display. I have similar results with a single one .

Do you have similar results?

IslandPluginManagerInterface::BUILDER_SURFACE

Still investigating.

Placeholders directly managed by sources

✅ "[Page] Title" is fixed. Thanks.

Then for 'Main navigation', this is probably the 'empty menu' issue that will need a follow up. Until then add at least one link in each used menus.

It may be more than an empty menu issue. Let's also try with 2 other sources, all rendering empty markup:

• Site branding block with all options unchecked
• Empty footer menu
• SDC with empty template.

Results:

3 problems:

• Why do they have placeholders in Preview instead of being hidden? Contrary to the ones managed by BlockSource and PageRegionSourceBase, those 3 renderables will not magically be rendered once they get "real" display context.
• In Canvas, the wording of the SDC ("Configure it to make it visible") is OK, but the wording of the blocks is misleading. They will not be rendered in a "real page", but according to configuration and retrieved data.
• SDC placeholder looks different (sl-card.db-placeholder-button instead of div.db-placeholder-region)

So, we may have a wording issue here. .db-placeholder__region-help is currently displaying:

Filled by the title of whatever page is being viewed.
Filled by the display of whatever page is being viewed.
Filled by the @module module when this display runs on a real page

Which are not always describing correctly what is happening and may be confusing (Will users guess they are placeholder? Care about the Drupal module name?).

I am not a wording expert, but we can try something a bit like that:

What do you think?

IslandPluginManagerInterface::BUILDER_SURFACE

context_requirements in BlockSource is read, not manipulated: inBuilderSurface() checks whether RequirementsContext carries BUILDER_SURFACE. It needs a migration answer when contexts are tidied, but it adds no new write.

The goal would be to not increase our usage of context_requirements which will be deprecated and removed. Moreover, the current proposal is extending this usage to a new plugin type: Display Builder's Islands, while it was used only with UI Patterns' sources until now.

The goal is to tell BlockSource it is in a visual builder instead of the "real" display, that's it?

Maybe it is the opportunity of using PreviewAwarePluginInterface already discussed in this thread. So instead of using context requirements, can we do something like that?

In src/Island/RealRenderTrait.php:

   protected function buildComponentRealRender(InstanceInterface $instance, string $node_id, SourceWithSlotsInterface $source, array $data, string $component_id, string $label, int $index): array {
+    if ($source instanceof \Drupal\Core\Plugin\PreviewAwarePluginInterface) {
+      $source->setInPreview(TRUE);
+    }

In src/Plugin/UiPatterns/Source/BlockSource.php:

-class BlockSource extends UiPatternsBlockSource {
+class BlockSource extends UiPatternsBlockSource implements \Drupal\Core\Plugin\PreviewAwarePluginInterface {
 
   use BlockRegionPlaceholderTrait;
   use RegionPlaceholderSourceTrait;
@@ -62,6 +63,13 @@ class BlockSource extends UiPatternsBlockSource {
     'help_block',
   ];
 
+  /**
+   * Whether the plugin is being rendered in preview mode.
+   *
+   * @var bool
+   */
+  protected $inPreview = FALSE;
+
   /**
    * {@inheritdoc}
    */
@@ -89,13 +97,20 @@ class BlockSource extends UiPatternsBlockSource {
     // exactly what the visitor should get. That includes a display previewed
     // on its own page: the sub-request runs the real page pipeline, and its
     // real messages and tabs are the point of previewing it that way.
-    if (!$this->inBuilderSurface()) {
+    if (!$this->inPreview) {
       return parent::getPropValue();
     }
 
     return $this->buildBlockRegion($plugin_id) ?? parent::getPropValue();
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function setInPreview(bool $in_preview): void {
+    $this->inPreview = $in_preview;
+  }
+

Placeholders directly managed by sources

Placeholders are fixed,

Nice. Tested with MR !357 and the UI Patterns patch, it looks good. ✅

In my opinion, this part is complete and OK 🎉. (and its implementation looks slicker in MR !357, that's great)

IslandPluginManagerInterface::BUILDER_SURFACE

PreviewAwarePluginInterface proposal, I think this would be way better in ui_patterns no?

Good idea. Stuff related to source plugins would be better on UI Patterns side, so it will be handled easier when we will tidy the source system, that's why we have this issue: #3572186: Use SourceWithSlotsInterface from UI Patterns

Here is a proposition of UI Patterns patch and a new branch here to review with this patch: 3618072-preview-aware-sources (failing ci is from missing patch ui_patterns, and views failure on playwright would be fixed in the views placholders mr).

Thanks a lot. The new MR looks great and the patch is interesting.

Does that means every SourceWithSlotsInterface implementation (the existing PageLayoutSource, the upcoming #3591025: Add a ViewTemplateSource for slots, and any potential implementations: paragraphs, config components, BEF...) will need to manually pass the #in_preview render property to the children renderable like that?

• LayoutSource (from the MR): $this->componentElementBuilder->buildSource(['#in_preview' => $this->inPreview], 'content', [], ...) ?? [];
• ComponentSource (from the UIP patch): '#in_preview' => $this->inPreview,

Is there a way to automate this somewhere? In ComponentElementBuilder::buildSource()? If we find the good way of doing that, merging this to UI Patterns will be my priority so we are not blocked.

(Also, if we inject a non empty array with #in_preview property to ComponentElementBuilder::buildSource()'s $build parameter and the source is not valid/found, the method will return a non-empty array, and i don't know if it will mess with some later checks)

DisplayBuilderHelpers::markSampleEntity()

I will address the Instance::refreshContexts() topic in a dedicated follow-up ticket: #3618945: Tidy Instance methods. So we can focus on PreviewAwarePluginInterface and merge sooner.

$contexts is already automatic: every SourceWithSlotsInterface threads it to its children anyway, so a new implementation propagates this for free. That puts the automation where you asked, in buildSource(), reading a plain Context rather than a RequirementsContext.

So, if my understanding is right, in this updated proposal:

1. In MR !357: we inject a ui_patterns:in_preview boolean context in every islands from IslandPluginManager::createInstance()
2. In RealRenderTrait, island contexts are already passed to ComponentElementBuilder::buildSource()
3. In the UI Patterns patch: ComponentElementBuilder is setting PreviewAwarePluginInterface::setInPreview() according to context
4. In MR !357: BlockSource implements PreviewAwarePluginInterface and can execute its logic.

I would prefer us to not use Drupal Context like that, because it will be a specificity to manage during our current efforts to tidy contexts for source plugins ([3540247], #3608162: Tidy source contexts...) but a "normal" boolean context is already better than using the custom RequirementsContext, so it may be OK like that.

I will work today on #3576385: Make sources rendering easier on UI Pattern side by adding:

• a $preview boolean parameter to SourceTreeRendererInterface::buildSource()
• the PreviewAwarePluginInterface logic

I tell you once its done.

Are you OK with this https://git.drupalcode.org/project/ui_patterns/-/merge_requests/498/diffs ?

With this, in RealRenderTrait::renderSource(), we can replace:

$build = $this->componentElementBuilder->buildSource([], 'content', [], $data, $this->configuration['contexts'] ?? []) ?? [];

by the same method with the new $in_preview boolean set as TRUE:

$build = $this->componentElementBuilder->buildSource([], 'content', [], $data, $this->configuration['contexts'] ?? [], TRUE) ?? [];

or by the new ::buildSources() (may returns a list of renderables):

$build = $this->componentElementBuilder->buildSources($data, $this->configuration['contexts'] ?? [], TRUE);

Are we still propagating as expected with this solution? Or do we need a context to propagate the way we want?

@just_like_good_vibes is currently reviewing and updating #3576385: Make sources rendering easier but we wait your agreement to merge.

On UI Patterns side, we moved to a more specific issue: #3619208: Manage PreviewAwarePluginInterface in ComponentElementBuilder (because #3576385: Make sources rendering easier was raising other points of discussions).

So, I guess we are back to your patch, without the $preview parameter. Let's talk at the weekly.

RTBC as soon as #3619208: Manage PreviewAwarePluginInterface in ComponentElementBuilder is merged on UIP side.

For information.

In #3540247: Map source contexts with enums, the "ui_patterns:in_preview" context key has been moved to the ContextNames PHP enum: https://git.drupalcode.org/issue/ui_patterns-3540247/-/blob/3540247-tidy...

Our ticket to adopt this enum is #3590817: Use UI Patterns' context names enum

The next release of UI Patterns 2.0.x is expected for Friday 28 August

UI Patterns 2.0.20 has been released yesterday as expected