Hi,

This looks totally awesome.
To improve the user experience, is it planned (if it even makes sense) to display a discreet spinner / loader image by default for all the blocks loaded after the first flush?

Thanks

Related reading

  1. http://www.callumhart.com/blog/non-blocking-uis-with-interface-previews
  2. http://www.callumhart.com/demo/building-interface-previews-with-react

Comments

fourmi4x created an issue. See original summary.

Fabianx’s picture

The plan I originally formulated at DrupalCon Barcelona was to use the theme system and theme system suggestions for the BigPipe placeholders itself.

e.g.


<div class="big-pipe-placeholder" data-drupal-id="...">
{% if preview %}
  <span class="preview">{{ preview }}</span>
{% endif %}
</div>

Then it would be possible to just create a:

theme/templates/big_pipe--block--user_login.html.twig

to override that block easily.

Obviously the suggestions to use would need some help, but it would

  • a) be the easiest in my opinion
  • b) allow to use theme_debug to find possible templates / suggestions
  • c) Usually it is the theme that knows how the preview should look
  • d) By providing a standard empty preview text, a module could still provide preview via hook_preprocess_big_pipe()

And possibly we could even provide a loading / spinner by default.

Thoughts?

Wim Leers’s picture

Title: Spinner / Loader ? » Interface previews
Priority: Minor » Normal

I'm not sure a spinner by default is a good idea. Imagine many simultaneous spinners. Imagine spinners in a display: inline situation. That'll be ugly and unusable.

That being said, we definitely need interface previews to improve the UX. I forgot to create an issue for that, so I'll just repurpose this issue.

I think #2 sounds like a great path to a first implementation/iteration of interface previews.

darol100’s picture

I do not think this should be part of big_pipe module instead of part of the theme project. Because the looks of the interface preview is going to depend base on theme looks. I think we should handle this by providing good documentation on how to support "Interface Preview" in your theme and provide few examples on how to provide support for the big_pipe project in your theme.

What you guys think about this approach instead of providing a interface preview that might look bad on a different theme ?

krlucas’s picture

Status: Active » Needs review
FileSize
4.07 KB

It seems like the first step is to make js placeholder markup theme-able. Patch attached.

I used the arguments for the #lazy_builder callback to create template suggestions and default classes which works OK for some things (like blocks), less OK for things like messages (it has no arguments).

Wim Leers’s picture

Wow, this is an awesome start! Thank you so much! Excited to have you work with us on this :)


This is very clever, but it also kind of breaks down as soon as the #lazy_builder callback argument order is unfortunate. For example, for the comment form, these are the suggestions:

  1. big_pipe_js_placeholder__node
  2. big_pipe_js_placeholder__node__6
  3. big_pipe_js_placeholder__node__6__comment
  4. big_pipe_js_placeholder__node__6__comment__comment

And indeed, for Drupal\Core\Render\Element\StatusMessages::renderMessages() it flat out fails.

I think using the #lazy_builder (and cleaning it up of course, removing colons and backslashes etc) instead would yield a more workable result. But even then it can be verbose.

I wonder if we can think of a more elegant solution. But again, I love this first iteration :)

krlucas’s picture

Glad to help!

Here's another iteration that uses the lazy_builder callback and cache keys (if available) for the suggestions. I also removed the class and id generation from the template preprocessor--it looks like most core modules just leave that up to the themes now.

krlucas’s picture

I raised this issue at the Boston Drupal meetup, demoing the current state of Big Pipe using the Big Pipe demo module (yay!!). In particular, I surfaced the issue of how to render a sensible, visual placeholder--one that claims page space so the page doesn't dramatically reflow as "actual" content is replaced--when by definition you and the system don't know much about the ultimate content.

One suggestion that came up was allowing users or developers to specify or suggest how the placeholder is rendered. For instance, if one is creating a custom block, allow the user to specify the relative size--small, medium, large--of the placeholder and then allow the theme to determine what that means in a particular region. There's a bunch of UX considerations here--in particular that a custom block would only really need that setting if the "Roles" visibility setting was set. Also "interface previews" are generally only necessary for content that appears above the fold.

But I think it does suggest an API change: #lazy_builder needs to be a little more robust. It should be able to provide not just a callback for the "real" content, but also some clues on how to render the placeholder. So #lazy_builder could be an array:

