Modules uninstall filter does not filter by machine name

Fixed

Thanks this is a good catch that removes some inconsistency in the user experience. With the patch both filter fields on the Modules and the Uninstall page return the same results.

Filter on the Extend page:

Filter on the Uninstall page before:

Filter on the Uninstall with patch:

This is a really nice feature, thanks everyone for working on it! Kudos for adding the dir attribute for non-LTR languages, very nice.

Re #7: Did you mean to remove the "Needs usability review" tag? Not sure, so keeping it for now.

+++ b/core/modules/system/templates/system-modules-uninstall.html.twig
@@ -43,6 +43,7 @@
           <span class="text module-description">{{ module.description }}</span>
+          <span dir="ltr" class="table-filter-text-source visually-hidden">{{ module.machine_name }}</span>

I am anything but an accessibility expert, but simply looking at the markup: Won't this simply read the module machine name right after the description for screen-reader users? I.e. "Logs and records system events to the database.dblog". That seems a bit strange, instead I think we should add a (hidden) label for this field. I.e. the markup could be something like this:

          <span class="text module-description">{{ module.description }}</span>
          <div class="visually-hidden">{{ 'Machine name: '|t }}<span dir="ltr" class="table-filter-text-source">{{ module.machine_name }}</span></div>

As far as I can tell, this would make the output more sensible for screen readers. Although, having written that, we generally try to avoid translating such "chopped-up" strings. I.e. ideally, we would translate Machine name: @machine-name in its entirety. That is not really so easy, though, because of the added wrapping markup, but also because the dir="ltr" makes this somewhat of a special-case.

Phew, not really sure. In any case I guess the "needs accessibility review" tag can't hurt here.

You were right: no additional UX review needed.

Accessibility review of the patch from #2.

I enabled the 4 multilingual modules in core, then filtered by "loc", which matches machine name "locale" of the Interface translation module.

There are some accessibility problems with this.

• The machine name is included as visually-hidden text. Sighted users will not encounter it, which is potentially confusing. (WTF? How does "loc" match "Interface Translation"?). Sighted users should get the machine name too - there's no reason why a screen reader user needs it but a sighted user doesn't. The machine names are visible on the install page, albeit buried inside a collapsed <details>.
• The machine name is presented in without context. A screen reader user hears the description as "Translates the built-in user interface. locale.". Compare this with the install page, where the visible text says "Machine name: locale".

From the issue summary:

• Decide, if we should
  • show the module machine_name in the description column
  • or include it visibility-hidden (if it would be allowed UX-wise) to only make it searchable

Showing the machine name is preferable. I'd say an extra column just for the machine name is the simplest way to do it, rather than putting it in the description column. Updating issue summary with this.

How about adding "priority-medium" as class to the th and td of the machine name column so that it doesn't get in the way on small screens?

Patch in #11 - Nice! This is a much more inclusive design.

The patch modifies templates inside the Stable theme. This isn't normally allowed during the D8 minor releases, so I expect we'll have to copy the template from system module to the Seven (and Bartik?) themes, and leave the Stable theme untouched.

Re: 12 - Good idea! Medium priority sounds right. Here's the change request which describes how to add those responsive table classes:
Responsive table classes for modules and themes

Hmm, I'm not sure if that change reocrd is relevant anymore. The table headers are in the twig file.

TODO:
add the responsive priority-medium class to the machine name column.
don't change Stable theme
copy system module template to Seven + Bartik themes instead

Tried out the new patch and it works great in all three themes.

I found a number of bugs related to responsive tables and table-striping but none of them are caused by this patch, so will open dedicated issues for those in a minute.

Also attaching screenshots in all three themes.

Not RTBC-ing as this touches a number of areas that I'm not to knowledgable about and I feel like I would going out on three limbs (i.e. accessibility, usability, theme system) at once by pulling the trigger on this one. But I couldn't find anything to complain about.

From a usability point of view: it can be RTBC.

By the way, opened the following issues that I found reviewing:

1. #2923302: Module install table is missing the "Show all columns" link
2. #2923305: Module (un-)install tables have a useless "data-striping" attribute
3. #2923309: Responsive tables to not work in Stark

Re: #16 - good catch on the show-all-columns link.

The install page already has responsive columns. It treats the description column as low priority, so on a phone you just see the checkbox and human-readable module name. To match this, let's make the description column low priority here. I think we can leave the machine name column as medium priority for tablets, as the overall intent of this patch is to help site builders who are familiar with machine names.

The module name variable should be documented in the template's docblock.

Updating issue summary, detailing tasks done/todo, and noting UI changes.

