Allow Layout Builder live previews to be toggled to allow easier drag-and-drop

Thanks for this report.

I wonder if we could truncate the block, maybe faded out toward the bottom, while it is in motion.

How about adding a collapse button that collapses and opens all blocks similar to the experimental widget of the Paragraphs module? They are solving the same problem -- that dragging to reorder large blocks of content is difficult. See screenshots of Paragraphs widget.

I agree that collapsing the blocks with toggle is the cleanest approach

This is a major UX bug; I've found it almost impossible to move any even medium-sized bug.

Besides the collapse toggle, another technique observed in the wild is the arrow buttons and drag handle that Wordpress Gutenberg reveals upon hover over a block. Of course, Layout Builder may require 4 arrows at times in stead of only up/down arrows.

I'm all for #7, if we can make that work then that potentially could also improve the accessibility of Layout Builder for those with motor conditions who struggle with dragging things around even with a mouse.

I say "if we can make that work", because of the top of my head we'd need an approach that can dynamically understand what the relative rows and columns of any given Layout are, since Layouts can come from any module, can be designed in whatever way suits the site builder, and since there's no guarantee on the structure of the mark-up used for a given Layout, then it becomes an important consideration.

#2995689: Allow reordering blocks without a pointer device

This is really about viewport size, not actual screen size...

• A user's browser window can be any size, using an arbitrary portion of the physical screen. Many users avoid maximizing their browser windows, because it's easier to keep track of windows when they are arranged spatially.
• I anticipate that screen magnifier users are likely to experience a similar frustrations, perhaps with blocks of any size, even if they have a pair of very large monitors.

#2:

I wonder if we could truncate the block, maybe faded out toward the bottom, while it is in motion.

But is this problem just about the size of the block being dragged? It could also involve the size of the other blocks you're trying to drag it past. It would be good to try some more permutations of this, e.g. moving a small block in a layout which already has other blocks of different sizes.

#3:
I think an option to toggle the block content preview would hugely useful, to the extent that it's a must-have.

As well as helping with this problem of moving large blocks in a small viewport, it would also help users who want to reduce distraction. Hiding the block content preview could make it easier to understand, discuss with colleagues, or otherwise concentrate on, the logical arrangement.

The blocks would need a visible label to tell them apart when the content isn't shown, such as using the block's existing administrative label. (Aside: a visible admin label may also help when the content preview IS shown, especially for non-text content like, embedded video blocks.)

#7 + #8:

[...] arrow buttons and drag handle that Wordpress Gutenberg reveals upon hover over a block.

[...] if we can make that work then that potentially could also improve the accessibility of Layout Builder for those with motor conditions who struggle with dragging things around even with a mouse.

No, this doesn't hold water - controls which appear on hover are ONLY going to work with a mouse (and some styli, such as the Samsung S-Pen). It won't help a motor-impaired user who relies on speech control, who need always-visible controls generally. Nor will they work reliably with pointers that don't provide a hover interaction, such as a finger and touch screen, or most kinds of stylus.

#8:

we'd need an approach that can dynamically understand what the relative rows and columns of any given Layout are, since Layouts can come from any module, can be designed in whatever way suits the site builder, and since there's no guarantee on the structure of the mark-up used for a given Layout

These spatial relationships are already available as structured (and markup-agnostic) data in the layout API's YAML files. We generate layout icons from this data, so we can probably derive permitted movements from the layout YAML too. Maybe even descriptive strings? I don't think the YAML has per-region labels, mind.

The spatial aspects of the UI go beyond just moving blocks though. There's also a need for users to understand a layout, before they even contemplate adding or moving blocks. Screen reader users are especially impacted here. I have some ideas about this, but they'll be better in a separate issue.

From #12:

We generate layout icons from this data, so we can probably derive permitted movements from the layout YAML too. Maybe even descriptive strings? I don't think the YAML has per-region labels, mind.

YAML files might be somewhere to start, thing is not every Layout uses the YAML file to build the icons (for example Bootstrap Layouts provides it's own set of image icons). However I don't think we can reliably build those relationships using the YAML file though, especially for complex Layouts and because Layouts often change shape depending on the size of the viewport.

What we'd really need is a JS based solution which can for each region intelligently figure out "what regions are around me" in the current viewport, and show buttons appropriately that work as expected. For example on a large screen you might have three regions next to each other, but on a small screen those same regions might be stacked, so the "positioning buttons" (let's call them that) need to know how to react and which ones should appear in any given situation.

+1 on the idea of adding a way to expand/collapse all the blocks. That would also be helpful for another UX problem - it can be very hard to tell exactly which blocks are in the layout, especially if you are using layout builder at the content type level, where all the content is lorem ipsum example content and you don't have any idea what each item is. It is even more confusing if someone else placed the blocks originally since you then have no idea at all which block is which.

The collapsed mode could display the name of the block and be a quick way to see which blocks are where, plus make it easier to use drag n drop.

Every block now has a placeholder representation available (for now they mostly all say 'Placeholder for the "@block" block', but can be improved per-block plugin).
These are already used for a block that cannot or does not return content while under preview.

Creating a toggle for this would be interesting for sure.

especially if you are using layout builder at the content type level, where all the content is lorem ipsum example content and you don't have any idea what each item is.

I noticed an interesting way to display real content in the Elementor theme builder (Wordpress ecosystem). They allow the person laying out a template to choose a piece of content as the example content.

So if made a "Hide block previews" that would just show the admin labels would this setting:

1. Be remembered every time you made a change and the blocks are reloaded?
2. Only be in effect for the particular layout until you clicked "Save layout" or "Cancel"?
3. globally for the user? Meaning after you hide if you edit any bundle default and also overrides the preview hide would remain in effect?

What I imagine is a toggle switch in the upper right corner, something simple, the same way you can toggle between a grid view and a list view in many places on the web, with some sort of icon for each option. Maybe called them Expanded view/collapsed view or Preview/Labels.

I think remembering the choice just for the current layout and session would make sense. You don't want to have to keep switching the toggle just because you added or edited a block. Other than that, I wouldn't care about remembering the choice.

@tedbow What is the difference between #1 and #2? For #1, did you mean to say "Lose the hide block preview setting each time you made a change and the blocks are reloaded"?

I like KarenS's toggle idea.

I spun up a D8 site with paragraphs to see how their experimental form display widget handles the "Collapse all" setting. They revert back to the default setting for the field on any page load. In other words, if I've collapsed all when by default all paragraphs are open, if I save the page or otherwise reload, the page loads with all paragraphs open.

Should we be asking the folks doing editor usability studies?

#17.1 - Yes, remember this setting after any change to the layout or block settings. For users who prefer it, we should avoid making them go to extra effort to keep things how they want it. If it was reset after every small change, that could be very frustrating to deal with.

#17.2 - No, because clicking save isn't always the end of the journey. Users naturally change their mind, and return to editing the layout (immediately, or later in the day). Has their need, preference, or disability changed in the meantime? We can't tell. As in the previous point, let's avoid making them repeat the effort of getting the UI how they like it.

