Machine-name field is not effectively hidden from keyboard and screen reader users.

I don't know if this is possible at this time. I believe the behavior you are describing is happening because a link is listed as hidden or invisible. The browser at this time will still navigate to an invisible element and I am unaware of any pattern or best practice that overrides this.

I agree it's annoying.

Thanks cortezj + kershme, I managed to replicate this.

There is a visually-hidden text input for editing the machine name, in between the "Edit" button and the "Description" text area.

Pressing the Edit button reveals the "Machine-readable name" text input field, by removing a visually-hidden CSS class from a wrapper <div>. The problem is that when the machine-readable name field is visually hidden, it still exists in the tab order for the page.

We can fix this. Instead of using our .visually-hidden CSS style to conceal the field, we should be removing it from the tabbing order and accessibility tree completely, using CSS display: none.

Drupal 8 already has some CSS utility styles for this. We just need to use .hidden instead of .visually-hidden.

I propose we try "hidden" class instead of "visually-hidden" class in machine-name.js, lines 53 and 112.

This mainly a problem for sighted keyboard-only users who can focus the machine-name input, but not see it on the
screen.

It also means that the machine-name field is presented to screen reader users, to whom it is announced.

Updating issue summary, for whoever works on fixing this. It's a good introductory issue for learning about keyboard accessibility.

minor issue summary edit.

Patch in core/misc/machine-name.js changing "visually-hidden" to "hidden"

Address test failure.

This is designed for screen reader I think. So that they able to read the machine name change? After patching, it required Drupal.announce to speak it out.

Having problems with my local test environment so I'm just gonna throw this one out there.

Completely hidden elements should be tested with \Zumba\Mink\Driver\PhantomJSDriver::isVisible

@droplet Not too sure if this should be done. Have you read this comment

     // Validate the machine name field is hidden. Elements are visually hidden
     // using positioning, isVisible() will therefore not work.

With current patch, the comment is wrong. Needs a fix on it.

But again. @see my comment on #18. I think that needs a review and further tweaks and new tests.

Per #21, #18

Changing the parent issue, making it part of a wider a11y review.

Reroll of patch for use with Drupal 9.3.x

Thanks for commit.
Applies and works on 9.3.

Test fails, because the ES6 version of the file is not modified. Working on a fix now.

I took the necessary changes made in #35 and applied them to the ES6 file too. Also provided a patch version for 9.4.x.

(Sorry, could not generate an interdiff because of a whitespace error.)

I've had found two separate issue about the machine name problem. for one content types and then for adding block types. i've searched upfront in the course of finding issues in regards of the focus visible meta issue for Claro https://www.drupal.org/project/drupal/issues/3163810 . I've closed both issues marked as duplicate thanks to the notification @tinto who dug up and found the issue. i set this issue as a related issue to the focus visible issue in claro since it is a general drupal issue and not an claro exclusive one.

in regards of #38. i've tested the patch manually against 9.4.x, i was able to apply it and everything worked as expected. i've also did the same with the updated patch for 9.3.x, with the exact same results. so from a manual testing perspective i would say the patch is good to go. from a code perspective i wouldn't consider myself fully qualified (am into content strategy and am not a coder). but except the tests which failed and need some work the rest looks none invasive and correct from my perspective. but a second opinion more knowledgable and code savvy would be welcome to be completely sure :)

Made some additional changes to patch #38 for 9.4.x in an attempt to get the test passed.

Tested this successfully on my local 9.4.x install.

thank you! i've applied the patch to a 9.4.x install. the patch still applies and the functionality behaves still as expected. so nothing changed function wise from the patch in #38. i've waited with my reply until the tests were finished and they are all on green now :) so your fixes there worked as well. and since the tests mostly changed visually-hidden to hidden according to the diff in the patch and others have checked those tests with visually-hidden before i would say the issue could be set to RTBC. the only change i make is i set the version from 9.3.x to 9.4.x for the issue since the patch is for 9.4.x now :)

Thank you for the quick test and follow-up!

@rkoller, you are correct. Patch #41 basically changes ./core/misc/machine-name.js so that the class that is provided on the form element div of the machine name field is now 'hidden' instead of 'visually-hidden'.

I think this approach is correct for this scenario, especially after reading the Drupal documentation about how to hide content properly. It states the following:

If an element on the page:

- shouldn't be shown until a JavaScript event makes it visible,
- isn't relevant to sighted users or screen-reader users in the context,
- contains values that are read/written by JavaScript but should never be shown directly, or,
- in general, shouldn't be visible to sighted users or screen-reader users,

... then you should completely hide it for all users.

You can do this by:

- giving the element the hidden class (in Drupal 8 or higher),
- (...)

See: https://www.drupal.org/docs/accessibility/hide-content-properly

I think we need to address #18 directly. I've read all the comments on the issue and not found a response.

I have tested #41 with Chrome Screen Reader and it works for the machine-name field, but only after it has been revealed (by clicking the Machine name: ... edit button).

I think this makes more sense than having an invisible element that gets picked up by screen readers and causes invisible focus when tabbing through the page. However, I'm no accessibility expert, so I hope someone with more knowledge on the matter can chime in.

P.S. The machine name edit button could use some improvements, as my screen reader currently describes it as "button, edit", not indicating what it is that a user will be editing. Perhaps this warrants a separate issue, depending on the outcome of this one.