'#lazy_builder' => [
  '#callback' => '/the/method/that/renders/the/real/content()'
  '#placeholder callback' => '/the/method/that/renders/the/placeholder()' 
   'placeholder' [
     // And/or just a Render Array for the placeholder
   ]
]

Thoughts?

krlucas’s picture

Status: Needs review » Needs work
Wim Leers’s picture

Project: BigPipe » Drupal core
Version: 8.x-1.x-dev » 8.1.x-dev
Component: User interface » big_pipe.module

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Version: 8.2.x-dev » 8.1.x-dev
Pol’s picture

Rerolled patch, just testing.

Pol’s picture

Now trying to find out why this is not working anymore.

Pol’s picture

Updated patch, working now.

If you want to see it working, edit the template and add some CSS:

<div{{ attributes }} style="background-color: #eeeeee; min-height: 100px;"></div>

Wim Leers’s picture

Wim Leers’s picture

Related to BigPipe because BigPipe can sometimes cause jumpiness, depending on the lazy-loaded content: https://developers.google.com/web/updates/2016/04/scroll-anchoring — Google Chrome is introducing scroll anchoring preventing jumpiness caused by reflowing.

krlucas’s picture

Note the latest patch is replacing explode() with DOMDocument parsing which I think is necessary to allow the placeholders to be fully theme-able (not dependent on the placeholder beginning exactly <div data-big-pipe-placeholder-id="). But there's discussion in issue #2678662: Ensure BigPipe does not break when HTML document contains CDATA sections or inline scripts matching certain patterns about whether DOMDocument is the best method.

krlucas’s picture

Re-rolled relative to the drupal root.

Wim Leers’s picture

I've thought about this some more. More importantly, I've been weighing whether this is something we should do in 8.1, or later. The thing is: we don't have enough insight or experience yet with how "interface previews" would and should work. I think #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) and #2759849: Create a new user-facing core theme could and should inform how this works. Especially the latter.

What matter most at this time, is two things:

  1. Whether this can be added without breaking BC.
  2. Whether we see a mechanism to elegantly and simply define interface previews, again without breaking BC.

So, I set out to prove that. I did not look at any work done here so far, to see what I'd come up with independently, and in which ways that would be worse or better. My results:

  1. Note that we only can use interface previews if placeholders are replaced using JS. The JS doesn't need any modifications to support that (it's just looking for an element with a certain data- attribute). The PHP needs to be modified slightly: it should not rely on string parsing, but on DOM parsing to determine the order in which to render the placeholders. That's it! (Commit 1 in the patch.)
    Turns out that I independently came up with pretty much identical code to what #19 already contains :D
  2. I did three incremental iterations (commits 2, 3 and 4 in the patch) of this:
    1. The first one was just to get a sense of what the end user's experience would look like. So, no elegant mechanism to define placeholders, but ugly hardcoded. Great for end users, not yet great for developers/themers.
    2. The second one was to evolve the first one to a place where there is a way for a lazy builder to provide an interface preview. If the lazy builder is called controller::foo, then you can provide an interface preview for it by ensuring controller::fooPreview exists. Very simple. And very logical for those writing lazy builders. Great for end users, great for developers, but bad for themers: they can't override it to match their theme.
    3. However, if we can change it to not use hardcoded HTML, but use the existing #type and #theme and just pass in placeholder variables… then we're again in a great place. When doing so, specify __preview as a suffix, to indicate this is for rendering a preview. That means it is also overridable per theme: you can provide a Twig template to render a preview first. (In the case of comment links, a theme can provide links--comment-preview.html.twig.)

      This doesn't work for everything — it works for things like links and blocks, but it doesn't work for more complex things, like forms. Because forms are often highly customized on a per-site basis. And so the preview for one site won't be good for all sites. Although even there, we can have rough approximations, in templates that themes can override.

All this demonstrated in the included screencast (see below).

This patch shows that this can be done without breaking BC. It's a pure API addition. It's different from the prior patches in that it won't have a very tricky template name, and for the cases where a separate template is not necessary (like the comment links example), it's far, far simpler, and in fact doesn't require any work on the side of the themer: the preview will automatically match the final result! (This is also the direction #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) takes.)