#17.3 - Unsure, but tentatively I'd say yes, let's make it global. There are lots of LB usage scenarios to consider, like default layouts for bundles of different entity types. Landing pages can be very long, but layouts for other entity types like blocks or media could be much shorter. A global setting would be the simplest to implement I think. If we learn later that users need it to be remembered per entity type, bundle, or entity, could we provide that later?

Overall, if a user has expressed this UI preference we should keep things that way, until the user chooses otherwise. Rather than expecting user's to put up with assumptions made by designers/developers. Some users may invoke it for a short time, while others want it all the time, but we won't know which.

#18:

What I imagine is a toggle switch in the upper right corner

The corner of the page, or the corner of the layout builder area? Over in #2917777-11: Improvements to the styling, grouping, etc. of the Layout Builder UI actions form, I said we need to move the save/cancel/revert actions, to avoid misuse of the secondary tabs navigation landmark region. That means we'll need a custom landmark region such as <div role="region" aria-label="Layout builder tools"> to serve like an application toolbar for the layout builder. I picked that "tools" label because there are a few other tools that could live there, such as this preview toggle. I propose that's where this toggle should live, grouped with the save/cancel/revert buttons.

the same way you can toggle between a grid view and a list view in many places on the web,

I agree, this is a well-established pattern that ties in well to universal/inclusive design principles. We already did this in the media library module. When we added the card grid, we kept the table view to (a) give users a choice in how the information is presented, (b) retain the UI they are already familiar with, and (c) safeguard against any unanticipated accessibility problems in the new card-grid, and (d) the table view is more easily extended with extra columns.

some sort of icon for each option

A visible text label would be preferable to assist speech control users, and conversations with IT support staff. A checkbox, or radio button would naturally provide this. Icons are good in combination with a text label, but icon-only buttons aren't so good for this.

#19:

Should we be asking the folks doing editor usability studies?

That could be good, but is this even something the previous tests have sought to answer? Also, have they included disabled participants in the studies? One of the use cases for this toggle is to reduce the distance a magnifier user needs to travel to examine the spatial relationships, or to move a block. Another use case is to support neurodiversity; users who want a distraction-free or plain-looking experience. If such users aren't included, the results of the study won't suffice to answer the questions here.

I spun up a D8 site with paragraphs to see how their experimental form display widget handles the "Collapse all" setting

I don't think we can rely on this as a precedent, because the word "experimental" in there implies this isn't well established.

P.S. there are some other visual display toggles worth considering. For example, an option to display visible names for the sections and regions. See comment #15 at #3007978-15: Accessibility Plan for Layout Builder

#2995689: Allow reordering blocks without a pointer device Is another way of solving this problem. Taking care of the accessibility issue also provides a way to move big blocks around the page. The TLDR is it adds a 'Move' option to the block, so each block's contextual links would say 'Configure, Move, Delete'. If you choose 'Move', you would have options to choose which region to move it to and use tabledrag to place it above or below the other blocks in that region.

In addition, that solution has a patch, which this one does not.

@KarenS thanks for pointing out #2995689: Allow reordering blocks without a pointer device. That will allow moving large blocks so maybe could this issue at least could not be a Layout Builder stable blocker.

I was trying to figure if we could still make the dragging of large blocks easier though. I attached a patch that just assigns a class to the currently moving item. Then CSS sets a max-height.

There are couple problems with this and the patch maybe just proves this method won't work. But maybe someone else can make it work.

1. Dragging a large item down doesn't work unless you first drag it up. It seems to be acting like the height is still the pre max-height height. I was wondering if somehow adding the layout-builder-moving class before the start event but I couldn't figure this out. As soon as you move up to dropzone the dropzones below start to work.
2. If you start dragging on a large item by clicking further down on the item then the updated max-height then your pointer won't be "on" the item any more. You can actually still drag but the item may even be out of your view.

   This actually relates to a problem without the patch too. If you click far down on a large item then the dropzone might still be out of you view.

   The problem with and without the patch could be fixed by adding "✢" (closet emoji) handle and the user would have to use this start dragging. I think this might be needed anyways because I have seen people not be able to understand that they could just click a block to drag it.

   The original mocks for the layout builder had drag handle much like we have on the block layout page and other draggable tables. So if we used the same handle the user might know this is for dragging because it is consistent without other parts of our ui.

Rewrote the issue summary to focus this on the toggle idea.

Here's a first stab at the toggle approach + a gif of it in action.