fixing broken list nested markup in summary.

There are quite a lot of related issues, in particular a bug with the screen reader announcement on the filter. Reviewing these, I think it's safe to keep them as separate issues, and just focus adding the machine name column to the table here. This issue is moving quickly, and the others would still need fixing regardless of the machine name column.

This is going to need a re-roll after #2923305: Module (un-)install tables have a useless "data-striping" attribute

Wow, that was quick. Thanks!

Thanks @szeidler. Review of patch #24:

It removes the data-striping attribute from the template in the system module, and the patch applies cleanly.
But there's atill a data-striping attribute in the Seven and Bartik versions, which are new files.

TODO: remove the data-striping attribute from the Seven and Bartik templates.

Modules can be found machine name, the table is responsive and the points raised in #26 are addressed.

+++ b/core/modules/system/templates/system-modules-uninstall.html.twig
--- /dev/null
+++ b/core/themes/bartik/templates/system-modules-uninstall.html.twig

+++ b/core/themes/bartik/templates/system-modules-uninstall.html.twig
--- /dev/null
+++ b/core/themes/seven/templates/system-modules-uninstall.html.twig

We can just do

{% extends "@system/system-modules-uninstall.html.twig" %}

Instead of duplicating the entire thing.

Also rather then having the machine name comment. Did we considers just using the same format as the install page - i.e putting the machine name in a collapsible thingy along with the other info.

Re-tested latest patch (without code duplication). Still works fine.

+++ b/core/modules/system/templates/system-modules-uninstall.html.twig
@@ -28,7 +30,8 @@
+      <th class="priority-medium">{{ 'Machine name'|t }}</th>
+      <th class="priority-low">{{ 'Description'|t }}</th>

It feels like the description is a higher priority than the machine name which in many cases is going to be a duplicate of the Name column. Which is one of my concerns about this patch... Views views is not entirely useful. Whilst a column might be the simplest way I still don't understand why we're not following the example of the listing page where you install and including it in the description.

We also should have a test asserting that a machine name does appear on the uninstall page.

It feels like the description is a higher priority than the machine name which in many cases is going to be a duplicate of the Name column

Fair point, they will often be similar. I don't think it's a big problem to swap those priorities.

I still don't understand why we're not following the example of the listing page where you install and including it in the description.

That should be a separate issue I think. Whatever the historical reasons, these pages ended up with a different UI. Adding an extra column here is just a simple way to solve the problem without needing to rethink the design.

FWIW, I don't think the install page is any better. Concealing the machine-name inside a details element means there are sometimes matches with no obvious reason. For example "lo" matches locale machine name, but isn't seen in the human-readable name or description.

Tested via below mentioned steps and latest patch #40:

Module list

Go to /admin/modules
Filter for dblog
You'll see the "Database Logging" module

Uninstall module

Go to /admin/modules/uninstall
Filter for "dblog"
You'll see no results

Now, uninstall filter does filter by machine name.
This can be moved to RTBC

I have tested using the patch #40.The issue is solved after applying the patch.
Including the screenshot gif : https://www.drupal.org/files/issues/2020-07-28/drupal-uninstall_machine_...

Hi Tanubansal and Abhijith, Thanks for your time. Abhijith, I have tested the same #40 patch and it is also working from my end. I also didn't find any coding issue in that patch.

We still have the needs tests tag here.
As this is a bug report, we need some test coverage demonstrating the issue, and that the fix resolves the issue.

I am working on the automated test to cover this scenario.

Adding test coverage to demonstrate the bug and fix for the issue.

Do we need system-modules-uninstall.html.twig for claro?

Applied #15 patch in drupal-9.3.x-dev applied successfully
After patch we able to filter module with machine name at a while of uninstall page
Thanks for the patch
For ref sharing screenshots .....

+++ b/core/themes/bartik/templates/system-modules-uninstall.html.twig
@@ -0,0 +1 @@
+{% extends "@system/system-modules-uninstall.html.twig" %}

+++ b/core/themes/seven/templates/system-modules-uninstall.html.twig
@@ -0,0 +1 @@
+{% extends "@system/system-modules-uninstall.html.twig" %}

Is there a reason we use extend here? Can't we just delete those templates and the templates defined in the system module will be used?

I also think we should update the templates in stable and stable9.

Removing credit for #56 as sufficient screenshots were already provided in #47, and the "after" (were it needed), should show an example of filtering to a list that includes matches, not an empty one.

This was a bugsmash triage target today. I tested this on Drupal 10.1.x, umami install and reproduced the problem.

The patch needs to be brought up to date. There is a question to answer in #57.