Wim Leers’s picture

Status: Needs review » Postponed
Related issues: +#2759849: Create a new user-facing core theme

I've moved this to 8.2, because I think this is something that we should first gain a better understanding of. #2759849: Create a new user-facing core theme will focus on component-based themes, and I think we may get valuable insights there about interface previews, that could help this implementation to change direction.

What matters, is that we've proven in two independent implementations (see patches #19 and #20) that this can definitely be implemented without breaking BC.

Therefore, marking this postponed for now.

The last submitted patch, 20: interface_previews-2632750-20.patch, failed testing.

Wim Leers’s picture

Dammit, I left some "resetting" code in the patch to record my screencast!

Also, I realize that the visual end result of the placeholders is not very clear at this low resolution. But posting the full resolution video would probably be excessive. So here's a screenshot showing the full detail:

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

I've had an interesting conversation with Kevin O'Leary about this.

  1. He first missed the placeholder boxes. Hence the screenshot in #23. Apparently this is called "greeking" (at least when boxes represent where text will appear).
  2. He felt it would be much better if there was also a placeholder to represent the text editor. Something like the attached image (he created that).

Point 1 is addressed in #23.

Point 2 is very interesting. I contemplated doing that. There's one big problem with that: we don't know where it will be positioned: it could be towards the bottom, or towards the top, depending on which fields the site has configured, and which fields are accessible for the current user. This would be a quite prominent preview, so it'd be quite strange to see this prominent thing shift around significantly.

We can choose to go with more-generic-but-broadly-applicable or high-fidelity-but-narrowly-applicable. I went with the former. To put it in Callum Hart's terminology: the current patch does something between "barebones" and "aspiring". Kevin would like to see something between "aspiring" and "perfectionist".

I'm +1 to striving towards that, but in the current render system (i.e. before #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering)), it'd be quite a chore to maintain all those preview templates.


Finally, Kevin added that for a certain set of generic elements like user pictures, wysiwyg editor, videos, images, lists, paragraphs etc. provide a lightweight scalable svg. placeholder. As long as those SVGs don't show too much detail, I think that could definitely be valuable.


Interesting stuff to take into account when we continue working on this!

Wim Leers’s picture

In March, I noticed that the (excellent!) Belgian real estate website https://www.realo.be/en also uses interface previews. Screenshot included.

Fabianx’s picture

I don't think we need to use DOM parsing.

I think we should use HTML comments to wrap the theme output of the selector in addition to having an ID, that comment could also have the ID itself, so no further parsing needed.

Then we can continue to use simple explodes() for non-JS and the data-selector for JS.

It would put some restraints on the templates, but that should be okay - given the flexibility it gives.

Wim Leers’s picture

I don't understand your comment.

Interface previews only ever make sense when using JS. When using no-JS, we can't first send a placeholder, because we can't replace it afterwards. Therefore we can't have previews in that case. And since we already generate no-JS BigPipe placeholders separately from JS BigPipe placeholders, we don't need to deal with the complexity that you are referring to in #27. :)

See my PoC patches above.

Fabianx’s picture

#28 I don't understand it either.

I am sorry, what I wrote was completely bizarre.

What I meant to write is:

Lets use placeholders that look like:

<!-- START_BIGPIPE_PLACEHOLDER BIG_PIPE_PLACEHOLDER_UNIQUE_IDENTIFIER="my-placeholder-id" -->
<span data-big-pipe-placeholder-id="my-placeholder-id">
  <img src="mypreview.png" />
</span>
<!-- END_BIG_PIPE_PLACEHOLDER-->

which means the code to get the placeholder order does not need to use DOM parsing, but can continue to use explode().

But this time not on the <div id="..."> or <span id="...">, but on the HTML comment and as those are safe for JS to replace we know we can add HTML comments around it.

And the user would never see those comments in HTML anyway, so its just helping markup.

Then the template can be whatever it wants to be - as long as it has the right data selector.

And then we don't need the DOM parsing and it might likely be more reliable, too.

Wim Leers’s picture

Hah :D

which means the code to get the placeholder order does not need to use DOM parsing, but can continue to use explode().

Ahhh! That 's an option, yes. We'll take that into account in the next iteration of this patch.


I won't be working on this for now: this issue is postponed, see #21 for the "why".

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

