Highlight active element while working with dialogs in Layout Builder

Here's an attempt at this - I added the same "highlight" effect to blocks, which felt more consistent.

Functionality look good to me.

Now that #2978964: Use Prettier for formatting core JavaScript is fixed we need to run
yarn run prettier
on all js patches.
After running this that
yarn run lint:core-js-passing
Now passes

There might be an accessibility dimension to this, I'm still thinking about it. The notion of "grey out" suggests some information is being conveyed by colour alone, so this idea would need to address WCAG SC 1.4.1 Use of Color (level A).

Can we add some indicator to emphasize the relevant area itself, rather than merely de-emphasising the irrelevant areas?

Update: I mean a non-colour indicator for the relevant area. An icon, or a change in border style perhaps.

Addressing #10.

Changed the outline and made the text bolder in the link.

Here are screenshots

I like #12 - that's the sort of thing I had in mind.

• It provides extra cues to identify the relevant region.
• It's more robust with respect to ambient light, e.g. working near a window.
• The change from a dashed outline to a solid one will carry over into Windows high-contrast mode, when the colours are all overridden.
• It may also be picked up by magnifier users, including some who have the magnified and un-magnified versions side by side (e.g. on two monitors). Magnifier set-ups vary greatly, but I expect this extra clarity will help some users.

RTBC-worthy from the screenshots in #12, but I'll leave the code review to others more familiar with the layout builder controllers and JS.

1. @andrewmacpherson thanks for the detailed explanation in #13 about the advantages of the current approach!

2. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -183,6 +183,12 @@ protected function buildAddSectionLink(SectionStorageInterface $section_storage,
   +        'data-layout-builder-highlight-id' => implode('-', [
   +          'section',
   +          $storage_type,
   +          $storage_id,
   +          $delta,
   +        ]),
   

   I don't for the data-layout-builder-highlight-id we actually need the storage_type and the storage_id.

   The only case I could see for this is you have 2 layout builder administrations sections open on the same page at the same time. Meaning you configuring 2 separate layouts at once. I assume this is not something we support.

   If we remove the extra elements in the array I think we can just use string concatenation.

3. Adding test that highlight class is added

JavaScript here is 👍.

One thing to consider for future development would be to consider moving away from jQuery unless we absolutely need to use some of its features.

This looks good to me for CSS and should be a nice UX improvement.

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -114,3 +114,11 @@
   +[data-layout-builder-highlight-id].layout-builder-highlight {
   

   Any particular reason why this couldn't be just .layout-builder-highlight?

2. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -114,3 +114,11 @@
   +  z-index: 1;
   

   Not sure if this is the correct z-index. For example, contextual links are now rendered on top of this.

3. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -114,3 +114,11 @@
   +  font-weight: bolder;
   

   Is there a clear benefit in setting this? This font-weight is not commonly used in Drupal core and might be something that doesn't work very well with custom themes since they don't necessarily have font loaded for this weight.

4. Currently, the overlay doesn't prevent interacting with the site. Is that intentional? I expected clicking on the overlay would lead into moving back to the main canvas.

@lauriii thanks for the review

1. The current is more specific and the functionality won't work if the attribute [data-layout-builder-highlight-id] was on the element too. I guess this also avoids accidently conflicting with a a theme is already working with the layout builder and is adding their own class .layout-builder-highlight.
2. The contextual links will be highlight if you hoover them or if you are in "edit" mode. I think this is correct because we should not be stopping you from doing any other actions just focusing your attention on the location where the section or block will be placed.
3. I wasn't aware Drupal core doesn't usually use this. I removed it and I think the effect still works.
4. Yes I don't think want to stop the user from taking any other action. We are just helping make the connection with the new section or block that will be added and the location it will be added. If the user decides they don't want to complete the action they can still directly take another action in the layout builder. Or close the dialog to unhighlight and then take the action.

The current is more specific and the functionality won't work if the attribute [data-layout-builder-highlight-id] was on the element too.

Could you elaborate since I don't think I quite understand this?

I guess this also avoids accidently conflicting with a a theme is already working with the layout builder and is adding their own class .layout-builder-highlight.

I don't think we should worry about this given that the module is in alphabeta stage. Also, this is not in Stable theme so we are allowed to make changes freely.

We should also update the issue summary with the approach taken on the overlay and get UX review specifically on that since it works differently than other overlays in Drupal.

From #10,

There might be an accessibility dimension to this, I'm still thinking about it. The notion of "grey out" suggests some information is being conveyed by colour alone.... Can we add some indicator to emphasize the relevant area itself, rather than merely de-emphasising the irrelevant areas?

Similar concerns were brought up at the Tues Jan 15 UX meeting. Attached are a few variations highlighting the relevant area without any grey-out

• 2994909-21-highlight-only-do-not-test.patch is identical to the patch in #18 but with the grey-out removed
• 2994909-21-highlight-slightly-louder-do-not-test.patch builds on the above patch, but with some subtle (I hope) additional styling to the active area to help indicate its relevance.
• 2994909-21-highlight-slightly-more-louder-do-not-test.patch builds on the above patch, with slightly more aggressive css

@bnjmnm thanks for the extra patches for the variations. Could attach screenshots that show the differences. Thanks

@tedbow Here are screenshot for each version. 2994909-21-slightly-louder-plus-one has a bit of css animation so I provided an animated gif for that one.

1. 2994909-21-highlight-only-do-not-test.patch
   

   +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -114,3 +114,9 @@
   +  position: relative;
   +  z-index: 1;
   

   Would we still need these if we don't have overlay? Presummable nothing should be on top of the add section and block divs anyways.

2. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -50,6 +50,7 @@
   +  transition: outline .7s ease;
   
   @@ -114,3 +115,15 @@
   +  transform: scale(1.005, 1.005);
   

   I would vote against the animation in any of the solutions

Oh no! these already need rerolls

reroll of #21

Thanks for the rerolls @kostyashupenko
I've hid the two variants we definitely wont be going with per #24 (2)

However, think we may need to explore something other than the current highlight-only approach. I tested this with a FE color scheme I used on a clients site last year. Unfortunately, the highlighting is almost indistinguishable from non-highlighted. I attached a gif demonstrating this.

The initially proposed gray-out approach would work with darker themes, but the reasons for moving away from this are valid ones.
I'll think over possible solutions that avoid the drawbacks of gray-out, while also accommodating a wider range of FE theme colors. Very interested in hearing suggestions.

Discussed #26 outside of this issue and the conclusion was addressing darker color schemes is not in scope for this issue. Will provide a patch refining the approach selected in #24

Re:
#24.1 The z-index and position are still necessary for the black outline to fully replace the blue dashed one.

#24.2 Excellent selection, that's what we'll go with.

Patch also has a tiny JS optimization and a nit fix.

Patch looks good. There's test coverage, minimal changes, and everything that's going on is pretty clear. I like it! However, I do think this issue could use some before/after screenshots, so I'm tagging it for that. Beyond that, I have two small points of feedback:

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -51,4 +51,18 @@
   +    const id = $element
   +      .find('[data-layout-builder-target-highlight-id]')
   +      .attr('data-layout-builder-target-highlight-id');
   +    if (id) {
   +      $(`[data-layout-builder-highlight-id="${id}"]`).addClass(
   +        'layout-builder-highlight',
   +      );
   +    }
   

   This logic is confusing. Can it have a comment explaining what it's doing and, more importantly, why?

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -92,4 +92,29 @@ protected function assertModifiedLayout($path) {
   +    $this->assertCount(1, $page->findAll('css', '.layout-builder-highlight'));
   

   Pro-tip: I believe you could use $assert_session->elementCount() for a slightly cleaner assertion here, or anywhere you're asserting that some number of elements match a selector.

Patch addresses:
#29.1 - added some detailed comments so it's actually clear what the JS does.
#29.2 - nice protip, changes made

I also spotted & addressed an additional UX issue in the dialog:aftercreate event listener. In some cases, the target element is scrolled out of view when the off canvas dialog appears. highlight-out-of-viewport.gif shows this occurring.

The solution in action can be seen with highlight-inside-viewport.gif

Screenshots:

Here's "Add Section" before the patch

Here it is after

Thanks for the updates. I noticed some very minor things. I'll try to test it now.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -51,4 +51,53 @@
   +  // Listener to highlight active Add Block/Add Section elements.
   

   Nitpick: For readability, perhaps:

   Add Block/Add Section => "Add Block" and "Add Section"

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -51,4 +51,53 @@
   +     * data-layout-builder-highlight-id, which identifies its
   +     * unique delta/region.
   

   Nitpick: 80 char wrapping.

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -51,4 +51,53 @@
   +     * data to create a data-layout-builder-target-highlight-id attribute.
   +     * The value of this attribute matches the data-layout-builder-highlight-id
   

   Nitpick: 80 char wrapping.

4. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -51,4 +51,53 @@
   +       * target element is still visible in viewport. If not, scroll page
   +       * so target element is visible in its pre-click position.
   

   Nitpick: 80 char wrapping.

Nits in #31 addressed, TY @kristen-pol

Changes in interdiff look good. I tested it and the Add Section box is highlighted as expected. Marking RTBC.

Embedding the SS in the IS.

Sorry to bump this back but it seems like #17.1 is still unresolved.

Discovered that highlight disappears when adding blocks/sections that require more than one dialog. For example, when choosing a Two Column layout, the highlight disappears when the column width options are loaded. Adding the failing patch here for now as I wanted to get this visible without waiting on a fix.

This patch is also a reroll.

This patch addresses additional use cases identified while testing

1. As mentioned in #40, highlighting is not maintained if block/section configuration requires consecutive dialogs.
2. No highlighting occurs when removing or configuring sections
3. When configuring a block via contextual links, the nearest "Add Block" is highlighted, which is not the triggering element.

To address all the above issues, I added the data-layout-builder-target-highlight-id attribute to all (?) of the dialogs triggered by the layout builder UI. Added data-layout-builder-highlight-id attributes to existing blocks and sections, previously this attribute was only present for "Add Block" and "Add Section".

Tests were expanded to cover all the scenarios mentioned.

IS needs updating to reflect that highlighting should be added to all dialog-opening inputs, not just "Add Section"

Here are screenshots of the new areas receiving highlights. I'm also wondering if it would be beneficial to use a different outline color when "Remove" is selected, to make it more apparent what is being removed - particularly since the confirmation dialog does not visibly specify the item being removed.

Remove Section

Configure Section

Remove Block

Edit Block

1. +++ b/core/modules/layout_builder/src/Form/ConfigureBlockFormBase.php
   @@ -180,6 +181,9 @@ public function doBuildForm(array $form, FormStateInterface $form_state, Section
   +    if (!isset($form['#attributes']['data-layout-builder-target-highlight-id'])) {
   +      $form['#attributes']['data-layout-builder-target-highlight-id'] = "block-$delta-$region";
   +    }
   

   Do we actually need to check if it is set? Would we ever want to preserve a different value? Could we just always set it?

2. Looking now at the number of places we are setting data-layout-builder-target-highlight-id I wonder if at this point it would make sense to have highlightIdTrait with just getHightLightId()
   Where maybe you send it in a array with the keys you have and it creates the id for you.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
   +    $this->assertNotEmpty($assert_session->waitForElement('css', '.new-section.layout-builder-highlight[data-layout-builder-highlight-id="section-0"]'));
   

   I think you should also wait for off-canvas or some elemet on it to be visible. To avoid random test fails.

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
   +    // Highlight is present with AddSectionController.
   

   need blank light before comment

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
   +    $this->assertHighlightedElement('new-section');
   

   This confirm there is only 1 highlight section and it has the class *new-section* but there are 2 new-section elements on the page. I don't think it confirms the correct one is highlight. Maybe we should switch to sending in a selector instead of a class.

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
   +    // Highlight is present with ConfigureSectionForm.
   

   need blank light before comment

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
   +    // Highlight is present with ChooseBlockController::build().
   

   need blank light before comment

8. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
   +    // Highlight is present with ChooseBlockController::inlineBlockList().
   

   need blank light before comment

9. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
   +    $page->pressButton('Close');
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $assert_session->elementsCount('css', '.layout-builder-highlight', 0);
   

   I am wondering if it would be safer to copy waitForNoElement()(with the todo) from one of the other layout_builder js test. Then we can wait to explicitly that for the highlight to be take away and nto have to worry about timing issues.

   Probably also want to wait for no off-canvas dialog. before proceeding with test.

10. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
    +    // Highlight is present with ConfigureBlockFormBase::doBuildForm.
    

    need blank light before comment

11. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
    +    $page->pressButton('Close');
    +    $assert_session->assertWaitOnAjaxRequest();
    

    Here we should allow wait for dialog to close(noElement). I don't think there is any ajax request on close.

12. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
    +    // Highlight is present when Remove section dialog open.
    

    need blank light before comment

13. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
    +    $this->clickContextualLink('.block-field-blocknodebundle-with-section-fieldbody', 'Remove block');
    

    I think because of #2918718: Using ContextualLinkClickTrait::clickContextualLink() multiple times per page can cause random failures we want to reload the page before the second use of clickContextualLink() here.

14. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +124,108 @@ protected function assertModifiedLayout($path) {
    +    $page->pressButton('Close');
    +    $assert_session->assertWaitOnAjaxRequest();
    +    $assert_session->elementsCount('css', '.layout-builder-highlight', 0);
    

    waitForNoElement()

This patch addresses everything in #45, created a trait with four methods for generating a highlight ID, to cover the add & update scenarios for both sections and blocks.

Also added a x25 test patch because these UI-heavy tests (particularly ones with contextual links) seem prone to random failures.

This required a reroll, but the interdiff provided is pre-reroll, in case that aids the review process.

Does this need manual testing?

@kristen-pol - thank you for asking! Manual testing would be great, particularly since the highlighting has been expanded to include existing blocks and sections, not just "Add Block" and "Add Section"

One of the things I'm most interested in is any scenarios where it seems like there should be focus and there isn't, especially since it took us a while to identify the need for highlighting existing Blocks/Sections.

Also interested in feedback about making the outline for "Remove Block"/"Remove Section" red instead of black. On the plus side: it could add a little gravitas to a potentially regretful operation -- particularly since currently it's kind of easy to mistakenly remove a section when it looks like it should just remove a block. On the down side: it deviates from the highlighting pattern people will be accustomed to at that point.

putting this back on my list, the screenshots after #30 look significantly different to what I reviewed in #13

♻️🥖 Reroll

Can somebody summarize the design changes since the accessibility review in #13, and the reasons for them? I'm lost.

Can somebody summarize the design changes since the accessibility review in #13, and the reasons for them? I'm lost.

That is a very reasonable request, @andrewmacpherson. The discussions that led to the changes since #13 took place at weekly UX meetings -- very little of that is documented here and it should have been.

My recollection of the the primary concerns about the gray-out approach:

• Visually, the interaction is very similar to that of using a modal despite all grayed elements on the page remaining active. Making this actually function as a modal was ruled out fairly quickly -- the reason I recall is the complexity to both the user and developer as the existing use of off-canvas means dealing with multiple interacting dialogs in the same interface
• The grayed parts of the page, despite being active elements, are difficult for some users to see. Tab navigation the grayed elements can be difficult as well. For example. the contrast ratio of the gray-out and Chrome's default focus outline is only 1:40 to 1

(if this seems incomplete I can review the UX meeting recordings)

Several different approaches were discussed, and the agreement landed on highlighting the element that corresponds to the active off-canvas dialog with a 3px black outline.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,139 @@ protected function assertModifiedLayout($path) {
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $this->assertNotEmpty($assert_session->waitForLink('Create custom block'));
   

   waitForLink() doesn't wait for visibility so usually I do something like
   $this->assertNotEmpty($assert_session->waitForElementVisible('css', 'a:contains("Link test")');

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,139 @@ protected function assertModifiedLayout($path) {
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $this->assertNotEmpty($assert_session->waitForLink('Recent content'));
   

   Same here

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,139 @@ protected function assertModifiedLayout($path) {
   +    $assert_session->waitForElement('css', "#drupal-off-canvas");
   

   This doesn't assert it is not empty.
   Also should use waitForElementVisible() instead.

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +124,139 @@ protected function assertModifiedLayout($path) {
   +    $assert_session->waitForElement('css', "#drupal-off-canvas");
   

   This doesn't assert it is not empty.
   Also should use waitForElementVisible() instead.

Updated tests per review in #55

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -203,17 +203,17 @@ public function testAddHighlights() {
   -    $assert_session->waitForElement('css', "#drupal-off-canvas");
   +    $assert_session->waitForElementVisible('css', "#drupal-off-canvas");
   

   This should still be wrapped in $this->assertNotEmpty()

   There is another instance like this in the test.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -160,4 +160,55 @@
+       * Wait 700ms for the off canvas dialog to fully appear. Then see if
+       * target element is still visible in viewport. If not, scroll page so
+       * target element is visible in its pre-click position.

I think this should say "to appear and for the page to be sized"

Isn't the resizing of the page the reason the element might get pushed out of the ViewPort?

Also created #3033410: Ensure that links that opening the off-canvas dialog are in the viewport after the dialog is open because I checked and this existing problem with the Setting Tray module. But could also affect any link using the off-canvas dialog.

So we should put a @todo here to find a general solution in that issue and get rid the custom solution here.

I tried to actually respond to one of the resize events fired in off-canvas.es6.js so that we won't need the wait but instead scroll after the page is resized. I couldn't get it work. So I think we general solution in the follow.

Per #57

1. Wrapped items in $this->assertNotEmpty()
2. Changed comment from ...to fully appear to ...to appear and for the page to be sized
3. Added @todo to that comment

Changes in #58 look good to address my review in #57

thanks, This issue is RTBC worthy in my opinion but we have needs accessibility review

Looks like @andrewmacpherson wants to take another look

Reroll

After the reroll, tests will not pass without disabling css animation. For good measure, I also added assertWaitOnAjaxRequest() after the tests AJAX requests.

Looks good!

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -118,3 +118,9 @@
+  z-index: 1;

I've seen the z-index brought up twice here already. @bnjmnm points out that it's necessary for it to be > 0, but what @lauriii asked I believe is whether 1 is high enough.

The JavaScript maintainer formerly known as @drpal (FKAdrupal) already signed off on the JS here, so I'm comfortable reviewing this for commit if we can confirm we've sufficiently address @lauriii's previous feedback about the z-index.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -195,4 +195,61 @@
+  $(window).on('dialog:afterclose', () => {
+    $('.layout-builder-highlight').removeClass('layout-builder-highlight');

We need to check here that Drupal.offCanvas.isOffCanvas($element)
otherwise the block configure forms could actually open up modal themselves in the case they are using ckeditor and other custom code.. If we don't check we remove the highlight when the modal is closed.

here is the fix for #66

Very good catch @tedbow. Checked this locally and it looks like a similar conditional needs to be added to 'dialog:aftercreate', otherwise the highlight is removed in the same manner. To reproduce, add a custom block and use ckeditor to add a link to the body field. Without adding a check it 'dialog:aftercreate', the highlighting goes away as soon as the "add link" modal appears.

@bnjmnm thanks for checking this and catching the other case.

The interdiff was created with git diff -w to ignore whitespace.

While confirming the z-index: value is fine for #65, I adiscovered that if the cursor hovers over a highlighted block's contextual link button, the highlighting goes away due to a css rule from the contextual module .contextual-region.focus taking priority. I addressed this by changing the .layout-builder-highlight selector to #layout-builder .layout-builder-highlight. Tests were modified to check for this, so a fail patch is included.

The z-index: 1 provides the results that I think work best to see, at least. It properly takes precedence over the blue dashed outlines, but contextual links appear over it and available for use.

Anything else with static positioning should not be a concern as it would be contained inside the outline. If there are other use cases where the blocks contain non-statically positioned elements from core, hopefully the reviewer can point those out.

This patch includes the changes from #3035669: Block contextual links intermittently ignore mouseup, otherwise the tests would return a false positive.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
   @@ -214,6 +214,13 @@ protected function assertContextualLinksClickable() {
   +    // its contextual link button. This confirms that contexual link mouseup
   

   contextual misspelled.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
   @@ -214,6 +214,13 @@ protected function assertContextualLinksClickable() {
   +    $assert_session->waitForElementVisible('css', '.search-block-form.focus');
   

   should be surrounded by $this->assertNotEmpty()

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
@@ -214,6 +214,13 @@ protected function assertContextualLinksClickable() {
     $this->clickContextualLink('.block-field-blocknodebundle-with-section-fieldbody [data-contextual-id^="layout_builder_block"]', 'Configure');
     $this->assertNotEmpty($assert_session->waitForElementVisible('css', '#drupal-off-canvas'));
+
+    // Check if contextual link container can get 'focus' class after clicking
+    // its contextual link button. This confirms that contexual link mouseup
+    // events are not disabled by the Layout Builder UI.
+    $this->toggleContextualTriggerVisibility('.search-block-form');
+    $page->pressButton('Open Search form configuration options');
+    $assert_session->waitForElementVisible('css', '.search-block-form.focus');

Running the test locally it seems there fail I get is because the off canvas is in front of the contextual link. Does the off-canvas dialog have to open for the test to prove what is proving or could we close it again?

I testing adding a second

$page->pressButton('Close');
$this->assertNoElementAfterWait('#drupal-off-canvas');

right before and it does fix that exact failure for me locally

Addressed the feedback in #72

Re: #73The test needs to happen with the dialog open. The problem is the supression of a mouseup event, so once another click happens the fail condition goes away. Hopefully the inconsistency is addressed as I rewrote the test so simulates the drag behavior that causes the problem with real-world use.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -195,4 +201,65 @@
+        /*
+         * Wait 700ms for the off canvas dialog to appear and for the page to be
+         * sized. After this, see if target element is still visible in viewport.
+         * If not, scroll page so target element is visible in its pre-click
+         * position.
+         *
+         * @todo Replace this custom solution when a general solution is made
+         *   available with https://www.drupal.org/node/3033410
+         */
+        setTimeout(() => {
+          const targetTop = $target.offset().top;
+          const targetBottom = targetTop + $target.outerHeight();
+          const viewportTop = $(window).scrollTop();
+          const viewportBottom = viewportTop + $(window).height();
+          const scrollAmount = targetTop - targetTopPreResize;
+          if (targetBottom < viewportTop || targetTop > viewportBottom) {
+            window.scrollBy({
+              top: scrollAmount,
+              left: 0,
+              behavior: 'smooth',
+            });
+          }
+        }, 700);

I haven't had a chance to review all of patch, but I did want to strongly urge you to not use `setTimeout`. Any time you have to wait for an arbitrary amount of time for the browser to complete an action, it's a signal of a larger issue. Imagine this being run on a slow computer, or the test environment which will behave differently than the environment that the 700ms was decided on.

I saw that you have a `@todo` for this, and thats great. I would suggest blocking further development on this issue until you solve this problem. Perhaps dispatching an event when whatever you are waiting on is complete and listening for it here?

I wasn't particularly comfortable with the timeout either so I'm happy to get the push from @alwaysworking in #75 to revisit it - found a solution that should be much less fragile.

The 700ms was to wait for a css transition in off-canvas to complete.

.dialog-off-canvas-main-canvas {
  transition: padding-right 0.7s ease, padding-left 0.7s ease, padding-top 0.3s ease;
}

I added an event listener that fires when the transition completes. The listener does roughly the same thing as the timeout, aside from the scroll amount being computed differently since there isn't access to the element's prior position.

 const mainCanvas = document.querySelector('[data-off-canvas-main-canvas]');

    // This event fires when canvas css transitions are complete.
    mainCanvas.addEventListener('transitionend', () => {
      const $target = $('.layout-builder-highlight');

      if ($target.length > 0) {
        // These four variables are used to determine if element in viewport.
        const targetTop = $target.offset().top;
        const targetBottom = targetTop + $target.outerHeight();
        const viewportTop = $(window).scrollTop();
        const viewportBottom = viewportTop + $(window).height();

        // If element not in viewport, scroll it into view.
        if (targetBottom < viewportTop || targetTop > viewportBottom) {
          const viewportMiddle = (viewportBottom + viewportTop) / 2;
          const scrollAmount = targetTop - viewportMiddle;
          window.scrollBy({
            top: scrollAmount,
            left: 0,
            behavior: 'smooth',
          });
        }
      }
    });

Reroll +
Also - because it may be easy to miss at this point - reminder from #70 that the JS changes in behaviors.layoutBuilderDisableInteractiveElements and changes to LayoutBuilderDisableInteractionsTest come from #3035669: Block contextual links intermittently ignore mouseup and are necessary for the test to not return a false positive.

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -114,3 +114,9 @@
+#layout-builder .layout-builder-highlight {

Using this method isn't sufficient for overriding the contextual styles. If we want to specifically override the contextual selector, we could just use this selector: .layout-builder-highlight.focus. However, I'm a bit concerned if removing the focus styles will be acceptable.

Since #78 was a bit open ended (overriding the focus styles vs. preserving what contextual provides), I've provided three options, gifs that demonstrate what they do, and the tests have been modified for each version to account for the differences.

1. 2994909-79-highlight-over-focus: contextual focus outlines are never visible if the element has .layout-builder-highlight
2. highlight-dashed-while-focused - a focused contextual region with .layout-builder-highlight will have a black dashed outline instead of the default blue
3. cx-focus-beats-highlight - .layout-builder-highlight does not apply any outline on focused contextual regions. (note there is still overlap with the outline applied to .layout-builder__region which would be outside the scope of this issue)

re #75 @alwaysworking agreed we should get rid of the timeout

re #76 This works well and doesn't rely on the specific timeout value.
I have tested in manually and the scrolling still works as expected!

re #79
I think cx-focus-beats-highlight is the best solution here.

It keeps the styles of the contextual module which also may have been customized by a particular theme. I don't like highlight-dashed-while-focused because it kind of assumes contextual will have dashed line and we don't this for any particular theme. 3) seem to assume the least about how contextual styling will look.

There are three patches in #79. When we RTBC an issue, let's make sure the solution chosen is uploaded by itself so we don't accidentally commit the wrong thing. :)

I think @tedbow is saying that #layout-builder .layout-builder-highlight:not(.focus) is what he RTBCed, but I looked at the gif of that and it's a weird and disorienting behavior.

It looks like a bug and it would confuse me if I were the user. I think this needs UX and a11y feedback.

This is already tagged for accessibility review, so we need that signoff before this is marked RTBC. I'm also going to specifically tag it for frontend framework manager review given recent discussion. Thanks!

Updating issue credit to include topic reviewers.

It's not clear which patch the RTBC is about. I'm still confused about what is being proposed. There's talk of focus styles now?

@xjm - "Weird and disorientating" is a bit vague. Can you explain in more detail? I'm having trouble following that animated GIF in #82. It's too fast and it loops so I can't tell what sequence of events it's supposed to depict.

re #72 sorry for RTBC'ing this to early.
I actually have change my opinion from #81. I detail my reasons below.

@andrewmacpherson hopefully I can clear up what is going on here with the focus styling.

1. This issue adds a border to highlight the current thing that is being configured, removed, or added in off-canvas dialog using the layout builder
2. This could be a section or a block
3. for an existing block the highlight block also has a contextual link trigger
4. The context module in also provides a focus style for the thing the context link trigger corresponds to. from core/modules/contextual/css/contextual.theme.css

   from <code>core/modules/contextual/css/contextual.theme.css

   /**
    * Contextual region.
    */
   .contextual-region.focus {
     outline: 1px dashed #d6d6d6;
     outline-offset: 1px;
   }
   

5. in #78 @lauriii pointed out that our highligh border won't override the contextual focus style when as for block the div that contextual module highlights via .contextual-region.focus selector is exactly the same as the div we highlight with .layout-builder-hightlight
6. so the 3 solutions @bnjmnm proposed in #79 where 3 different solutions the problem
7. Since the gifs are hard to demonstrate here is short video with all the possible solutions: https://youtu.be/zhVPFTJFGHw

After making the video and looking at the solutions in detail I now think we should use the patch 2994909-79-highlight-over-focus.patch

As you will see in the video in this solution if the block is highlighted via layout builder because it is being configured(or removed) in the off-canvas it does not apply the highlighting from the contextual module when the blocks is in focus. The contextual trigger is still highlighted.

The reason I think this is good is because the purpose of the contextual modules css to highlight the element that the contextual link trigger will effect is to let the user know what part of the page they will be affecting if they use the contextual links connected to the trigger.

Since in this case the part of the page they will be affecting is the block in the layout builder which already has a highlight then I don't think we need an extra highlight.

I'm inclined towards the first behavior. Highlighting the same way everywhere for contextual links makes that action feel more consistent.

Lack of focus effect doesn't necessarily cause critical problems for mouse users. If we completely remove the focus effect, it means that the only indication of the area being focused is the contextual link icon, which is very subtle.

@DyanneNova would your concerns be addressed if we could come up with an alternative design where focus and the highlight work together in a more elegant way?

I've watched the video from #86, and tried all the patches from #77 and #79

Firstly, there is a very confusing terminology problem in the comments here. Lots of comments talk about focus, but all the GIFs and videos show mouse hover behaviour only. Are you actually talking about focus? (i.e. document.activeElement, the :focus pseudoclass, the tabbing order, and all that.)

It seems what you are actually talking about is styling a container DIV, when the mouse hovers over it, or a control inside the container has keyboard focus (i.e. what :focus-within is for, once all the browsers support it). However the container itself doesn't get focus, so the WCAG "Focus visible" success criterion does not apply to it. The contextual menu button is what gets focus, and the patches here don't touch that.

From an accessibility viewpoint, I have no strong opinion about which patch from #79 is best. The simplest approach is 2994909-79-highlight-over-focus.patch. There is no actual accessibility need for the extra styles in the other patches, at least as far as WCAG is concerned. The hover border style is a secondary signifier only; the primary signifier for a pointer user is the pointer itself (mouse arrow icon, screen magnifier cross-hairs, or actual finger).

What I DO have strong opinions about:

• .focus is a misleading class name for the container outline styling. (Doesn't affect the user, but has made it hard to comprehend the comments here. That's a DX issue.)
• The focus styling for the contextual menu button fails "Use of Color" (WCAG 2.0), and the resting state fails "Non-text Contrast" (WCAG 2.1). Those are general problems for everywhere it is used, so I'll file a separate issue for this.

Important related issue!

While I think that the highlight proposed here is helpful, it only helps sighted users. If we commit this issue on it's own, it will introduce a WCAG level A failure of "Info and relationships" because the only way you know what a remove block/section dialog refers to is by the visual highlight.

So we must not commit this issue, until after fixing:. #3037550: Clarify which block or section is being removed in layout builder dialog. I've made that a separate issue, because it's important for accessibility regardless of the highlight introduced here.

Active again now that #3037550: Clarify which block or section is being removed in layout builder dialog is in.

Re-queued tests.

After meeting with UX team on Tuesday it was decided that the 2994909-79-highlight-over-focus.patch patch was the way to go from #79.

so only that patch needs a reroll.

The UX team also suggested we make a non-stable blocking follow-up Explore ways to make the highlighted Layout Builder element stand out more. This could be tricky because it is displayed in different front-end themes. So it might end up being just recommendation in the theming guide for Layout Builder.

I tried to improve the design a bit from the black border.

Add section:

Highlighted add section:

Block:

Highlighted block:

Add block:

Highlighted add block:

Section with blocks:

Highlighted section with blocks:

@lauriii do you know what patch you started with if so I can make interdiff. I chatted with @lauriii and he said he only made CSS changes but want to confirm again with him.

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -122,3 +123,32 @@
   +  background: rgba(47, 145, 218, 0.05);
   

   Since #2938182: Design intuitive affordances for Layout Builder (for illustrating which parts of the page are editable in a given context) added a white background to the layout builder we are able now to actually do a background for the highlighted block too.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,74 @@
   +          'is-layout-builder-highlighted',
   

   The class now here was changed from 'layout-builder-highlight' which is why the test failed.

   I just was want to make sure this was intentional. Seems good to me.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
   @@ -116,4 +125,165 @@ protected function assertModifiedLayout($path) {
   +  private function assertHighlightedProperlyOutlined() {
   +    $outline = $this->getSession()->evaluateScript("window.getComputedStyle(document.getElementsByClassName('layout-builder-highlight')[0],null).getPropertyValue('outline')");
   +    $this->assertEquals('rgb(0, 0, 0) solid 3px', $outline);
   +  }
   

   This will also cause a fail since the actual styling has changed. I doubt we need to do this.

   I don't think anywhere has in core we check that when apply a class it has certain style properties;

   I removed it. If we wanted to keep it it would have be different based on whether it is block or "add section" or "add block" now since they are different.

I used 2994909-79-highlight-over-focus.patch as a base for my patch in #97. Here's interdiff.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,74 @@
   +      $('.is-layout-builder-highlighted').removeClass('is-layout-builder-highlighted');
   

   This should be split to a new line. Check Prettier.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,74 @@
   +    mainCanvas.addEventListener('transitionend', () => {
   

   This event, `transitionend`, can this be name-spaced? I am not sure where/what is triggering this event.

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,74 @@
   +        const targetTop = $target.offset().top;
   +        const targetBottom = targetTop + $target.outerHeight();
   +        const viewportTop = $(window).scrollTop();
   +        const viewportBottom = viewportTop + $(window).height();
   

   These are all numbers?

4. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,74 @@
   +      $('.is-layout-builder-highlighted').removeClass('is-layout-builder-highlighted');
   

   This should be split to a new line. Check Prettier.

Can we get animations of the latest behavior with the final design and embed them in the IS? Thanks!

re #102

1. Ran prettier, new line added.
2. The transitionend event is a standard DOM event that is triggered at the end of a CSS transition.

3. These are all numbers?

   I double checked that height, scrollTop, outerHeight, and offset().top return numbers.

4. Ran prettier, new line added.

Additional things:

1. For the functionality that scrolls the highlighted element into view, I discovered that IE does not accept scrollBy() with an object as the first arg, but it does accept scrollBy(x-coord, y-coord). The options object is desirable when available (it provides the option to scroll smoothly instead of instantly - the default in some browsers), but not critical, so I added a check that uses scrollBy(options) when compatible, but uses scrollBy(x-coord, y-coord) otherwise. Here is a video of IE11 using scrollBy(x-coord, y-coord) https://youtu.be/0hkVsas8blM
2. The highlights for Configure Section resulted in two borders around the highlighted section. Made a small CSS change to eliminate this doubling.
   Before:
   
   After:
   
3. Per #103, recorded a video with the current behavior and added it to the issue summary https://youtu.be/r2UlWDmuAxY

I guess I should have mentioned that the changes look good to me. 🎉

At UX meeting today there was unanimous that we switch back to highlight border colors that we had in before the changes in #97.

So this would mean always black border around highlighted elements not matter if they are new/existing block or section.

So this would just mean that

+++ b/core/modules/layout_builder/css/layout-builder.css
@@ -122,3 +123,32 @@
+  background: rgba(47, 145, 218, 0.05);

And we the consensus was that we remove the background change for blocks.

Since there were other JS and class name changes in #97 I don't think we can got back previous patches just need to update CSS for the new black border on highlighted elements.

crediting those from UX meeting who wasn't already credited.

Let me know if I missed anyone

Ok implementing the UX meeting recommendations re #108

Going to file 2 follow ups:

1. When you click "Add section" the ajax progress throbber briefly changes the height briefly
2. Determine if it is possible for the "Configure Section" highlight completely hide the dotted border it replaces.

Adding a couple other contributors from the UX meeting.

I manually tested and confirmed this addresses the usability and accessibility feedback we received during the UX meeting yesterday. Nice work!

I reviewed the docs and backend code. Just a bunch of docs nitpicks, mostly regarding small words like "the" and "is" being omitted from inline comments.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,85 @@
   +       * Every dialog has a single 'data-layout-builder-target-highlight-id'
   +       * attribute.
   +       * Every dialog-opening element has a unique
   +       * 'data-layout-builder-highlight-id' attribute.
   

   This formatting is a bit weird; I think we can combine these two half-paragraphs?

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,85 @@
   +       * an elements value of data-layout-builder-highlight-id, the class
   

   Should this be "an element's value"?

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,85 @@
   +       * 'is-layout-builder-highlighted' is added to it.
   

   "Added to the element", I think. (As the sentence is currently written, "it" refers to the data attribute rather than the element itself.)

4. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,85 @@
   +   * When a Layout Builder dialog is triggered, the main canvas resizes. After
   +   * the resize transition is complete, see if target element is still visible
   +   * in viewport. If not, scroll page so target element is again visible.
   

   Nit: the word "the" is missing twice here ("the target element").

5. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,85 @@
   +        // These four variables are used to determine if element in viewport.
   

   "If the element is in the viewport".

6. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,85 @@
   +        // If element not in viewport, scroll it into view.
   

   "If the element is not in the viewport"

7. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,85 @@
   +          // Check if browser supports scrollBy(options) if not, use
   

   "Check whether the browser supports scrollBy(options). If it does not, use..."

8. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,85 @@
   +  // When a dialog closes, remove highlight from all elements.
   

   "Remove the highlight".

9. +++ b/core/modules/layout_builder/src/LayoutBuilderHighlightTrait.php
   @@ -0,0 +1,64 @@
   +   *   The region within the section the block is placed.
   

   This isn't quite a sentence... maybe "The section region in which the block is placed"?

10. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +   * Tests that dialog opening elements are properly highlighted.
    

    Nit: "dialog-opening elements", or, better, "elements that open the dialog".

11. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // Highlight is present with AddSectionController.
    ...
    +    // Highlight is present with ChooseBlockController::build().
    ...
    +    // Highlight is present with ChooseBlockController::inlineBlockList().
    

    "The highlight is..."

12. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // Submit form to add section then confirm no element highlighted.
    

    "Submit the formm to add the section and then confirm that no element is highlighted anymore."

13. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // Add custom block.
    

    "Add a custom block."

14. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // Highlight should persist with all block config dialogs.
    

    "The highlight should..."

15. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // Highlight is present with ConfigureBlockFormBase::doBuildForm.
    

    "The highlight is..." Also, doBuildForm() should have parens.

16. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // Highlight is present when Configure section dialog open.
    

    The highlight is present when the "Configure section" dialog is open.

17. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // Highlight is present when Remove section dialog open.
    

    The highlight is present when the "Remove section" dialog is open.

18. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // Block is highlighted when its "Configure" contextual link is clicked.
    ...
    +    // Block is highlighted when its "Remove block" contextual link is clicked.
    

    "A block is..."

19. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // Make sure highlight remains when mouse reveals contextual links.
    

    "Make sure the highlight remains when contextual links are revealed with the mouse."

20. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +    // @todo: Remove reload after https://www.drupal.org/node/2918718 completed.
    

    @todo Remove the reload once https://issue is completed.

21. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +   * Confirm presence of is-layout-builder-highlighted element.
    

    "Confirms the presence of the 'is-layout-builder-highlighted' class."

22. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderUiTest.php
    @@ -116,4 +125,153 @@ protected function assertModifiedLayout($path) {
    +   * Waits for dialog to close and confirms no highlights present.
    

    "Waits for the dialog to close and confirms no highlights are present."

In #114 \Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderDisableInteractionsTest::assertContextualLinkRetainsMouseup() failed because it checks that mouse up events are working correctly by moving the mouse after using the contextual link on the body field and making sure the body field is in exactly the same place. This is because if the mouseup event is not working correctly than the body field would actually dragged when the mouse moved.

The problem is with our new border JS getBoundingClientRect() of the element will change by 4 pixels(the exact difference in the fail). We could change assert to take these 4 pixels into account but this seems we would be just asking for random fails if we measure to the exact pixel.

Instead I changed the assert to take into account that the body might move very slightly but it should be much much less than the distance the mouse is move(because it is not moving because the mouse moved).

Please read he comments in the interdiff on core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php for more detailed explanation of the assert logic but it finally comes down to this assert.
$this->assertGreaterThan($distance_body_block_moved * 20, $minimum_distance_mouse_moved);
In assertGreaterThan() the 2nd should be greater.

RE #120 @xjm thanks for the review. Fixed all the nits 🔎

@tim.plunkett thanks for review. fixed

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -121,3 +122,22 @@
   +.layout-builder-block.is-layout-builder-highlighted,
   ...
   +  margin: -4px -2px;
   

   It would be great if we didn't change the margin of the .block element since it is not a layout builder element.

   Discussed with @tim.plunkett on a call and he proposed that we wrap blocks in the layout builder preview with another div that we could theme more safely. I think that is a good solution for fixing this problem.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -201,4 +201,86 @@
   +    // This event fires when canvas css transitions are complete.
   

   s/css/CSS

I'm fine with postponing #125.1 to a follow-up. We do other modifications to the spacing in the layout builder preview as well, and the worst case scenario is that someone experiences different margin between blocks. This shouldn't render the site unusable or nothing of the sort.

I've manually tested the changes on Chrome, Firefox and IE 11 (normal and high contrast mode) and I couldn't find any regressions on either Umami or Bartik.

I've got a backend signoff from @xjm in #120. I also recognized #91 as a sign-off from accessibility topic maintainer since we've already solved the related issues.

There was an extra new line that was caught by PHPCS. I fixed this on commit and attached an interdiff.

Committed 40d6aca and pushed to 8.8.x. Also cherry-picked to 8.7.x. Thanks!