Created a patch for latest version of Drupal. Please review.

No interdiff, but I have compared patch #64 against #51 and the differences are:

1. Trailing whitespace added to core/modules/system/templates/system-modules-uninstall.html.twig (but there was no coding standards fault, so maybe .twig files are not checked for this?)
2. Bartik and Seven twig templates are dropped
3. Olivero twig template added instead
4. No new line at end of Olivero template

I've not been involved with the actual work on this, so cannot comment on any previous actual chnages, just giving the comparison from last patch.

In response to raman.b in #51 yes we should cater for Claro, given that it is the new default admin theme. Just tested the UI manaully and it works fine already in Claro, but if we are adding a twig template for Olivero we should add one for Claro too.

If we are going to change the templates we will need a change record for contrib themes.

Added a change record.

Testing the patch can confirm it is filtering by machine name now

Uploading before/after screenshots to IS

Think this falls under 3.2.3.

Minor tweaks required for #65 .1 and .4 and Claro template

Using MR so that we can more easily see changes without providing interdiff. The first commit is patch #64.

From #57

Is there a reason we use extend here? Can't we just delete those templates and the templates defined in the system module will be used?

From #63

Adding the extend was based on the feedback in #30, but might need to be reevaluated with the current best practices in Core.

So I have removed the Olivero template which appears to be unnecessary. Also fixed trailing space.

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 request as a guide.

Testing MR 3273

On Drupal 10.1.x with a standard install
Verified seeing machine_name now.
Go to the uninstall page and searched for menu_link and got "Custom Menu Link" which was expected

One minor inconsistency I noticed (in code, not in functionality) is the way that the drupal.tableresponsive library is added. The Install page form already had $form['#attached']['library'][] = 'core/drupal.tableresponsive'; but this was missing from the uninstall form. It does not appear to be needed for manual interactive functioning of the filter, but it is required to allow the javascript tests to react to changing input. It was only when the uninstall form got it's first javascript test (in this issue) that the library was added. However, it was added via {{ attach_library('core/drupal.tableresponsive') }} in templates/system-modules-uninstall.html.twig

So is there a preference for adding the library via the $form or via the twig template? Even if there is no preference, we should make the two consistent, so that we don't ask this question again. I inadvertantly also added the library to the uninstall $form in this MR (because I was working on #3331975: Uninstall form does not add drupal.tableresponsive library so we at least need to remove one of them as it is unnecessary to do it twice.

We need to resolve #73 before committing this.

Thanks @alexpott. I have removed the attach from the twig template, and left it in the $form. This also matches what is in the install form.

Retested following steps from #72

Good job @jonathan1055!

I've looked at the design of this and I'm not sure what was implemented in #11 is the best we can do. I think we should do what the install form does and add the machine name to the description so that it appears visually (unlike patches before which added it as hidden text).

The machine name is not at all important when uninstalling a module - it's really only useful as something you might know to search for. Yet we're placing in in front of the description which can contain important things like why a module can not be uninstalled.

Compare...

With...

I agree with @alexpott. Adding the new column just to get the machine name on the page is not the best way to do it. It wastes a lot of screen space, meaning more scrolling, and shows duplicated info for 90% of the modules. This is an admin page, and making admin tasks quick to achieve is a primary goal. It seems from comments #10 and #11 that the decision to make it a new column was done for ease of coding. It's a backwards step in usability and we should be able to improve it to be more like the install page.

In the first instance the machine name can be added into the description cell. In the longer term, we could have a details element, collapsed by default, to match the install page.

Moving to NW for that change then. Makes sense to me.

Happy to give this a try, unless anyone else wants to do it? No need for more than one person to do duplicate work.

Argh, unintentional change (stale-form data?). Back to NW

First draft. Based the details element on the install page. Themeing was OK but not looking quite right, then I realised that Claro has it's own custom twig template for the install page, so I also made one for the uninstall page.

I puposely left the modified twig template indentation wrong on the first commit, so that it is eaaier to see which lines are changed and which have not been altered but merely indented.

If/when this design change is accepted I will update the issue summary.

The test failures were because I missed copying {{ form|without('filters', 'modules', 'uninstall') }} when creating the Claro template, which meant that the Uninstall button was not in the form. I based it on the install template, which does not have this. Interesting that it was only picked up by HelpTopicSearchTest which checks for uninstalling. Or maybe other tests use the system twig template? The HelpTopic test has protected $defaultTheme = 'stark' so I'm not sure why the Claro template was being used anyway. But lucky that it was, otherwise we would have have green passing tests for a major error in the code.