This is a great start!

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,4 +1,6 @@
   -(($, { ajax, behaviors }) => {
   ...
   +  const { ajax, behaviors, announce } = Drupal;
   

   Why move the destructuring?

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -49,6 +51,67 @@
   +          const $this = $(this);
   

   Do we still do this? I feel like if @alwaysworking was here he'd say something like "use an arrow function"

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -49,6 +51,67 @@
   +          $this.children(':not(.contextual)').hide(500);
   ...
   +            .show(500);
   

   What's the 500?

4. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -49,6 +51,67 @@
   +              announce(
   +                Drupal.t('Layout Builder editor is in live preview mode.'),
   +              );
   ...
   +              announce(Drupal.t('Layout Builder editor is in grid mode.'));
   

   Did prettier do this? Weird that they are different

5. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -344,4 +359,49 @@ public function saveLayout(SectionStorageInterface $section_storage) {
   +    $live_preview_enabled = $this->privateTempstore->get($live_preview_id);
   ...
   +      '#checked' => !is_null($live_preview_enabled) ? $live_preview_enabled : TRUE,
   

   To me this seems like an odd mix of client side and server side. I'm not saying it's wrong/bad, just that I was surprised.

   How does this behave when the page is refreshed? Or when another tab is opened?
   I'm not even sure what is desirable behavior, but we should decide what is and make sure this does what we want.

6. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -97,7 +97,11 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      $grid_mode_label = $block->getPluginDefinition()['admin_label'];
   +      if (!empty($block->getConfiguration()['label']) && $block->getConfiguration()['label'] !== (string) $grid_mode_label) {
   +        $grid_mode_label .= ': ' . $block->getConfiguration()['label'];
   +      }
   

   Can this have an @todo pointing to #2025649: Add title-related methods to BlockPluginInterface to help clarify and resolve many issues?

#26.1
Moved the destructuring because PHP scans the js files for the specific string Drupal.t in order to find translation strings, so the Drupal object still needs to be available.

#26.2 Seeing the jQuery dollar sign sometimes blinds me to arrow functions - that can be switched to event.currentTarget

#26.3 The default 400ms transition for hide() and show() felt a little too fast for me so I changed it to 500ms. I'm open to the possibility of that 100ms only being noticeable to someone several hours into creating a patch 🙂

#26.4 Yep - its something prettier is doing. Switched it back several times and re-ran prettier just to be sure it wasn't something on my end.

#26.5 When the layout builder edit UI is rebuilt after making changes to a block or section, the Live Preview input is part of that rebuild. There needed to be a way for the rebuild to know if Live Preview was disabled, otherwise it would switch back to the default value after any change. I went with the private tempstore approach as it also allowed this value to be maintained on reload/refresh - I'm partial to that as I think it provides a more consistent experience, but quite open to the possibility I'm overlooking some drawbacks.

So it sounds like #26.5 requires some discussion. Perhaps we can reach an agreement on that, then I'll take care of #26.2 #26.6 and anything we conclude needs to happen with #26.5

#26.2
Switched to arrow functions
#26.4
With other refactoring, prettier isn't doing that weird formatting to the first announce(), so that's cool.
#26.5
Switched to local storage - better solution in general and the code is easier to follow.
#26.6
Added the @todo to improve block label methods. Also switched to use getPreviewFallbackString() to create the grid mode labels, since that already returns sufficient information about the blocks without additional clunky customization.
I do want to mention that I'd be happier if the string didn't include "Placeholder for the", but it seems like the cleanest way at the moment to distinguish the block types.
This is how it looks with the patch

I was very tempted to just do a string replace for "Placeholder for the", (it looked nice) but that seems kinda hacky,

Fixed a few JS code standards after remembering that prettier doesn't do everything for me.

Added tests and a JS fix.

This is shaping up nicely! Awesome work.

One question, bunch of nits.

1. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -344,4 +347,44 @@ public function saveLayout(SectionStorageInterface $section_storage) {
   +  public function buildLivePreviewToggle(SectionStorageInterface $section_storage) {
   

   Nit: Please make this method protected

2. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -344,4 +347,44 @@ public function saveLayout(SectionStorageInterface $section_storage) {
   +      '#attributes' => [
   +        'id' => 'layout-builder-live-preview',
   ...
   +      '#label_attributes' => [
   +        'for' => 'layout-builder-live-preview',
   +      ],
   

   I thought we only had to use for when it was on a separate label element pointing to the input element. Seems redundant here?

3. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -344,4 +347,44 @@ public function saveLayout(SectionStorageInterface $section_storage) {
   +      '#description' => $this->t('Block preview content will be visible.'),
   +
   +    ];
   

   Nit, unnecessary blank line

4. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -108,8 +108,14 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      // @todo  Use new label methods so data-layout-grid-mode-label doesn't
   

   Nit, remove extra space after @todo

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,149 @@
   +  public static $modules = [
   

   nit: Should have an {@inheritdoc}, and should be protected

Needs rolls.

I was playing around with this patch on Umami demo. the contextual links don't work when live preview is disabled. Not sure on Bartik yet. but probably.

I would be great if they worked as I think some people would want to most of their work with the live preview.

Rerolled since I broke it. Also fixed #31 points 1, 3, 4, 5.
#31.2 still needs addressing.

Wanted to update in case this gets looked at during DCNJ before I can provide a new patch

1. #31.2 I've always read the spec as doing it whenever possible. The most tangible benefit that comes to mind in this instance is that the for attribute makes the label clickable in addition to the input. The larger target area really benefits users with limited motor control. For this reason it may actually warrant a separate issue to somehow make this default behavior when rendering checkbox inputs, so a @todo may show up in the next patch
2. #32 Contextual links should work in grid mode -- it looks like there's a problem with them working if the editor page loads in grid mode. If you start in live preview, or switch to live preview and back to grid, they work as intended. Will fix.
3. Also spotted an issue where extra fields preview content is not hidden in grid mode. I know what is happening there and the fix will be in the next patch.

1. #31.2 -- realized the for attribute renders the way I was hoping if #id is set at the top level of the render array instead of doing it inside #attributes. Plus, it's way more concise
2. #32 addressed the issue with contextual links not appearing on page reload. This scenario is also part of the test coverage.
3. This patch also has a solution for hiding/showing non-tagged children of a block, which aren't covered by the hide() method. In these instances, this content is stored in a data attribute in grid mode, then returned to the block in live preview. It feels like there may be a cleaner way to do this, but perhaps that will surface in the review.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -51,4 +66,105 @@
   +       * @param transitionTime
   +       *  Time in milliseconds that the elements transition to hidden.
   +       */
   +      function gridMode(transitionTime) {
   

   For me the animation is just distracting. I played around with transitionTime == 0 and it seems better.

   Especially with big fields like body field default value you really see it scrolling up.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -51,4 +66,105 @@
   +          const adminLabel = document.createElement('h2');
   +          adminLabel.className = 'data-layout-grid-mode-label';
   +          adminLabel.innerHTML = $element.attr('data-layout-grid-mode-label');
   +          $element.prepend(adminLabel);
   

   I am not sure what the benefit of rendering the admin labels here in js vs just rendering them on the server side and showing/hiding them with a css class.

3. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -130,6 +133,42 @@ protected function layout(SectionStorageInterface $section_storage, $is_rebuildi
   +    $live_preview_id = "Drupal.layout_builder.live_preview.$storage_type.$storage_id";
   

   should we make this id also unique for user id also.

   Maybe an edge case but if I log out and someone else uses by browser. Should they have their own setting?

4. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -108,8 +108,14 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      $preview_fallback_string = $block->getPreviewFallbackString();
   +      // @todo Use new label methods so data-layout-grid-mode-label doesn't
   +      //   have to use preview fallback in https://www.drupal.org/node/2025649.
   +      $build['#attributes']['data-layout-grid-mode-label'] = $preview_fallback_string;
   +
          if ($is_content_empty && $is_placeholder_ready) {
   -        $build['content']['#markup'] = $block->getPreviewFallbackString();
   +        $build['content']['#markup'] = $preview_fallback_string;
   

   Right now for a field that will only show the preview like 'Placeholder for the "Links" field' when I toggle live preview there is still a transition even though the text of the block does not change. It is just the same text now in an h2.

   So it looks kinda of weird just getting bigger and not changing.

   Could we consider the blocks that just the placeholders(or in the future labels) to already be in "grid mode" so they would not be affected by the toggle button(since there text isn't going to change.

   Maybe this would mean they would always be in an h2.

Reroll attached

1. #38.1
   

   For me the animation is just distracting.

   I'm partial to the animation because switching mode can potentially change the appearance of the page significantly. I think easing into it makes it easier for users to infer what is happening when they change modes. Perhaps this is somewhere we'd benefit from a few more people weighing in. I've attached gifs of the 500ms transition and one with no transition. If nobody weighs in here I'd like to get feedback at the next UX meeting. Definitely willing to let go of it, but would feel better with a little input from someone who isn't immersed in Layout Builder as much as either of us

2. #38.2
   

   I am not sure what the benefit of rendering the admin labels here in js vs just rendering them on the server side and showing/hiding them with a css class.

   That approach was in the initial iteration of this, but didn't work as it's possible for the container div to have text nodes children not wrapped in tags. The only way to hide that content would be to hide the container div, and the container needs to remain visible. There's some possibly sweaty JS that deals with these non-tagged children, too

   /**
              * If the immediate child of an element is a text node, it won't be
              * included in the hide() function. Save the value with data() so it
              * can be used later, then remove from the DOM.
              */
             element.childNodes.forEach((child, index) => {
               if (child.nodeType === 3 && child.textContent.trim().length > 0) {
                 const notNestedText = child.textContent.trim();
                 $element.data('not-nested-text', notNestedText);
                 element.childNodes[index].nodeValue = '';
               }
             });

   If there's a more elegant way to do this, I'm very much on board.

3. #38.3 I added user ID to the live preview id as suggested. Its a nice edge case to address.
4. #38.4 this crossed my mind as well - it's less jarring with the transitions, but I also erred in the direction of the grid labels all being h2 so it's visually consistent. I had also thought this benefitted screenreaders, but my test drive suggested there was no difference. Also worth mentioning that the Grid mode labels will probably change if https://www.drupal.org/node/2025649 is completed.

Re: #38.1 , talked with @tedbow and he's OK with the transitions

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -160,4 +160,106 @@
   +          livePreviewMode(500);
   +          announce(Drupal.t('Layout Builder editor is in live preview mode.'));
   +        } else {
   +          gridMode(500);
   

   Would there be way to set 500 as a var so that other modules could change this?
   Like behaviors.layoutBuilderToggleLivePreview.transitionTime or something. Maybe I am wrong it just seems like it wouldn't a 1 size fit all thing.

2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -127,6 +130,43 @@ protected function layout(SectionStorageInterface $section_storage) {
   +    // $live_preview_id is used for local storage to store live preview status.
   ...
   +    $live_preview_id = "Drupal.layout_builder.live_preview.$storage_type.$storage_id.$user_id";
   ...
   +        'data-live-preview-id' => $live_preview_id,
   

   I think the comment here would make more sense where we actually set the attribute. Because the PHP var is not what is used but the attribute.

   Maybe directly data-live-preview-id line we should have a comment

   "Set attribute to be used by local storage for live preview status."

3. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -127,6 +130,43 @@ protected function layout(SectionStorageInterface $section_storage) {
   +      '#description' => $this->t('Block preview content will be visible.'),
   

   This description seems weird to me. Do we use this pattern elsewhere where say "will be" as description of will happen if you check a box. If it is already checked this seems a weird thing say.

   some ideas:
   "Show content preview"
   "Preview block content"
   "Toggle Block preview"

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,175 @@
   +    $assert_session->waitForElementVisible('css', '.data-layout-grid-mode-label');
   

   Should be wrapped in $this->assertNotEmpty()

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,175 @@
   +    $assert_session->elementExists('css', '.data-layout-grid-mode-label');
   

   I think this should be a $this->assertNotEmpty($assert_session->waitForElement() because this happens in behaviors so I could see it failing randomly.

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,175 @@
   +    $assert_session->waitForText($live_preview_body_text);
   

   Should be wrapped in $this->assertNotEmpty()

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,175 @@
   +    $assert_session->waitForText('Placeholder for the "Links" field');
   

   Should be wrapped in $this->assertNotEmpty()

8. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,175 @@
   +  protected function assertContextualLinks() {
   

   I am not sure about this assertion. Could we change it something to like choosing a block(or all) and just using ContextualLinkClickTrait to open the configure form and then close the form without submitting. Seems more functional.

9. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,175 @@
   +  protected function assertOrderInPage(array $items) {
   

   Are the things that we are asserting just block labels? Or text that should be in the blocks?

   I think we can do this without throwing exceptions and instead just using assertions which might make it more readable.

   If this was changed this to send all the block labels(we should know what they are) in the right order. We could use
   $blocks = $this->getSession()->getPage()->findAll('css','[data-layout-block-uuid]'); and then make sure the elements have the text in the correct order.

   This would be very similar to test function in #2995689: Allow reordering blocks without a pointer device assertBlockTable().

   it won't be exactly the same but very similar.

This patch uses 500. jQuery has the string 'fast' which is 600.
Or, the default is 400.
If we use the string 'fast' we might not need to document the magic number

This patch addresses everything in #41. Because the test method changed as a result of the excellent feedback in #41.9, I also included a fail patch with the test params intentionally swapped.

For #41.1, I took Tim's advice from #42 and changed it to 'fast', which I subjectively prefer to the previous 500 anyway. To make sure it was OK to not have the duration stored as a variable, I confirmed core JS has several uses of jQuery hide() and show show() without any arguments. This means there are multiple existing defined-by-jquery duration values for hide/show - just like the arg 'fast' would be doing. Based on this, it seems like 'fast' should be acceptable.

Reroll + two changes

1. Updated a selector for layout blocks to one created in #2973921: Interactive controls inside preview block in the Layout Builder form should be disabled.
2. Added spaces above a few comments in the JS file

Attached interdiff between reroll and the changes I just made.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,166 @@
   +    $this->assertNotEmpty($this->assertSession()->waitForButton('Close'));
   +    $page->pressButton('Close');
   +  }
   

   Just to be extra careful here we should use $this->waitForNoElement('#drupal-off-canvas'); to make sure the the test does not proceed until the off-canvas dialog is removed from the page.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,166 @@
   +    $blocks_with_expected_text = array_filter($blocks, function ($block, $k) use ($items) {
   +      $block_text = $block->getText();
   +      return strpos($block_text, $items[$k]) !== FALSE;
   

   We could probably afford to spell out $key here 😜

   Otherwise rewrite of this function looks good!

3. Everything else addressing my review in #41looks great
4. I like the change to use 'fast' since this allows up not be picking an exact number but use what core uses in other places.

Made the changes requested in #46

Looks good! 🌝

Is the added CSS here compatible with #3034347: Update the Layout Builder UI classes to implement BEM naming conventions?

Reroll

Per #50, Refactored selectors to use BEM

Also worth mentioning that yarn prettier changed a line unrelated to this issue

--- a/core/modules/layout_builder/js/layout-builder.es6.js
+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -127,9 +127,7 @@
            */
           update(event, ui) {
             // Check if the region from the event and region for the item match.
-            const itemRegion = ui.item.closest(
-              '.layout-builder__region',
-            );
+            const itemRegion = ui.item.closest('.layout-builder__region');
             if (event.target === itemRegion[0]) {
               // Find the destination delta.
               const deltaTo = ui.item

Prettier makes this same change if run on HEAD, so I suspect this is due to prettier not being run with #3034347: Update the Layout Builder UI classes to implement BEM naming conventions

Fixed failing test

The patch in #55 is incorrect -- ignore that one.

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -75,6 +75,17 @@
   +.layout-builder-grid-mode .layout-builder-block__grid-mode-label {
   

   Could this be just .layout-builder-block__grid-mode-label since the element would never exist outside of .layout-builder-grid-mode?

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -195,4 +193,110 @@
   +        $layoutBuilder.addClass('layout-builder-grid-mode');
   

   How about layout-builder--grid-mode?

3. I don't usually get motion sickness, but the transition animation on the change of preview mode made me feel dizzy immediately. I tested how this would feel like without the transition, and I would definitely support removing the transition altogether.

Re #57

1. Could this be just .layout-builder-block__grid-mode-label since the element would never exist outside of .layout-builder-grid-mode?

   The additional specificity beyond a single class is required to fully reset the margins.

2. How about layout-builder--grid-mode?

   Right, it's a modifier. Made the change

3. I tested how this would feel like without the transition, and I would definitely support removing the transition altogether.

   That's further evidence my preference is the exception so I've removed the transition

Fixing tag

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -75,6 +75,17 @@
   +  cursor: move;
   

   Any thoughts if we should make this change for the default UI as well?

2. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -75,6 +75,17 @@
   +.layout-builder--grid-mode .layout-builder-block__grid-mode-label {
   

   It seems like the difference this makes in Bartik is very minimal. I think that we could at least temporarily ignore it. Maybe we could remove layout-builder--grid-mode from the selector and create a follow-up to add a layout builder specific selector into Bartik after layout builder is stable.

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -195,4 +193,104 @@
   +          adminLabel.className = 'layout-builder-block__grid-mode-label';
   +          adminLabel.innerHTML = $element.attr('data-layout-grid-mode-label');
   +          $element.prepend(adminLabel);
   

   We probably should move this to a theme function since some people might want to customize this. Let's make also sure that once we move this into a theme function that we add a js- prefixed class that we utilize in JavaScript.

Re #61

1. Any thoughts if we should make this change for the default UI as well?

   I support this and even created a ticket for it several weeks ago #3024774: Visual indicators to indicate block draggability. Since it sounds like its in scope here I added to the patch.

2. Maybe we could remove layout-builder--grid-mode from the selector and create a follow-up to add a layout builder specific selector into Bartik after layout builder is stable.

   Sure, .layout-builder--grid-mode removed from selector and I created #3037389: Add Layout Builder specific selector to Bartik once Layout Builder stable

3. We probably should move this to a theme function since some people might want to customize this. Let's make also sure that once we move this into a theme function that we add a js- prefixed class that we utilize in JavaScript.

   Created theme function and js- prefixed classes

+    $preview_checkbox = [
+      '#title' => $this->t('Live Preview'),
+      '#type' => 'checkbox',

I can only see one checkbox mentioned in the patch. Has this issue only implemented 2 states?

The wireframes we reviewed with the Berkeley Web Access team used radio buttons for 3 states: show block content previews, show block titles, and show both.

@andrewmacpherson I think the one checkbox is fine for now, and if desired you could create a followup issue to investigate more options. The current patch adds a checkbox that toggles between admin labels only and preview only (close to what will be rendered when the page is viewed). Preview plus admin labels is going to be most useful if there is no label or block title in the rendered preview, or if whatever is in the preview is too generic to be clear what it actually is. Or, if there is no admin label or the admin label is too generic.

I chatted with @andrewmacpherson and @cboyden who is on the Berkeley Web Access team @andrewmacpherson mentioned

We all agreed the current checkbox is sufficient for the first implementation and serves the dual purpose of making it easier to drag and allowing users to know what different blocks are when the preview doesn't make it obvious.

We can look later to determine if we actually need the 3 state "show both live preview and admin labels

Despite what caniuse.com reports, forEach() is not supported in IE11. I refactored the JS so it works on all supported browsers and addressed a few nits.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
@@ -143,7 +145,6 @@ protected function assertOrderInPage(array $items) {
     $blocks_with_expected_text = array_filter($blocks, function ($block, $key) use ($items) {
...
     }, ARRAY_FILTER_USE_BOTH);

I don't think this works on PHP 5.5, sadly.

If you're set on this, you can use \Zend\Stdlib\ArrayUtils::ARRAY_FILTER_USE_BOTH with \Zend\Stdlib\ArrayUtils::filter

Re: #68 Swapped out to use \Zend\Stdlib\ArrayUtils::filter instead.

#70.1

Slightly nitpicky: I assume "bool tracking" means it implies the variable holds a boolean. This is only true if "livePreviewId" key exists in the local storage, otherwise it returns null, which isn't a boolean.

Good catch, particularly because of the changes made to address #70.3. Comment has been revised.

#70.2

Maybe we can encapsulate this logic to a "toggleGridMode" or enableGridMode, v.s disableGridMode, having livePreviewMode removing the grid mode and its state seems off.

I renamed the livePreviewMode() to disableGridMode(), as you correctly pointed out that livePreviewMode() isn't actually enabling live preview. I updated several comments to make this distinction clearer and explain what the functions are doing. If I overlooked additional refactoring opportunities, hopefully this renaming+comments will make those easier to identify.

#70.3

is often implies a boolean type, but we're checking for a string here. Does .is return a string boolean?

.is returns boolean, but localStorage (at least currently) stores everything as string. Having this pointed out makes it apparent how this could be confusing. I added the use of JSON.stringify() and JSON.parse() in setting/getting localstorage so the type is preserved.

1. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -128,6 +131,43 @@ protected function layout(SectionStorageInterface $section_storage) {
   +    $user_id = \Drupal::currentUser()->id();
   

   This should be injected, add it to __construct/create above

2. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -109,8 +109,14 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      $preview_fallback_string = $block->getPreviewFallbackString();
   
   +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   index f0d83e80fb..fe04df6329 100644
   --- a/core/modules/layout_builder/tests/src/Unit/BlockComponentRenderArrayTest.php
   

   It's not clear from the patch, but can there be an explicit test for a block that does NOT implement PreviewFallbackInterface?

   Because that's not required, and this needs a more clear instanceof check (it's currently buried in the $is_placeholder_ready variable)

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,169 @@
   +   * Test live preview toggle.
   

   Nit: "Tests"

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,169 @@
   +    $this->drupalPostForm(
   +      'admin/structure/types/manage/bundle_for_this_particular_test/display/default',
   +      ['layout[enabled]' => TRUE, 'layout[allow_custom]' => TRUE],
   +      'Save'
   +    );
   

   Does this actually work? Previously I've needed two form submissions since allow_custom was disabled at first.

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,169 @@
   +    $node = $this->createNode([
   ...
   +    $node->save();
   

   createNode saves it for you, I don't think you need the local variable at all

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,169 @@
   +    $assert_session->elementNotExists('css', '.layout-builder-block__grid-mode-label ');
   ...
   +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', '.layout-builder-block__grid-mode-label '));
   ...
   +    $this->assertNotEmpty($assert_session->waitForElement('css', '.layout-builder-block__grid-mode-label '));
   

   There's a trailing space here. If it's on purpose, that deserves a comment (and maybe putting the string into a variable or something?)

This should address all feedback from #73 an #74.

Well, except for one small (tests-breaking) mistake; I forgot to inject the current user into the LayoutBuilder render element.

Apparently that isLivePreview variable should default to true, not false, in order to keep everything shipshape...

Nits while you fix the test fail :)

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LivePreviewToggleTest.php
   @@ -0,0 +1,167 @@
   +    $node = $this->createNode([
   

   $node isn't used

2. +++ b/core/modules/layout_builder/tests/src/Unit/BlockComponentRenderArrayTest.php
   @@ -130,6 +138,82 @@ public function testOnBuildRender($refinable_dependent_access) {
   +    $expected_cache = $expected_build + [
   +        '#cache' => [
   +          'contexts' => [],
   +          'tags' => ['test'],
   +          'max-age' => -1,
   +        ],
   +      ];
   

   Weird indentation

Fixed the nits and removed the || true I had added in the JavaScript, since it was the wrong approach. If the live preview state has not previously been set, it will be null, which means that || true will make it true (which is correct). But if it's false, || true will force it back to true, which is wrong. So I changed it a bit; it will now only be true if it's explicitly not false.

I have some concerns about the h2 here.

Firstly, what's the purpose of the h2? It's mentioned in #38-39 as being for visual consistency.... but consistency with what, exactly?

I think the h2 may not be appropriate for the document outline here. Remember, the document outline must make sense for all users, but screen reader users in particular rely on it heavily. There is still no clear plan for how screen reader users will be able to make sense of layout builder (at all!) but one way we might address that is with headings.

I think it's likely that we will introduce heading semantics later on as part of #3019487: [plan] Describe layout builder UI to assistive technology, however we won't know if h2 is appropriate here, until after that issue has been addressed. It may end up being h2 for layout sections, h3 for layout regions, and h4 for layout blocks. Or it might remain as a div.

So far, this patch principally helps sighted pointer users with the drag distance. This use case has no particular need for machine-readable heading semantics at all. The only places the h2 is mentioned in patch #81 is when the element is created, and in a few test assertions.

If the h2 is currently only being used for visual effect, I propose we remove it from the patch here, and use a class for visual styling.

The screen reader announcements in patch #81.

+        if (isChecked) {
+          disableGridMode();
+          announce(Drupal.t('Layout Builder editor is in live preview mode.'));
+        } else {
+          enableGridMode();
+          announce(Drupal.t('Layout Builder editor is in grid mode.'));
+        }

What does "grid mode" mean, and why would a screen reader user care? The phrase "grid mode" doesn't appear visibly anywhere, so a sighted user presumably doesn't need to know it. Yet both people are using the same UI. How are they intended to help?

• Think about what you actually need to convey to the user.
• Think about what actually changes on screen, not what you have decided to call it as developers.
• Also think about the checkbox itself; what information does the label convey, and what expectations does that set for the user?
• What does a screen reader normally announce when you click a checkbox?
• Do you need Drupal.announce() at all?

Update: also, when we provide the option to display the block content previews and the block label, what will that be called?

The live preview checkbox should be outside the layout builder, in the separate tools area above. It's something which assistive tech users will need to be able to find quickly. See #2917777: Improvements to the styling, grouping, etc. of the Layout Builder UI actions form.

+        $preview_fallback_string = $this->t('Placeholder for the "@block" block', [,

This is needlessly verbose....

• It's repetitive, and increases the reading effort needed to see what you really want.
• It takes up space, and causes block names to wrap to several lines of big text in narrow region columns. But the main driver for this issue was to make blocks smaller. Some of them actually end up far bigger.
• If #3019487: [plan] Describe layout builder UI to assistive technology is addressed using headings, it will really slow down a screen reader user who navigates by headings, because they have to wait to hear the fourth word before they can decide it's the wrong one.

I recommend removing the phrase "Placeholder for the ", and the quote marks, from this phrase. I Propose shortening this to just the actual name of the block - i.e. these will do:

• Cooking time
• Authored on
• Body
• Revision create time

Instead of using $block->getPreviewFallbackString(), can we just use $block->label()?

I'm unsure if the block type matters here (field, views block, etc). It's kinda handy but it still takes up space, and one word is enough to cause an extra line in the narrow layout regions.

edit: fixed broken markup in this comment.

#85
I'm looking forward to #2917777: Improvements to the styling, grouping, etc. of the Layout Builder UI actions form, as I think it will be a great addition. If this issue is committed before that, it will become that issue's responsibility to relocate this toggle.

#86

The reason for that "Placeholder for the.." string is this:

• It's the \Drupal\Core\Block\BlockBase::getPreviewFallbackString() implementation.
• 100% of all core blocks and 99.99999% of every other block will follow the if code block.
• Only blocks that eschew the base class (BlockBase) and choose to implement BlockPluginInterface from scratch (which never happens) that also choose to not implement PreviewFallbackInterface (even more unlikely) will use this else case.

Already, the preview fallback will be used in multiple cases when editing the Defaults anyway, so this is used for consistency.

If there is desire to change the current core implementation of getPreviewFallbackString(), please open a follow-up.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -201,4 +201,135 @@
+          announce(Drupal.t('Layout Builder editor is in grid mode.'));

Following up on #84; how about we announce that the layout builder live preview is turned off? That way we would be able to avoid naming the non-preview mode.

Implemented Lauri's suggestion from #88.

#83 still needs to be addressed

#84/88 was addressed

#85 is covered by that followup

#86 would need a separate issue, per #87

So, NW for #83

Discussed with @bnjmnm and @tim.plunkett. We concluded that the best solution to #83, at least for now, is to change the element to a div and copy basic H2 styling to it so it looks consistent. So...I did that. Screenshot attached of what it looks like in Bartik.

I did check Umami's styling too and discovered that there is slightly more, er, detailed styling applied to H2 elements. Not sure what, if anything, to do about that...so I defer to the experts (@lauriii) for further guidance about that.

Forgot to remove a couple of h2 references in the tests.

Attaching a new screenshot of what this looks like after #91.

I believe that is the end of all outstanding feedback, thanks!

I still have lots of feedback.

#87:

If there is desire to change the current core implementation of getPreviewFallbackString(), please open a follow-up.

No, that's not what I mean. I'm not suggesting there is anything wrong with the implementation of getPreviewFallbackString(). I can see where it's being used for image fields already, in the manage-display default layout.

Rather, I mean it's the wrong method to use here. We don't use the placeholder string to identify blocks in admin/structure/block, or fields in manage display (when layout builder isn't enabled). I think we should use $block->label() for consistency with those UIs. This (loosely) relates to WCAG SC 3.2.4 Consistent Identification.

We are going to need the $block->label() somewhere in the DOM, constantly, to address #3019487: [plan] Describe layout builder UI to assistive technology. I think it will probably be a heading element (likely H4). Can we make it double up as the visible block label here?

The JS currently adds or removes a block label element every time the checkbox state changes. Instead, this could be changed so the checkbox toggles the block label's style from display:none to display:block, but it wouldn't add and remove the block label element itself.

One of the use cases for this feature is that visible block labels make it easier for colleagues to have a conversation about the layout. For example, if a sighted marketer wants to instruct a blind content manager to move a block. So I'm worried that won't work so well if a screen reader user finds their way around using the block label, but when they turn visible labels on their sighted colleague sees a different string. That's not really an equivalent experience.

Edit: forgot the WCAG reference.

I've been fleshing out #3019487: [plan] Describe layout builder UI to assistive technology. The needs of that issue overlap somewhat with this one, specifically the need for an administrative block label. I think it would be good to do develop that one in tandem before committing this.

Considered as separate feature requests, #3019487: [plan] Describe layout builder UI to assistive technology isn't exactly a prerequisite to this one. But in practical terms, if we commit this one in it's current state, we'll end up having to undo some of this work to address that one.

#65/66 - is there a follow-up issue yet, for the feature to display the content previews and the block label at the same time? I can file one if it doesn't exist yet.

#88/#89:
+ announce(Drupal.t('Live preview is disabled in the Layout Builder editor.'));

That's somewhat better, because the mystery term "grid mode" is no longer announced. But it's really just repeating the checkbox name and value, which the screen reader will annouce anyway. Is that the whole story, or has anything else changed?

The phrase "live preview" niggles me. It suggests to me things like twitter feeds, slideshows, stock tickers, etc. - content that is dynamically updated without the user doing anything. In fact that's not really the case; nothing changes in the layout builder UI except in response to the user's actions. Assistive tech also uses the phrase "live region" in the UI, to mean content that updates dynamically (e.g. macOS VoiceOver can list live regions in the Web Rotor tool).

My hunch is that "live preview" is too close a term to these. Could we change this to "show content preview", or something like that?

#100
Grouping these arbitrarily since you didn't number or group your feedback.

1)
In the patch already there is an @todo for #2025649: Add title-related methods to BlockPluginInterface to help clarify and resolve many issues.
But in the meantime, we *cannot* use $block->label() here.
The result of that is nonsensical for derived blocks like fields or views.
No one is arguing that the current output of getPreviewFallbackString() is ideal. But it is at least not ambiguous.

2)
This was suggested in #38.2 but cannot be done for technical reasons. Citing #39.2:

That approach was in the initial iteration of this, but didn't work as it's possible for the container div to have text nodes children not wrapped in tags. The only way to hide that content would be to hide the container div, and the container needs to remain visible. There's some possibly sweaty JS that deals with these non-tagged children, too

#101

we commit this one in it's current state, we'll end up having to undo some of this work to address that one

That's fine. This is a stable blocker and must be resolved in the next week and a half, that is not.

#102
No there is no issue for this. I wasn't sure it was even desirable to have a third state after the last discussion, but feel free to open that follow-up.

#103
Are you suggesting that the announce calls be removed? I agree that it confirming that the checkbox has indeed done what it was supposed to do isn't very useful.

#104
There is a very real possibility that the content is actually live. That's up to the content.
Twitter feeds, slideshows, and stock tickers could very well exist as blocks.

Additionally, Layout Builder can be used for things that aren't typically referred to as "content".
Mega menus, user profiles, comment forms, etc.
I'd much rather leave this naming as-is.

When rerunning prettier it made one additional line-wrapping change.

There is a very real possibility that the content is actually live. That's up to the content.

But that's not what the checkbox refers to. The checkbox is about whether the content is shown at all.

We're storing this UI preference per layout builder. This was something that hasn't been discussed earlier in this issue, however it was discussed at some length with the UC Berkely Web Access team. There were 2 key points:

1. Where should such UI preference live? We agreed these are things a user would want to be able to change on the fly, while using the layout builder. The upshot was these controls should be available as part of the layout builder UI itself, rather than live on their user account page. (That's not really relevant here, I'm just noting it as background context to the second point).
2. How granular should the UI preferences be? In particular, should they be stored per user, or per layout builder?
   • Looking back over this issue, it hasn't been discussed here, but the patches have implemented the UI preference on a per-layout basis. I've seen local storage keys for several default layouts and node-specific overrides.
   • When we discussed this with the Web Access team, we didn't reach a clear conclusion. However we noted that a user's needs (their disabities, assistive tech, and other technology choices) don't change simply because they go to edit a different page.
   • So the option we were leaning most towards, was storing it per-user. The reasoning was that if they have indicated they want a particular UI configuration, the most agreeable thing to do was respect that until they indicate otherwise. Coincidentally, this would be the simplest to implement.
   • If the UI preference is stored on a per-layout basis, then the user has configure the layout builder preferences every time they want to deal with a new page. When there are a few more UI preferences (like "show section and region headings") the user will be expected to jump through a lot of hoops to keep the UI how they like it.
   • The possibility of storing it per-user and per-device was discussed. Although we didn't expect a user's needs to change between editing different pages, we did note that editors may use multiple devices during the course of a day, and having different needs per device sounded likely. For example a partially sighted user may employ a screen magnifier and multiple monitors at their main desktop machine, but prefer a screen reader when using a laptop elsewhere. Using local storage achieves this with no special logic required.

#3019487: [plan] Describe layout builder UI to assistive technology most certainly should be a stable blocker, so I've tagged it as such. It was already the child of a stable blocker.

I'm caving on #100. You want it fixed, here's the fix.

I agree on #107. This switches to storing the preference per-user, per-device. This matches how the toolbar orientation is stored.

Are you suggesting that the announce calls be removed?

No, I'm encouraging others to keep thinking about what actually happens when the checkbox is used. The suggestions so far were a bit abstract, in that they said something about what "mode" it was in, but didn't really say what difference that makes.

My suggestions are:

• "Block previews are visible. Block labels are hidden."
• "Block previews are hidden. Block labels are visible."

Thanks, I'll try patch #109 soon.

Thanks for digging up the explanation from #39.2. I was watching a variety of custom blocks in the browser dev tools, but there's no accounting for custom templates. If you've all agreed that's awkward, I'll buy that.

The accessible names in #3019487: [plan] Describe layout builder UI to assistive technology could come from aria-label or aria-labelledby. When the accessible name is visible (or, sometimes visible) the latter is preferred because it's only one string instance to maintain. However, either will work just as well for assistive tech goes. I'll note that in the other issue.

A simple fix to the placeholder string would be:

from:
$preview_fallback_string = $this->t('Placeholder for the "@block" block')

to:
$preview_fallback_string = $this->t('"@block" [Placeholder]')

This solves several problems cited above:

1. It's shorter, fitting better on the screen
2. Screenreaders will get to the name of the block faster, without have to read a bunch of repeating text first
3. It still conveys that this is not the way it will look on the front end, it's just a placeholder, which may not be clear if it was just the name of the block

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,134 @@
   +      // Tracks if live preview is enabled for this layout. Defaults to true
   +      // if no value has previously been set.
   

   Multi line comment format.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,134 @@
   +      function enableGridMode() {
   

   No `function declaration`. Also probably needs updated jsdoc.

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,134 @@
   +        // Iterate over all Layout Builder blocks to hide their content and
   +        // add grid mode labels.
   

   Multiline comment format.

4. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,134 @@
   +          for (let index = 0; index < element.childNodes.length; index++) {
   

   Use an iterator.

5. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,134 @@
   +      function disableGridMode() {
   

   No `function declaration`. Also probably needs updated jsdoc.

6. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,134 @@
   +        const $target = $(event.currentTarget);
   

   This is only used once, probably don't need to assign a variable.

7. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,134 @@
   +      if (isLivePreview === false) {
   

   You can probably use `!` here.

This patch addresses the JS feedback in #114, the failing test in #109, and some refactoring to reflect the change from the name "live preview" to "content preview". This patch does not address anything related to the a11y items currently being discussed.

Point-by-point for #114

1. Multi line comment format.

   Changed to multi line

2. No `function declaration`. Also probably needs updated jsdoc.

   Switched to arrow functions and updated doc.

3. Multi line comment format.

   Changed to multi line

4. Use an iterator.

   Unfortunately, I discovered the forEach() I'd been using is not recognized by IE11, despite what the docs say.
   Switched to $.each (if theres an IE11-accepted vanilla iterator you recommend, that works for me as well)

5. No `function declaration`. Also probably needs updated jsdoc.

   Switched to arrow functions and updated doc.

6. This is only used once, probably don't need to assign a variable.

   got rid of the variable assigning.

7. You can probably use `!` here.

   Tested a few different scenarios as confirmed that !isLivePreview works just fine.

Unfortunately, I discovered the forEach() I'd been using is not recognized by IE11, despite what the docs say.

Hmm we definitely use it plenty of places in core. I think IE11 should be using the transpiled ES5 JS though, no?

IE 11 supports Array.prototype.forEach(), (MDN web docs). The problem here is that element.childNodes is an instance of NodeList, which IE 11 doesn't support iterating using .forEach() (MDN web docs).

ES5 transpilation doesn't help us on inconsistencies that are outside of the ES6 scope. For those inconsistencies, we have to use other workarounds such as polyfills.

#117

The problem here is that element.childNodes is an instance of NodeList, which IE 11 doesn't support iterating using .forEach()

Thank you @lauriii! It is now using forEach() with the IE compatible approach of using Array.prototype.forEach.call. I also checked to see if this approach was used anywhere else in core, and it is also used in /core/misc/message.es6.js

👍 I forgot to mention the pitfall of `nodeList`. Nice catch there. These changes look good to me.

@lauriii pointed out that since nothing user-facing references "grid mode" anymore, the patch should be refactored to reflect that.
We + @timplunkett discussed the approach and decided on naming based on content preview being enabled/disabled.

Also refactored anything referring to "grid mode labels" to be "placeholder labels".

The patch should no longer have any references to live preview or grid mode.

Fixed two code standards issues. Including an interdiff from #118 as well in case that makes reviewing easier.

#110 suggests new strings for some Drupal.announce() calls. I believe that is the only remaining thing that needs addressing in this issue

Added the suggested Drupal.announce() calls from #110 - thanks for spotting that @timplunkett!

To the best of my knowledge, that was the only outstanding thing that needed changing, and it follows @andrewmacpherson's suggestions exactly. Thanks @bnjmnm!

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -201,4 +201,158 @@
+          '.js-layout-builder-block__content-preview-placeholder-label',
...
+      'layout-builder-block__content-preview-placeholder-label js-layout-builder-block__content-preview-placeholder-label';

js- prefixed classes shouldn't use the BEM convention. This could become just js-layout-builder-content-preview-placeholder-label.

Fixing #125

Thanks!

Tested manually; this is definitely a helpful feature. Embedding a screenshot of what it looks like when the content preview is disabled.

This is still tagged for accessibility review. I think @andrewmacpherson's points have all been addressed (either here or with followup issues), but left a ping asking for confirmation.

Oops, looks like this needs a reroll.

Straight reroll, just broke on git fuzz after #2995689: Allow reordering blocks without a pointer device

MySQL server has gone away

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -304,4 +304,158 @@
   +              if (node.nodeType === 3 && node.textContent.trim()) {
   

   any reason not to use Node.TEXT_NODE here, instead of the magic 3? Note also https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType lists support for .nodeType as unknown for many browsers we target (not sure that list is up to date, because it works for me in Firefox)?

2. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -119,8 +122,19 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +      // @todo Use new label methods so data-layout-content-preview-placeholder-label doesn't
   

   nit > 80 here

Re #134

1. any reason not to use Node.TEXT_NODE here, instead of the magic 3?

   good call node.nodeType was just force of habit. Node.TEXT_NODE is easier to read so I made the change

2. Fixed nit

#134 +1 to using the constant. I think the table must be out of date because I tested manually with Firefox and IE 11 and both of them seem to support that property. There are also some other references such as W3 Schools listing it as supported by those browser

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -304,4 +304,156 @@
+              if (node.TEXT_NODE === 3 && node.textContent.trim()) {

The condition should be node.nodeType === Node.TEXT_NODE.

Changed conditional per #136

Also verified this is supported on IE11, Chrome, Firefox, Safari, and Opera.

Well spotted, thanks!

1. For some reason, #120 reverted some changes made in #109 to address #107.
2. Do we have sufficient test coverage given that #135 didn't fail?

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -304,4 +304,156 @@
   +             * If the immediate child of an element is a text node, it won't be
   +             * included in the hide() function. Save the value with data() so
   +             * it can be used later, then remove from the DOM.
   

   This could use an explanation why this is needed.

4. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -304,4 +304,156 @@
   +    $element.prepend(contentPreviewPlaceholderLabel);
   

   This theme function isn't following the theme API (see documentation for Drupal.theme). Instead of modifying the element here, let's return the themed content here and place the markup to the element in the calling function.

5. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -304,4 +304,156 @@
   +    $element.prepend(contentPreviewPlaceholderLabel);
   

   While prepending the label to the existing block might be the best solution for now, we might want to look into moving it into a completely new element in the future instead to make it easier for theme to support layout builder. Let's open follow-up for this.

Re: #139

1. In a discussion with @lauriii we confirmed that this was not the case and the changes in #109 were not reverted.

2. This could use an explanation why this is needed.

   Instead of an explanation, I removed it entirely. Apparently something changed between when this was implemented and now -- I added/togged every possible block and the problem this chunk of code addressed is no longer happening. Every first-child of layout builder blocks are wrapped in a tag, so there's no text nodes to be concerned with.

3. I removed it instead per the changes mentioned in 2 ^^^
4. The function now returns an html string.

6. While prepending the label to the existing block might be the best solution for now, we might want to look into moving it into a completely new element in the future instead to make it easier for theme to support layout builder. Let's open follow-up for this.

   I gave this one more shot just in case, but there were definite obstacles. I described them in the followup #3043215: Create non-js placeholder element for content-preview-disabled placeholder labels.

Thank you for making all those changes @bnjmnm! This is starting to look good for me. All I could find at this point:

1. Error: Call to a member function id() on null in Drupal\layout_builder\Element\LayoutBuilder->buildContentPreviewToggle() (line 166 of core/modules/layout_builder/src/Element/LayoutBuilder.php).
   

   Since we've changed the Drupal\layout_builder\Element\LayoutBuilder::__construct parameters, we should add empty post update hook to avoid this error.

2. 
   The region text looks very similar to the blocks when moving blocks using the interface added in #2995689: Allow reordering blocks without a pointer device. In my opinion, this doesn't have to block this issue since there's a difference between font sizes and a text for identifying regions. However, I think it would be good to open a follow-up issue for this since we could probably make it even clearer.