alexpott asked me to review this, assigning myself. I'd like to test this manually, and remind myself where the moving parts are.

Meanwhile...

1. I'm wary of #18's suggestion to use Drupal.announce. I think that's likely to be overkill, because
   • Screen readers can already be configured to echo text entry, and many do by default. If a user has just edited a text field, they don't need an additional announcement of what they've just typed. We don't do this for other text fields.
   • If a user has configured their screen reader not to echo text entry, then Drupal.announce is just disrespecting their preferences.
2. #21-24 hasn't been addressed yet. This comment in the test is no longer accurate, because the hiding method is changing.

        // Validate the machine name field is hidden. Elements are visually hidden
        // using positioning, isVisible() will therefore not work.
   

3. #45 makes an excellent point about the button name. I'd fix that in this issue, not defer it until later. Make the computed accessible name "edit machine-name".

Thank you for your feedback @andrewmacpherson

Here's a new patch that contains the necessary changes in order to address #21-24, so that MachineNameTest.php now uses isVisible() to check visibility.

Concerning #46.3: I propose to add an aria-label="edit machine-name" attribute to the button element. For my own sanity, I will create a separate patch for this additional change after #47 passes testing.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
@@ -104,19 +104,18 @@ public function testMachineName() {
     // Validate the visibility of the second machine name field.
-    $this->assertEquals(TRUE, $machine_name_2_wrapper->hasClass('visually-hidden'), 'The ID field must not be visible');
+    $this->assertTrue($machine_name_2_wrapper->isVisible(), 'The ID field must now be visible');

This should be assertFalse(), the second machine name field should still be hidden after pressing the button for the first field.

Edit: Oh and the comment should remain 'The ID field must not be visible'

Thank you @Lendude. Applying your suggested changes.

Looks good from a code point of view, setting back to 'needs review' for a review from @andrewmacpherson

I think the last remaining task is to address my PS in #45 (and #46.3). Working on that now.

Adding an aria-label to the Machine name: ... [Edit] button, so that screen readers now say "Edit machine name, button" instead of "Edit, button".

Attempting to fix automated testing error

Edit: fixed!

@tinto code looks great, could you update the Issue summary too with the arguments for the added change to the aria-label?

Updating the issue summary by with an explanation of the aria-label addition.

the patch from #54 probably needs a reroll. i was unable to apply it to a fresh install of Drupal 10.1.x-dev with composer-patches. The apply failed.

Opened merge request.
Ready for review.

thanks for the fast reroll the patch successfully applies in 10.1.x-dev now! i did a brief basic manual test and it looks like there is a regression to the previous patch by @tinto now. i've tested on /admin/structure/types/add. if i tab through the form the outline disappears in between the name and the description field when the not visible machine name field gets into focus.

Believe the issue is because the description field has a tabindex of -1

From what I can tell this is coming from the tabbingmanager.js line 237

        // Make all tabbable elements outside of the active tabbing set
        // unreachable.
        $disabledSet.prop('tabindex', -1).prop('autofocus', false);

For testing I commented that line out. And when I go to the add content type page I don't lose focus and it goes into description field.

Disregard #65. That was because of #2991686: Enabling "Edit" to show all contextual links breaks tabbing in edit forms in the backend

Fixed the issue with the reroll.

Hi i Applied patch#54 on Drupal 9.4.x it works and issue got fixed. adding videos for the reference.

re #66 oh that is interesting. first I can confirm that your changes fixed the issue that the focus outline was hidden again for one tab press as soon as the edit link got into focus while hidden (tested on 10.1.x-dev). awesome thanks! the only odd detail is, that the patch worked for 9.4, after the reroll the focus not be visible for one tab problem turned up again, but the root cause you have identified for fixing that regression with #2991686: Enabling "Edit" to show all contextual links breaks tabbing in edit forms in the backend is already existing for much longer, so i wonder why it hasnt applied to the patch in 9.4.x (but that shouldn't stop this issue - i was just thinking out loud out of curiosity).

The only thing left to do is just a formal accessibility review and sign off by @andrewmcpherson who assigned himself before but got unassigned in the course by people working on the changes for the reroll. but i am unable to re-assign the issue to him.

Thanks! I've applied the merge request (3071) with the lastest changes to a Drupal 10.1.x-dev install. I've tested on MacOS with Safari 16.1 and the latest versions in Firefox and Edge. I can confirm that the regression was fixed and the focus outline is displayed as intended again.

I've noticed after i've posted my initial reply that another merge request (3119) was opened. That one fails to apply with composer patches unfortunately on my end. :(

I think we can classify this under WCAG 4.1.2.

Re roll patch for 9.5.x

Reviewing MR 3071

For accessibility

Before patch

1. Went to /admin/structure/types/add
2. While tabbing focus is lost between label and description

After patch
1. Went to /admin/structure/types/add
2. While tabbing focus goes between label and description with no in between
3. Backtab back to label and type "Test"
4. Machine name appears with Edit link and I can tab to it in this order: Label > edit link > description
5. Press enter on edit link. Can tab and backtab in this order label > machine name > description.

Issue appears to have been solved.

Thanks for testing this and describing the process you went through @smustgrave.

Based on this, I am going to remove the Needs Accessibility Maintainer tag so this issue can proceed.

Thanks! @mgifford

Committed 60cce59 and pushed to 10.1.x. Thanks!