Are these test failures random and/or unrelated to the chnages in this MR?

Failure 26 Jan 2023 at 01:46 GMT https://www.drupal.org/pift-ci-job/2576971

Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest::testButton
Failed asserting that a NULL is not empty.

Rebase against 10.1.x but no code changes on this MR, we get a different fail
26 Jan 2023 at 11:28 GMT https://www.drupal.org/pift-ci-job/2577847

Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::testMediaElementAllowedTags
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "filters[media_embed][settings][allowed_view_modes][view_mode_2]" not found.

Branch test at 26 Jan 2023 at 12:42 GMT passes OK.

I think the failures here are unrelated because there was a branch test at 25 Jan 2023 at 11:48 GMT with the same failure in ests/src/FunctionalJavascript/CKEditor5AllowedTagsTest
https://www.drupal.org/pift-ci-job/2576653

If they’re ckeditor5 functionalJavascript most likely random.

Yes, either random or unrelated to this MR, core branch did have a failure ealier. Re-ran after new rebase and all passes here now.

Are we in principle agreed that the expanding details element I added in #82 in response to @alexpott in #77 is the right solution? It works for me, but would be good to get confirmation as many others have looked at this issue before I started on it.

I am going to expand the test coverage for the uninstall page to check the Claro template in addition to the system default, as it was only by luck that the HelpTopic tests used the Claro theme and tested uninstalling, which showed the error in my change.

I have expanded the test to cover Claro theme in addition to the default. This will fail, demonstrating that the Claro template needs work. Also attempting to run only the 'system' module tests temporarily, for speed and efficiency.

As expected, we now have one test failure:

There was 1 error:
1) Drupal\Tests\system\Functional\Module\UninstallTest::testUninstallPage with data set #1 ('claro')
Behat\Mink\Exception\ElementNotFoundException: Element matching css "tr[data-drupal-selector="edit-module-test"] .module-machine-name.table-filter-text-source" not found.

This shows that the new claro template system-modules-uninstall.html.twig is now being tested when it was not before.

Tests now pass. Here are few other tweaks to the claro template, to make it align better with both the Claro install template, as I based the original template only on system uninstall. This is now ready for full review again.

Functional this works like a charm.

Searching for dynamic_page_cache I get nothing
Apply the fix and not I get the module.

Tagging for CR updates as there seems to be additional changes to the system template that others may need to adopt for their custom themes.

Updated the issue summary to match the solution.

I made some updates to the change record but I haven't fully updated it as suggested in #91.

Thanks. The change record still refers to adding a new column, but that is not how the solution is. Since #77 the description is now a details element, collapsed by default, but expandable to show machine name and requirements, similar to what is done on the install page.

Sorry I can't update the change record right now, happy for someone else to do it, or I will next week.

@jonathan1055 can you update the MR for 11.x

The tests on MR 3273 were OK but now we get one failure
https://git.drupalcode.org/issue/drupal-2895388/-/jobs/564727#L1443

Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest::testWidgetViews
    "Loading grid view." not found
    Failed asserting that a boolean is not empty.

Is this related, or some other problem?

I have created a new MR against 11.x along with some minor coding standard fixes. Also, upon reviewing the changes,
description table header of core/modules/system/templates/system-modules-uninstall.html.twig uses the priority-medium class while core/themes/claro/templates/admin/system-modules-uninstall.html.twig uses priority-low class. Was this intentional, or was it just a mistake?

Multiple MRs not sure what's to be reviewed. Should have 1 MR visible or specify which is the one to be reviewed.

I have hidden MR3273 as that was the original one, on 10.1.x so will not be committed.

@Akhil Babu, can you confim that your first commit on the new MR 6065 for 11.x is exactly the same as was on the 10.2.x MR?

@jonathan1055 Yes, The first commit in 6065 has all the changes from MR 5496. The next 2 commits are for coding standard fixes.

Thanks @Akhil Babu, it looked like that, but better to ask the person who actually did it.

Your quesion in #102

description table header of core/modules/system/templates/system-modules-uninstall.html.twig uses the priority-medium class, while core/themes/claro/templates/admin/system-modules-uninstall.html.twig uses priority-low class. Was this intentional, or was it just a mistake?

I recall that it was difficult to decide which template to base the new one. Should the Claro uninstall match the Claro install or the System Uninstall? I used priority-low because Claro install had that. My thinking was that it was more important to be consistent with Claro, rather than between the two uninstall templates. But happy to change it if that was the wrong decision.

Moving to NW for change record updates.

Also believe we need a template for some of the other themes, stable9, maybe starterkit (not 100% on that).