Incorrect module count passed to Drupal.announce() on uninstall page

The problem in system.modules.es6.js is that line 65 $rowsAndDetails.find('tbody tr:visible').length searches for visible rows inside a table.
But $rowsAndDetails as defined Line 88 $rowsAndDetails = $table.find('tr, details'); consists of all table rows and detail elements.

On the Module list/install page each module list table has a category detail wrapper, so that works fine, but on uninstall page, it's just one table, so never finds tbody tr inside tr.

WIthout modifying the markup, I was able to get it working everywhere by using filter instead of find and targeting the visible table rows with [data-drupal-selector] attribute:
Line 88 to { '!modules': $rowsAndDetails.filter('tr[data-drupal-selector]:visible').length }

May be worth categorising modules on uninstall page, to match the markup.

Thanks waako for the analysis. Here is the patch based on that.

Applied patch in #7 It works perfectly.

Added test core/modules/system/tests/src/FunctionalJavascript/ModuleFilterTest.php (inspired from https://www.drupal.org/project/drupal/issues/2715663#comment-11682359).

Only 2 modules are available on uninstall page for testing profile, but still manage to test the following:

• The filter reduces the number of visible modules
• The Drupal.announce() message div contains a correct plural message when multiple modules are visible
• The Drupal.announce() message div contains a correct plural message when one module is visible
• The Drupal.announce() message div contains a correct plural message when zero modules are visible

+++ b/core/modules/system/tests/src/FunctionalJavascript/ModuleUninstallFilterTest.php
@@ -0,0 +1,87 @@
+  /**
+   *
+   */
+  public function testModuleUninstallFilter() {

This needs an actual comment, not an empty docblock.

Fixed missing comment in function docblock from #10.
Replaced deprecated JavascriptTestBase with WebDriverTestBase.

Sorry couldn't get an interdiff, original patch wouldn't apply to 8.9.x so had to recreate the changes manually.

I had issues running the tests locally, but finally got it working so was able to better identify issue.
With testing profile, number of preinstalled modules has changed, so removing assertEquals that original module count matches filtered list with string cache as we can't know how many modules will be in the list and if all have a common string.

1. +++ b/core/modules/system/js/system.modules.es6.js
   @@ -64,9 +64,11 @@
   +            Drupal.formatPlural(
   +              $rowsAndDetails.filter('tbody tr:visible').length,
   +              '1 module is available in the modified list.',
   +              '@count modules are available in the modified list.'
   +            )
   

   There are some JS coding standards not being met here, but they can be fixed automatically with yarn prettier.

2. +++ b/core/modules/system/tests/src/FunctionalJavascript/ModuleUninstallFilterTest.php
   @@ -0,0 +1,91 @@
   +    $module_rows = $page->findAll('css', '.system-modules-uninstall tbody tr td .module-name');
   

   .module name is the only part of the locator that is necessary here.

3. +++ b/core/modules/system/tests/src/FunctionalJavascript/ModuleUninstallFilterTest.php
   @@ -0,0 +1,91 @@
   +    self::assertNotEquals(count($module_rows), count($visible_rows));
   

   It's quite possible that using self:: has no adverse effects, but these uses should be changed to $this to be consistent with other FunctionalJavascript tests

4. +++ b/core/modules/system/tests/src/FunctionalJavascript/ModuleUninstallFilterTest.php
   @@ -0,0 +1,91 @@
   +  /**
   +   * Removes any non-visible elements from the passed array.
   +   *
   +   * @param array $elements
   +   * @return array
   +   */
   +  protected function filterVisibleElements($elements) {
   +    $elements = array_filter($elements, function($element) {
   +      return $element->isVisible();
   +    });
   +    return $elements;
   +  }
   

   There are several coding standards not being met here. The @return and @param need to be separated by a space, the should be descriptions for both, the $elements arg needs a type hint and there should be a space after function. Few have been able to remember all these rules and remain sane. Let code sniffer take care of this for you.

5. This final item could use some feedback from someone with more accessibility expertise: When filtering modules on the uninstall page, not all of the results are uninstallable. If other enabled modules depend on them, the uninstall option is grayed out and they are not reachable via tab navigation.
   
   I suspect this could be quite confusing, but not sure if it's in the scope of this specific issue. If it's determined it's not in scope, a followup should get created before this is RTBC.

This includes fixes for items 2 and 3 from comment #17.

Thanks for the code review feedback bnjmnm, and DamienMcKenna for fixing 2. and 3.

Here's items 1. and 4. updated, likely similar issues with my work at https://www.drupal.org/project/drupal/issues/2715663
I've used the test logic from BlockFilterTest.php for updating the @para and @return as that's what it was inspired from in first place.

With regards to the the filtered results including modules that cannot be uninstalled, I think a follow up issue for discussion is appropriate:
A screen reader can usually list all visible form controls on the page, this will include the disabled checkboxes but on focusing it will announce that it is dimmed/disabled then can read remaining text in table row which explains why module can't be uninstalled.
I can't say if that is good enough, so best discussed in a separate issue.

Re-rolled #19 for 9.4

Updated patch will fix test failures.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

At this time we will need a D10 version of this patch.

Rerolled for 10.1.x.

Removing the Reroll tag.

Testing patch #30 by going uninstall page and doing random filters checking that the announce was updating correctly.

I didn't see a tests-only patch so ran locally without the fix

1) Drupal\Tests\system\FunctionalJavascript\ModuleUninstallFilterTest::testModuleUninstallFilter
Behat\Mink\Exception\ElementTextException: The text "1 modules are available in the modified list." was not found in the text of the element matching css "#drupal-live-announce".

/var/www/html/web/vendor/behat/mink/src/WebAssert.php:847
/var/www/html/web/vendor/behat/mink/src/WebAssert.php:467
/var/www/html/web/core/modules/system/tests/src/FunctionalJavascript/ModuleUninstallFilterTest.php:63
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Passes with fix.

Opened a follow-up for #17.5.

Committed 89eb557 and pushed to 10.1.x. Thanks!

As a non-disruptive change, this would be qualified for a backport. However, the patch doesn't apply to 10.0.x or 9.5.x so that will require additional patches.

Patch for 9.5.x.

Patch for 10.0.x.