The Layout Builder block listing should be filterable

Needed to run yarn prettier and 1 function could be turned into "fat arrow" function

This is a usability issue for sure, tagging as such.

#2968500: Change inline blocks workflow in Layout Builder to match mocks is in, so this needs a reroll or at least a re-upload.

Minor nits to begin with...

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

   Should be protected.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,109 @@
   +    $user->save();
   

   No need to call $user->save(), drupalCreateUser() does it already.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,109 @@
   +    $this->assertEquals(1, count($visible_rows));
   

   This should be assertCount().

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,109 @@
   +    $this->assertEquals(0, count($visible_rows));
   

   So should this.

Rerolled, adressed #9.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,70 @@
   +(($, { ajax, behaviors, debounce }) => {
   

   You're using destructuring here, however also using `Drupal.*` further down.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,70 @@
   +      let $filterLinks;
   

   This is a bit strange to declare this at the top. Why not just move the if condition higher to assign it?

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,70 @@
   +      function filterBlockList(e) {
   

   I'm not generally a fan of function declarations. Can these become function expressions?

4. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,70 @@
   +              $(this).find('.js-layout-builder-block-link:visible').length > 0;
   

   You use an arrow function for this, but also `this`, are you sure this is what you want?

5. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,70 @@
   +          Drupal.announce(
   +            Drupal.formatPlural(
   

   What I mentioned above about destructuring.

Adding screenshots.

Also, I discovered that this patch breaks when Quick Edit is enabled. So we'll probably need to enable Quick Edit in the test.

When I run yarn run build:js -- --file modules/layout_builder/js/layout-builder.es6.js, I get changes in layout-builder.js (see attached diff file).
I think currently this patch is only working by accident? this is not set in arrow functions.

I'm going to try to solve this.

Addressed #12 and #14.

hen I run yarn run build:js -- --file modules/layout_builder/js/layout-builder.es6.js, I get changes in layout-builder.js

Yup, that's expected. In core, you should only ever edit .es6.js files; they get transpiled down to .js files, which are ECMAScript 5 and should not be edited directly.

I, um, think we're back to needing review. @bendeguz.csirmaz tells me that the patch is not broken by Quick Edit, so I'll manually test (it may have merely been a missing file in patch #10). Removing the "Needs tests" tag until I can confirm or deny the influence of Quick Edit on this patch.

Gave this a quick manual test (with Quick Edit enabled) and it worked like gangbusters. Reviewed the patch and found only some minor things, plus one accessibility question. Otherwise, I'd say this terrific UX improvement is good to go.

1. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -110,8 +110,21 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +    $build['filter'] = [
   +      '#type' => 'search',
   +      '#title' => $this->t('Filter'),
   +      '#title_display' => 'invisible',
   +      '#size' => 30,
   +      '#placeholder' => $this->t('Filter by block name'),
   +      '#attributes' => [
   +        'class' => ['js-layout-builder-filter'],
   +        'title' => $this->t('Enter a part of the block name to filter by.'),
   +      ],
   +    ];
   

   Accessibility question: will the placeholder be available to assistive technology?

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,108 @@
   +    $session->wait(10000, 'jQuery("#drupal-live-announce").html().indexOf("blocks are available") > -1');
   

   The JavaScript condition here should check that indexOf() returns greater than 0, because if the index of "blocks are available" is zero, the message is malformed.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,108 @@
   +    if (count($blocks) > 0) {
   

   This seems a bit dicey to me; it could cause the assertion to be skipped, which makes the test a bit unreliable. How about using assertGreaterThan(0, count($blocks))?

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,108 @@
   +      $this->assertNotEquals(count($blocks), count($visible_rows));
   

   assertLessThan() might be a better choice here.

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,108 @@
   +    $session->wait(10000, 'jQuery("#drupal-live-announce").html().indexOf("block is available") > -1');
   

   This should also check that the string indexOf() is > 0.

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,108 @@
   +    $session->wait(10000, 'jQuery("#drupal-live-announce").html().indexOf("0 blocks are available") > -1');
   

   This should verify that the message index is === 0.

Hey There,

Per our conversation @NEDCamp:

Screen reader users are not limited to those who are visually impaired. To reduce the chance of poor UX for the user that takes in information by multiple ways simultaneously (ie. screen reader + visually) please have the non-visible form field label and the default value match. Otherwise, the misalignment of information could be perceived as confusing.

Please be sure that the default text has significant color contrast as well. Please see https://webaim.org/articles/contrast/ for additional details.

Addressed items in #19 and #20.

Thanks, @starshaped! Interdiff looks right to me, and still works well under manual testing. RTBC once it passes Drupal CI.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,69 @@
   +      const filterBlockList = function(e) {
   

   Arrow function.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,69 @@
   +        const toggleBlockEntry = function(index, link) {
   

   Arrow function.

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,69 @@
   +            .each(function() {
   

   Arrow function, and you will probably have to change `this`.

Additionally, I think this patch needs to have Prettier run.

1. fixing the arrow functions from #23

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,69 @@
   +              const hasBlocks =
   +                $(this).find('.js-layout-builder-block-link:visible').length >
   +                0;
   +              $(this).toggle(hasBlocks);
   

   We are hiding the categories here but not actually testing that.

   Added test for this

@tedbow, you're a hero. Changes look great, and thanks for adding the additional test.

Back to RTBC.

Demoed this for the UX team today. We found one bug, and two follow-ups:

1. Follow-up: It'd be nice if the filter also filtered by category, in addition to block labels. I'm not 100% sure how this would work, and it doesn't do that on the module listing page either, so let's open a follow-up to investigate and possibly implement.
2. Follow-up: Alongside the module listing page, this pattern is now implemented at least twice in core, yet has no generic implementation. Let's look into making this generic.
3. We found a bug: if you search for a string, the #drupal-live-announce div updates with "N blocks found in the modified list". If you then delete the search string, the announcement div is not updated. Let's fix that bug and ensure the tests cover it.

@phenaproxima thanks for the update from the UX meeting.

for #26.3

If you then delete the search string, the announcement div is not updated.

Should the announce div be cleared? Not sure Drupal.announce() supports that. Or should it say something else like "All blocks are now ...."

Should the announce div be cleared? Not sure Drupal.announce() supports that. Or should it say something else like "All blocks are now ...."

I think it should be something like "All available blocks are listed." Shall we open the bikeshed?

See also

• Drupal.behaviors.simpletestTableFilterByText
• Drupal.behaviors.blockFilterByText
• Drupal.behaviors.viewTableFilterByText

The Views UI implementation allows searching by both the machine name and description.

#26.2: a generic component might help, but there are some differences between the various filter implementations.

• The main views listing provides one filter field, but controls the content of two separate lists (for enabled and disabled views). The Drupal.announce() mrssage would need to convey this.
• The views dialigs for adding fields, sorts, etc. These have two filters controlling the same list (a textbox and a select).

#27: don't bother trying to clear the announcement, as it would only be announced once anyway. It's a live region which is announced after it changes. You could perhaps send an empty string to it, but I don't think there's much point.

#28 is more on the right track. It's a special case where we show them all, so I think it needs a different nessage (e.g. saying it's a "modified list" diesn't make sense, it's really the default list). There may be some value in using the full count though, as it provides a sense of scale.

#29: Not all of these filters have a Drupal.announce() behaviour yet - only the module page did that when D8.0.0 was released. Since then, Drupal.announce() was added to the biock filter, and there are some open issues to add it to other filters like views UI. Note there are two places with filtering in Views UI - the main list of views, and the dialogs for add field/filter/sort etc. The module filter also responds to machine_name, I don't recall how the others behave.

#26.3 - This could be a follow-up, since none if the other filter announcements do this either.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,69 @@
   +        $categories.find('.js-layout-builder-category').show();
   +        $filterLinks.show();
   

   Found this problem. It is just a small optimization.

   We are showing all the links and categories but right after we call toggle() on them depending on whether they shown or not.

   So we actually don't need to do the show() call. The only time that we need to do this is in the case where query.length < 2. So moving this into an else block.

3. so re @andrewmacpherson's comment in #30
   

   don't bother trying to clear the announcement, as it would only be announced once anyway. It's a live region which is announced after it changes.

   It looks like we don't need to worry about #26.3

Thanks, @tedbow! However...

It looks like we don't need to worry about #26.3

I think we do need to worry about it. @andrewmacpherson said not to worry about clearing it...but we do need to worry about updating it. If all blocks are now visible after clearing the search query, we should announce that. Kicking this back for that, plus test coverage.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -1,5 +1,69 @@
+          $categories.find('.js-layout-builder-category').attr('open', '');

I notice here that we are opening all categories.

But we don't close them if the user deletes their search. Is this intentional? Maybe it doesn't matter because we know the user is looking for a block.

After #2977587: Improve block listing in Layout Builder by hiding uncommon block plugins lands most categories will be closed by default and I think will be more apparent and maybe a problem.

1 thing we could do is set a data attribute for which ones were closed before the search and close them when they cancel the search.

I think we already going to have keep track of whether the user had a query that was more than 2 letters and was querying or was just typing their first character. Otherwise when the user first types 1 character we are going to announce all blocks are available when nothing has actually changed.

Added the "All available blocks are listed." message.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,4 +1,4 @@
   +(($, { ajax, behaviors, debounce, announce, formatPlural, t }) => {
   

   We can't actually use Destructuring for Drupal.t

   It is because PHP scans the js files for Drupal.t to find translation strings 😱😩

2. +++ b/core/modules/layout_builder/js/layout-builder.js
   @@ -37,6 +38,7 @@
   +          announce(t('All available blocks are listed.'));
   

   I think this will also announce when you first start to search. So

   1. User press 1 key
   2. gets announce about all blocks being listed

   Since nothing has changed in the listing at this point it seems weird to me.

Re #32.3 and #33
Phenaproxima is right, that's what I meant. There's no point trying to clear out the Drupal.announce DIV, but I do think it's worth making a message to say that all items are shown, when a user clears the filter.

I'm not sure how to phrase it. The message in #35 will do, but maybe it could be improved. What are we trying to convey? The essential point is that we have not hidden anything.

The Drupal.announce tag has some related issues where we are adding announcements to other filters.

I gathered up the various ideas into a plan, for better consistency across all our filter lists. Some already have issues in progress.

Looks good, needs work for #36

Addressing #36

Retesting b/c drupalci error

Great patch. Simple, concise, useful, and well-tested. I have only a few minor questions.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,73 @@
   +          $categories.find('.js-layout-builder-category').attr('open', '');
   

   This line could use a comment explaining why it's doing what it's doing.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -1,5 +1,73 @@
   +          $categories
   +            .find('.js-layout-builder-category')
   +            .each((index, category) => {
   +              const hasBlocks =
   +                $(category).find('.js-layout-builder-block-link:visible')
   +                  .length > 0;
   +              $(category).toggle(hasBlocks);
   +            });
   

   This seems like a good place to take advantage of jQuery's has() filter. Something like:

   $categories.find('.js-layout-builder-category:has(.js-layout-builder-block-link:visible)').toggle(true)

3. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
   @@ -110,8 +110,21 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
   +      '#attributes' => [
   +        'class' => ['js-layout-builder-filter'],
   +        'title' => $this->t('Enter a part of the block name to filter by.'),
   +      ],
   

   Is the title attribute useful here from an accessibility and usability standpoint? Maybe it should be a #description instead?

Patch addresses 2/3 of #43
1) Change: Added comment explaining category open call (and a few other comments explaining some nearby lines)
2) Change Refactored code responsible for category to leverage has()
3) No Change but definitely open to discussion. Using #description vs. the title attribute is interpreted identically by modern screenreaders (I qualify with modern only because I can't confirm this with old ones - there may be no difference). Regarding the UX side of it, my subjective take is the text in the title attribute is helpful for screenreader users because they're not yet aware of the list of blocks beneath - but users viewing the screen will have enough context to make that additional instruction unnecessary. Very open to feedback on this one.

While reviewing I caught to additional things that should be considered -
1 - If any block categories are collapsed prior to filtering the block list, their closed-state is not remembered if the list is filtered then returns to non-filtered. This closed-state should be remembered when returning to displaying the nonfiltered list.

2- The input placeholder does not have sufficient WCAG approved contrast. I advise switching from placeholder to label as it would address the contrast issue, and adhere to Nielsen's UX recommendations regarding placeholders https://www.nngroup.com/articles/form-design-placeholders/ (among other things, Neilsen and a few other sources mention that some older screenreaders ignore placeholder inputs.

Added revised patch to address test failures from #44 - all comments there still apply.

Forgot to mention in #44 that one additional change was made after discussing it with @tedbow: prior to this patch, category visibility was based on a data-blocks-filtered attribute on each category. To save unnecessary DOM manipulation, this was refactored to be a single bool variable declared just prior to the layoutBuilderBlockFilter, which simply indicates if filtering is taking place.

From discussion with @tedbow, we agreed that preserving category open/closed state should be maintained before and after filtering. Adding this to remaining tasks in the issue description.

This is the patch from #44 tweaked to pass tests

This patch addresses closed-categories not remembering they were closed after filtering occurs. This scenario was also added to testBlockFilter()

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,143 @@
   +    $contentFieldsCategory = $page->find('named', array('content', 'Content fields'));
   +    // Link that belongs to the Content Fields category, to verify collapse.
   +    $promoteToFrontPageLink = $page->find('named', array('content', 'Promoted to front page'));
   

   Need to use short array syntax.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,143 @@
   +    $contentFieldsCategory->click();
   +    $this->assertFalse($promoteToFrontPageLink->isVisible());
   

   I don't(edited) think we need a wait here because this just an html element behavior and not JS but pointing it to be sure.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
   @@ -0,0 +1,143 @@
   +    $session->wait(10000, 'jQuery("#drupal-live-announce").html().indexOf("blocks are available") > 0');
   

   I think you do this with waiting on script string by using
   $this->assertNotEmpty($assert_session->waitForElement('css', '#drupal-live-announce:contains("blocks are available")'));

   There are couple other occurrences below.

For #52 (2) - I can do the wait-for-element-not-exists far more elegantly if this issue were committed #2892440: Provide helper test method to wait for an element to be removed from the page (the issue reporter looks familiar). It was already RTBC once, and should be ready again. I rerolled the patch but had to make a few small changes so I can't RTBC it myself. I'd like to see if we can get that pushed through before addressing the tests here.

sorry for #52.2 I meant to say

I don't think we need a wait here because this just an html element behavior and not JS but pointing it to be sure.

Opening up a details doesn't involve JS if I am correct. If so I don't think we need the wait. Just wanted to point out we are not waiting incase we actually should be.

#52.1 , #52.3 are addressed, and confirmed #52.2 does not require waits.

Aaaaand I gotta actually upload the patches

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -1,5 +1,86 @@
+  let blocksFiltered = false;

This should probably be declared inside the attach function.

re #58 I think we need that outside the attach function because we don't want it be reset if the a part of the page is updated via ajax when the block listing is open. This would call attach.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php
@@ -93,34 +93,30 @@ public function testBlockFilter() {
-    $expected_message = '0 blocks are available in the modified list.';
-    $assert_session->elementTextContains('css', '#drupal-live-announce', $expected_message);
+    // Using wait() to check for index since block count may be double-digit.
+    $session->wait(10000, 'jQuery("#drupal-live-announce").html().indexOf("0 blocks are available") === 0');

We can also get rid of this JS evaluation

with

$announce_element = $page->find('css', '#drupal-live-announce');
$page->waitFor(2, function () use ($announce_element) {
  return strpos($announce_element->getText(), '0 blocks are available') === 0;
});

Something about this ticket apparently makes me bad at uploading - the correct files in the next comment.

Refactored per the recommendation in #60

Also was inspired by #58 - although blocksFiltered is intentionally outside of the attach function, I renamed it to something a bit more specific: layoutBuilderBlocksFiltered, since it has wider scope than most variables on the page.

#62 looks good!

RTBC 🎉, should pass tests but will get kicked back if not

The change itself looks good from my POV. However, we still have to open follow-ups for what has been mentioned in #26. We should open another follow-up for a problem where Umami styles are leaking to the search element:

Created follow ups:
#3028972: Add category to the search for Layout Builder block listing search/filtering
#3028968: Create Javascript library for searching rendered lists on the client.
#3028977: Umami theme break Layout Builder block filtering text box.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -1,5 +1,86 @@
+(($, Drupal) => {
...
+  behaviors.layoutBuilderBlockFilter = {

We should still add @file documentation and document the behaviors according to our JavaScript documentation standards.

@tedbow re. #59, could you add some documentation around why this is done this way?

#68 - documented per JS coding standards
#69 - added comment explaining why layoutBuilderBlocksFiltered is declared outside of attach.

Setting Needs Review for testing

I believe all feedback here is addressed. Back to RTBC.

Looks all good for me now. Had to rebuild the JavaScript files but it only led into adding a few new lines. Committed 59f8c0d and pushed to 8.7.x. I will also backport this to 8.6.x after the bug release commit freeze. Thank you, everyone!

fixing accessibility tag