Module name context missing for 'help' and 'permissions' links on admin/modules

+++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
@@ -44,10 +44,10 @@ public function testModuleListForm() {
-    $this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/people/permissions/module/system_test') and @title='Configure permissions']");
+    $this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/people/permissions/module/system_test') and text()='Permissions ']/span[contains(@class, 'visually-hidden') and text()='for System test']");
 
     // Check that system_test's help link was rendered correctly.
-    $this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/help/system_test') and @title='Help']");
+    $this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/help/system_test') and text()='Help ']/span[contains(@class, 'visually-hidden') and text()='for System test']");

This seems like a small regression to me, why remove the test for the tooltip?

With that noted, 2.4.9 includes the advisory technique H33: Supplementing link text with the title attribute. Why make this change and keep the title/tooltip? It no longer adds anything to the link - especially if you move the "configure" verb into the visually-hidden part of the permissions link.

Good plan to get this addressed by experts.

If two spans is icky (I agree) how about <span>Configure @module module</span> Permissions?

Links with verbs are good, yes. Perhaps "view" rather than "get"? Or even "read"?

To me "get" implies active rather than passive help, and documentation is most definitely passive help.

I'm guessing the accessibility team (we have one of those right?) will have input on that too.

We look at this issue today at #3245345: Drupal Usability Meeting 2021-10-29.

The recommendation is that we use the format [Action] for [module] so that would be for example Permissions for Views, with the visually hidden part being for [module]. The configure link being the exception, which we felt would be fine with Configure [module].

This is consistent with the practice of less is more, and from an accessibility perspective - particularly for screen reader users - keeping the link text short is beneficial, especially on the modules page where there is a lot of text and a lot of links. If you consider the modules page with just core modules, removing even just one unnecessary word from the link text would save a significant amount of time when tabbing through the entire page.

Presenting the action word first, aka the visible part of the link text, ensures that for someone quickly tabbing through the text on the screen, they can navigate through those links quickly, only needing to hear the first word to know where the link goes.

Keeping the format of [Action] for [module] or [Action] [module] also means the text on all of the links is consistent and avoids confusion. For example, the proposed text of Configure [module] Permissions could be easily confused with the Configure [module] link, especially if a screen reader user is quickly tabbing across the links because they would essentially hear two configure links and it would be very easy to then select the wrong one.

Hi Derek

Thanks for adopting our recommendations! We also discussed the title attribute during the UX review, after looking quickly at the accessibility guidance, we agreed that the title attribute was not appropriate and so could be removed, however that did not form part of our formal recommendation because we agreed to defer to our accessibility maintainers on that point.

If there is agreement on the accessibility side that the title attribute is not required, then it sounds like we are all good to remove it.

Thanks,
-Aaron

@AaronMcHale thanks so much for taking the time to comprehensively explain the decision, I definitely learned something today.

@dww thanks for making two patches! That should really help it get past the accessibility review, which, correct me if I'm wrong, only happens once an issue has gone RTBC?

I was about to turn this green to get it in the queue for that, but I noticed:

+++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
@@ -41,13 +41,13 @@ public function testModuleListForm() {
-    $this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/people/permissions/module/system_test') and @title='Configure permissions']");
+    $this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/people/permissions/module/system_test') and text()='Permissions ']/span[contains(@class, 'visually-hidden') and text()='for System test']");
 
     // Check that system_test's help link was rendered correctly.
-    $this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/help/system_test') and @title='Help']");
+    $this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/help/system_test') and text()='Help ']/span[contains(@class, 'visually-hidden') and text()='for System test']");

We're still clobbering the test of the title attribute here in the with-title patch.

I agree the no-title patch is the best way forward so the risk of being sent back to fix the (now hidden) with-title patch is minimal.

Let's see how we go :)

I stand corrected :) thank you

@dww Patch giving error while appliying in drupal-9.3.x-dev
Needs to Re-roll -----

Checking patch core/modules/system/src/Form/ModulesListForm.php...
warning: core/modules/system/src/Form/ModulesListForm.php has type 100755, expected 100644
Hunk #1 succeeded at 257 (offset -1 lines).
error: while searching for:
if ($module->status && $this->currentUser->hasPermission('administer permissions') && $this->permissionHandler->moduleProvidesPermissions($module->getName())) {
$row['links']['permissions'] = [
'#type' => 'link',
'#title' => $this->t('Permissions'),
'#url' => Url::fromRoute('user.admin_permissions.module', ['modules' => $module->getName()]),
'#options' => ['attributes' => ['class' => ['module-link', 'module-link-permissions'], 'title' => $this->t('Configure permissions')]],
];
}

error: patch failed: core/modules/system/src/Form/ModulesListForm.php:268
error: core/modules/system/src/Form/ModulesListForm.php: patch does not apply
Checking patch core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php...
warning: core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php has type 100755, expected 100644
error: while searching for:
$this->drupalGet('admin/modules');

// Check that system_test's configure link was rendered correctly.
$this->assertSession()->elementExists('xpath', "//a[contains(@href, '/system-test/configure/bar') and text()='Configure ']/span[contains(@class, 'visually-hidden') and text()='the System test module']");

// Check that system_test's permissions link was rendered correctly.
$this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/people/permissions/module/system_test') and @title='Configure permissions']");

// Check that system_test's help link was rendered correctly.
$this->assertSession()->elementExists('xpath', "//a[contains(@href, '/admin/help/system_test') and @title='Help']");

// Ensure that the Database Logging module's machine name is printed. This
// module is used because its machine name is different than its human

error: patch failed: core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php:41
error: core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php: patch does not apply

I reviewed the issue. Agreed that @dww correctly described a bug. Titles are a challenge for assistive technology as they have generally been very badly implemented. Removing them and moving to describing the function as per the patch is the right way to go. It also makes it more consistent with the Configure link which is currently done differently than the other links. That helps with usability for everyone.

I couldn't test the patch on SimplyTest.me but it looked fine to me. Lets get the bots to verify this.

Removing accessibility review.

This has passed accessibility review and the code looks good to me, so marking RTBC.

Crediting the usability team for discussing this issue. It's helpful if comments like #16 contain who to credit as I have no idea who was active in the discussion of this issue.

Committed and pushed 67b083bfe70 to 10.0.x and da50a859252 to 9.4.x. Thanks!