First, let's rebase this. These patches are identical to #23, but apply cleanly to 8.4.x.

Wim Leers’s picture

My main concern with #23 (and #33) is the TX/DX: it works, but it's not clear how you can make it work.

#19 and earlier patches definitely were simpler: they allow you to define a template, always, and call it a day. However, they do come with a significant disadvantage: they require you to duplicate potentially complex compositions of existing render elements. Because you're replacing an entire render array of arbitrary size with a single template.

This is why I took a different approach in #20. Let me quote a bit from that comment:

This patch shows that this can be done without breaking BC. It's a pure API addition. It's different from the prior patches in that it won't have a very tricky template name, and for the cases where a separate template is not necessary (like the comment links example), it's far, far simpler, and in fact doesn't require any work on the side of the themer: the preview will automatically match the final result!


The key example of this benefit (as that quote already indicates) is the comment links example:

+++ b/core/modules/comment/src/CommentLazyBuilders.php
@@ -151,6 +163,25 @@ public function renderLinks($comment_entity_id, $view_mode, $langcode, $is_in_pr
+  public function renderLinksPreview() {
+    return [
+      '#theme' => 'links__comment__preview',
+      '#pre_render' => ['drupal_pre_render_links'],
+      '#attributes' => ['class' => ['links', 'inline']],
+      'preview' => [
+        '#theme' => 'links__node',
+        '#links' => [
+          ['title' => '▆▆▆▆▆', 'url' =>  Url::fromRoute('<none>')],
+          ['title' => '▆▆▆▆▆', 'url' =>  Url::fromRoute('<none>')],
+        ],
+      ],
+    ];
+    return $this->renderer->renderPlain($links);
+  }

This is what the developer would need to provide. A themer can customize it by providing a links--comment--preview.html.twig template. But usually, that would not be necessary. Because in this example, the preview lies not in a complex rendering, but in showing bars/blocks in the place of the text that will appear.

The patch also provides an example of something that is too complex to benefit from this approach: the comment form.

+++ b/core/modules/comment/src/CommentLazyBuilders.php
@@ -111,6 +112,17 @@ public function renderForm($commented_entity_type_id, $commented_entity_id, $fie
   /**
+   * Interface preview for ::renderForm().
+   */
+  public function renderFormPreview() {
+    return [
+      '#type' => 'form',
+      '#theme' => 'form__comment__preview',
+      '#form_id' => 'comment_form_preview',
+    ];
+  }

+++ b/core/themes/classy/templates/form/form--comment--preview.html.twig
@@ -0,0 +1,3 @@
+<div style='height: 450px; background-color: lightgrey'>
+    <div style='position: absolute; bottom: 20px; width: 150px; height: 30px; background-color: darkgrey'></div>
+</div>

For the comment form, the interface preview callback does not even try to mimic the comment form (it has no way of remotely knowing what it would look like — because a site might e.g. have customized it with lots of additional fields).

So in this case, it makes sense to use that "interface preview template" to provide a rough approximation as the interface preview.


It's entirely possible that experience will tell us that option 1 is something we use so rarely that it is not worth it, and that we hence might as well go back to the approach in #19. That's fine. That's why this is not yet in the BigPipe module in core: because it needs experimentation.

However, in the mean time, the TX/DX should point people in the right direction: a developer (DX) needs to provide an "interface preview callback". And the themer then has the ability to provide a template to customize the default interface preview. That's what I'm doing in this iteration: generate HTML comments (much like Twig debug output) if Twig debug is enabled (duh!), to provide direction.

For the two examples above, that results in:

  1. <span data-big-pipe-placeholder-id="callback=comment.lazy_builders%3ArenderLinks&amp;args[0]=1&amp;args[1]=default&amp;args[2]=en&amp;args[3]=&amp;token=f2l6E3exSxFDxa_4V_oxZaC4lhbUjGxcEMIVDS7iiXI">
    <!-- BEGIN BIGPIPE INTERFACE PREVIEW -->
       <!-- #lazy_builder callback: comment.lazy_builders:renderLinks -->
       <!-- interface preview callback: comment.lazy_builders:renderLinksPreview -->
       <!-- interface preview callback implemented? YES! See theme debug output below: -->
    
    <!-- THEME DEBUG -->
    <!-- THEME HOOK: 'links__comment__preview' -->
    <!-- FILE NAME SUGGESTIONS:
       * links--comment--preview.html.twig
       * links--comment.html.twig
       x links.html.twig
    -->
    <!-- BEGIN OUTPUT from 'core/themes/classy/templates/navigation/links.html.twig' -->
    <ul class="links inline"><li><a href="">▆▆▆▆▆</a></li><li><a href="">▆▆▆▆▆</a></li></ul>
    <!-- END OUTPUT from 'core/themes/classy/templates/navigation/links.html.twig' -->
    
    
    <!-- END BIGPIPE INTERFACE PREVIEW -->
    </span>
    
  2. <span data-big-pipe-placeholder-id="callback=comment.lazy_builders%3ArenderForm&amp;args[0]=node&amp;args[1]=1&amp;args[2]=comment&amp;args[3]=comment&amp;token=TPDftITiSkK7fDcIvkjj1cDUjEih2r6BVIhw_dm-H4I">
    <!-- BEGIN BIGPIPE INTERFACE PREVIEW -->
       <!-- #lazy_builder callback: comment.lazy_builders:renderForm -->
       <!-- interface preview callback: comment.lazy_builders:renderFormPreview -->
       <!-- interface preview callback implemented? YES! See theme debug output below: -->
    
    <!-- THEME DEBUG -->
    <!-- THEME HOOK: 'form' -->
    <!-- BEGIN OUTPUT from 'core/themes/classy/templates/form/form.html.twig' -->
    <form method="post" accept-charset="UTF-8">
      
    
    <!-- THEME DEBUG -->
    <!-- THEME HOOK: 'form__comment__preview' -->
    <!-- FILE NAME SUGGESTIONS:
       x form--comment--preview.html.twig
       * form--comment.html.twig
       x form--comment--preview.html.twig
       * form.html.twig
    -->
    <!-- BEGIN OUTPUT from 'core/themes/classy/templates/form/form--comment--preview.html.twig' -->
    <div style='height: 450px; background-color: lightgrey'>
        <div style='position: absolute; bottom: 20px; width: 150px; height: 30px; background-color: darkgrey'></div>
    </div>
    
    <!-- END OUTPUT from 'core/themes/classy/templates/form/form--comment--preview.html.twig' -->
    
    
    </form>
    
    <!-- END OUTPUT from 'core/themes/classy/templates/form/form.html.twig' -->
    
    
    <!-- END BIGPIPE INTERFACE PREVIEW -->
    

And for everything else, which doesn't have that interface preview callback, this is what you see:

<span data-big-pipe-placeholder-id="callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages&amp;args[0]&amp;token=_HAdUpwWmet0TOTe2PSiJuMntExoshbm1kh2wQzzzAA">
<!-- BEGIN BIGPIPE INTERFACE PREVIEW -->
   <!-- #lazy_builder callback: Drupal\Core\Render\Element\StatusMessages::renderMessages -->
   <!-- interface preview callback: Drupal\Core\Render\Element\StatusMessages::renderMessagesPreview -->
   <!-- interface preview callback implemented? NO, contact a developer… -->
<!-- END BIGPIPE INTERFACE PREVIEW -->
</span>
Wim Leers’s picture

One of the most common things that you might want to create an interface preview for: a personalized or uncacheable block. As of this patch, blocks are supported.

This led me to discover an interesting limitation in the current approach: the "preview" callback does not receive any arguments… and hence it technically only allows a single "interface preview" for all blocks. That's of course then missing the point.

So, I had to make this slightly more flexible. It's working great :)

Now seeing output like this:

<!-- BEGIN BIGPIPE INTERFACE PREVIEW -->
   <!-- #lazy_builder callback: Drupal\block\BlockViewBuilder::lazyBuilder -->
   <!-- interface preview callback: Drupal\block\BlockViewBuilder::lazyBuilderPreview -->
   <!-- interface preview callback implemented? YES! See theme debug output below: -->

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'block' -->
<!-- FILE NAME SUGGESTIONS:
   * block--preview--bigpipedemoblock-2.html.twig
   * block--preview--big-pipe-demo-block.html.twig
   x block--preview.html.twig
   * block.html.twig
-->
…
…

:)