Interactive controls inside preview block in the Layout Builder form should be disabled.

Title change?

+++ b/core/modules/layout_builder/js/layout-builder.js
@@ -11,7 +11,11 @@
+      $(context).find('.layout-builder--lay     out__region').sortable({

What's going on with this additional space in the selector?

@drpal - I think it's a miss from @tedbow so doing changes to previous patch because there is no update on this issue from last 4 days. also added an interdiff to highlight changes.

I think the patch looks, just a quick thought:

Can't we add context to the event listeners for slightly better performance when LB is re-rendered through AJAX?

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -1,6 +1,10 @@
+      $('[data-layout-block-uuid]', context).children().on('click mousedown', (e) => {
+        e.preventDefault();
+        e.stopPropagation();
+      });
       $(context).find('.layout-builder--layout__region').sortable({

Somewhat interesting that we do this two different ways. Maybe a followup to figure out which one is more 🎉

I tested this and it works well even if you tab around the form and use the return key. One thing I'm wondering is accessibility. Do we need to announce that the action is disabled in some way? Or would that as annoying as a javascript alert message that informs the users the click has been ignored. Maybe what we should do is add something to the help to tell the user that links and forms in the blocks will be disabled in the preview - i.e. add something to layout_builder_help().

I think it would be best to disable all interactive controls, not just forms and links. Videos, embedded Prezi slideshows, social share widgets, could all have the potential to take you away from the layout builder.

+      $('[data-layout-block-uuid]', context)
+        .children()
+        .on('click mousedown', e => {
+          e.preventDefault();
+          e.stopPropagation();
+        });

Is "click mousedown" going to be enough? Especially given that we have a "mouseup" in our ajax.js and third-party embeds have who-knows-what. I don't know how robust we can be, but I think we should consider all the mouse*, pointer*, and touch* events here.

Aside: we'll need to replace "mousedown" in core with "mouseup" in lots of places for #2958654: Assess JavaScript behaviours for WCAG 2.1 Pointer Cancellation

+       * Remmove from the tabbing order all input elements and elements

Nit: "remove".

+      containingElements
+        .find(
+          'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"]):not(.focusable)',
+        )
+        .attr('tabindex', -1);

The .focusable selector is from jQueryUI, right? There's also .tabbable which might be more readable here.

Will these approaches be effective at disabling controls inside an iframe?

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -49,6 +49,37 @@
   +      $blocks.find('a').on('click mouseup touchstart', e => {
   

   What specifically are you trying to capture? All interaction events?

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -49,6 +49,37 @@
   +        typeof context.closest === 'function' &&
   

   You should be careful here. If `context.closest` is somehow a `String`, `Date` or `Object`, `typeof` will tell you they are functions.

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -49,6 +49,37 @@
   +        context.closest('[data-layout-block-uuid]')
   

   There's a warning about `context.closest` needed to be destructured.

@tedbow - The changes look good. I would just want to make sure we are capturing all the input events we need to from @andrewmacpherson's comment in #34.

Added JS to make iframes non-interactive
Added pointer-events: none; to preview CSS so cursor/hover behavior does not act as if items are clickable.

Refactored tests to account for this CSS change - now we check for non-clickability vs. the previous check for default-click behavior being suppressed.

Checked video & audio elements and concluded no changes were necessary - while its possible play those media items, none of the controls take the user away from Layout Builder or alter anything on the site.

Patch addresses feedback from @tedbow in #42 (including refactoring assertElementUnclickable() - much nicer.

Went with the important! approach for the mask + container. I'm inclined to think this is one of those legitimate uses of !important despite it being one of the most commonly discouraged anythings in web development.

Re: #45 (2): I provided a test-only patch.

Also a small change to the #43 patch - added fail message to click test.

Thanks for the patch. I have reviewed and found only very minor things including one typo (see below). I will try to test this.

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -56,6 +56,22 @@
   +.layout-section .layout-builder--layout__region .preview-mask-container {
   +  position: relative!important;
   

   Nitpick: Other parts of core have a space before !important.

2. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -56,6 +56,22 @@
   +  position: absolute!important;
   +  width: 100%!important;
   +  height: 100%!important;
   +  margin: 0!important;
   +  padding: 0!important;
   

   Same as above.

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -49,6 +49,52 @@
   +      /*
   +       * Disable interactive elements in Layout Builder Preview
   

   Nitpick: No period at end of sentence.

4. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -49,6 +49,52 @@
   +      /*
   +      * Remove from the tabbing order all input elements and elements
   +      * specifically assigned a tab index.
   +      */
   

   Nitpick: Alignment of * is off.

5. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -49,6 +49,52 @@
   +      /*
   +      * Mask iframe with trasparent div to prevent clicking.
   +      */
   

   "trasparent" is a typo.

   Nitpick: Same as above.

6. +++ b/core/modules/layout_builder/js/layout-builder.js
   @@ -30,6 +30,30 @@
   +
   +      containingElements = containingElements.add($(context).closest('[data-layout-block-uuid]').children().not('[data-contextual-id]'));
   

   Nitpick: Consider formatting so it's easier to read.

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
   @@ -0,0 +1,185 @@
   +use Behat\Mink\Element\NodeElement;
   

   Nitpick: Should be first in the list.

8. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
   @@ -0,0 +1,185 @@
   +    'layout_builder',
   +    'block',
   +    'node',
   +    'search',
   +    'filter',
   +    'filter_test',
   +    'block_content',
   

   Nitpick: Make alphabetical?

9. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
   @@ -0,0 +1,185 @@
   +    //   https://www.drupal.org/project/drupal/issues/2917777.
   

   Nitpick: Remove extra space before https?

10. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
    @@ -0,0 +1,185 @@
    +    // Add a block with a link, another with a form, and one with an iframe.
    +    $this->addBlock('Search form', '#layout-builder .search-block-form');
    +    $this->drupalGet("$field_ui_prefix/display-layout/default");
    +    $this->addBlock('Block with link', '#link-that-should-be-disabled');
    +    $this->drupalGet("$field_ui_prefix/display-layout/default");
    +    $this->addBlock('Block with iframe', '#iframe-that-should-be-disabled');
    

    Nitpick: The order in the comments is not the same as the code. Comment has link, form, and then iframe. Code has form, link, and then iframe. Would be good if they matched.

11. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
    @@ -0,0 +1,185 @@
    +      $this->fail(format_string('@tag_name was clickable when it shouldn\'t have been', ['@tag_name' => $tag_name]));
    

    Nitpick: Consider double quotes around string to avoid escaping quote since double quotes aren't used in the string.

12. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
    @@ -0,0 +1,185 @@
    +   * Asserts that forms, links and iframes in preview are non-interactive.
    

    Nitpick: Use Oxford comma.

I have tested the patch from #46 by:

1. Enabled layout builder modules
2. Configured layout builder on page content type
3. Added link and iframe to body text of node 1
4. Go to layout page
5. Both link and iframe controls are disabled

Marking back to "Needs work" in case any changes from previous comment should be done.

Thank you for the thorough review & testing @kristen-pol
Items in #51 are addressed other than ones commented on below
6) core/modules/layout_builder/js/layout-builder.js is automatically generated from core/modules/layout_builder/js/layout-builder.es6.js so it's not a change that can be made. The .es6.js file is fair game though and should be formatted with readability in mind.

9) There isn't (afaik) a firm coding standard for inline comments using tags, but I prefer using the indentation so IDEs format the additional lines as part of the @todo statement.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -49,6 +49,52 @@
+      containingElements = containingElements.add(

I wonder if this makes sense. Could we possibly update the previous selector to provide an equivalent result?

Additionally, from #53, formatting is not fair game. We have a pipeline which includes Prettier for a reason. Having a wild-west environment where every file is formatted differently makes it quite difficult to parse unfamiliar files.

Summary of changes in this patch

1. #53 By adding context to the creation of $blocks, I was able to remove everything related to containingElements and just work with $blocks. Despite the tests passing, I was still concerned that I overlooked the use case that promoted containingElements to be distinct from $blocks, so I did some manual sanity-checking as well. This included taking into account pre and post Ajax requests, using quickedit, mouse & tab navigation, and making sure LB contextual links continue to work. All seems fine?
2. Removed everything related to masking the iframe with a transparent div in JS and CSS. I realized it was unnecessary once pointer-events: none;was added. This successfully prevents mouse interactions, and the setting of tabindex to -1 prevents keyboard interactions.
3. Rephrased a few comments in the JS.

Discussed #56 with @tedbow and we agreed it needed a few more changes. A revised patch will arrive shortly.

@tedbow explained the use case I was wondering if I overlooked in #56.1
containingElements was added to catch any AJAX changes applied directly to a block's content from other modules.
We realized that the same could be achieved by removing context from the creation of $block.
To mitigate the performance hit of removing context, the Layout builder ID was added to the selector
const $blocks = $('#layout-builder [data-layout-block-uuid]');

I tested #58 using the search form, links, and an iframe. Interactivity for all of these was disabled.

I reviewed the interdiff and, while I don't see anything obviously wrong, it would be good if @tedbow could confirm the changes are as expected. If so, this looks RTBC to me.

Split JS into multiple behaviors & added JS Documentation.

Checked that #61 still works and it does. Someone with more js chops will need to verify the interdiff is ok.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -1,5 +1,19 @@
+(($, Drupal) => {
+  const { ajax, behaviors, announce } = Drupal;
+  /**
+   * Provides the ability to drag blocks to new positions in the layout.

There's a missing blank line before the docblock. Was this eslinted? (It could be that the blank line just isn't in the AirBnB standards, but I've not seen this before in our JS.)

Addressed #66.1

Patch addresses the goof spotted in #68.

Reroll 🥖

Does this need another round of manual testing?

Just in case, I did and it is working. Marking RTBC.

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -56,6 +56,10 @@
   +.layout-section .layout-builder--layout__region .block [tabindex="-1"] {
   

   This selector only works with themes that extend Classy. We should add class to blocks that are rendered inside the layout builder editor.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -160,4 +160,37 @@
   +      // Don't allow links to be used as a sortable selection.
   +      $('.layout-builder--layout__region').sortable({ cancel: 'a' });
   

   Why is this needed?

1. #75.1 Very good catch, added new class to these blocks.
2. #75.2 Removed this after confirmation from @tedbow that it is no longer required.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -160,4 +160,34 @@
+    attach(context) {

You're passing `context` here but you never use it inside the function.

#80 addressed by removing `context`

For #79 I had to address the nonfuncitoning contextual links differently than suggested - this solution would make the contextual links clickable, but they would still not be tabbable. This problem was probably not notices sooner as it requires a specific order events. Very pleased @tedbow discovered it. The easiest way to reproduce is go to the layout builder UI and refresh the page. On a refresh, it looks like contextual links init and behaviors.layoutBuilderDisableInteractiveElements happen in a different sequence and the contextual links are disabled.

Reproducing this in tests was not as simple as refreshing the page, but I think I found steps to reliably reproduce the issue. I attached x25 and x25-FAIL versions of the patch to confirm this.

Adding the new block class required updating some unit tests.

It seems like #75.2 is now relevant because contextual links can be used and shouldn't start sorting.

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -56,6 +56,10 @@
+.layout-section .layout-builder--layout__region .layout-builder--block [tabindex="-1"] {

+++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
@@ -106,6 +106,7 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
+        '#attributes' => ['class' => ['layout-builder--block']],

We shouldn't use the modifier pattern here.

Let's also make sure that follow-up gets opened for #75.2.

1. #88 Modifier pattern removed
2. #89 Created #3033153: Contextual links can be used to drag and drop Layout Blocks. for the symptoms discussed in #75.2

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -55,6 +55,10 @@
+.layout-section .layout-builder--layout__region .layout-builder-block [tabindex="-1"] {

Do we still need .layout-section and .layout-builder--layout__region in the selector?

Re #92

Do we still need .layout-section and .layout-builder--layout__region in the selector?

Good call. These are no longer necessary and they've been removed.

These became unnecessary in the rule above it as well

.layout-section .layout-builder--layout__region .block {
  padding: 1.5em;
}

was changed to

.layout-builder-block {
  padding: 1.5em;
}

And these were moved in the file as to not interrupt the grouping of .layout-section .layout-builder--layout__region rules.

@tedbow spotted an issue with the patch - will upload a corrected one

This is identical to the patch actually uploaded in #93, so the interdiff is from #90

Them being identical is only apparent if you view the patch with a cache busting query. Visiting the URL directly gets a cached version that was removed before saving #93. Re-uploading the patch and interdiff with new filenames.

Looks good! Thank you everyone!

Committed ef68ad6 and pushed to 8.7.x. Thanks!

fixing accessibility tag