Use ARIA disclosure pattern for submenu buttons in vertical toolbar orientation

Made MR based on the following feedback from @maxstarkenburg in #accessibility Slack:

1. Remove the ["Expand/Collapse" text] because it's redundant with information a screen reader would already announce via aria-expanded.
2. Depending on who you ask, the button's accessible name (potentially via aria-labelledby) ought to be "[sibling link text]" or "[sibling link text] sub-menu" or "More [sibling link text]".
3. Also, personally, I would recommend not letting the effort to add aria-controls block this issue (perhaps could be made into a separate issue?), since adding aria-expanded and removing the incorrect state would both be easier wins, adding value sooner. aria-controls seems to have limited AT support and/or be semi-"controversial" in its usefulness, see e.g. https://heydonworks.com/article/aria-controls-is-poop/ and https://github.com/w3c/aria/issues/995

Know it's a task but possible to add a simple assertion that checks the aria value. So we can ensure we don't break this.

I re-rolled the patch from #4 to work with Drupal 10.4.x

Adding Needs Tests based on #19, and related issue.

I know nothing about ARIA recommendations, but on #3286466-50: Tabbing order does not satisfy 508 accessibility requirements → , @rkoller suggested using aria-pressed instead of aria-expanded:

... But i wonder if aria-expanded is the right choice for a toggle button. I always have problem as a sighted user understanding the current state for toggle buttons that change their label inbetween states. A point that Leonie Watson also illustrated in their talk for smashing magazine: https://youtu.be/OUDV1gqs9GA?t=3222 . She advocated to use aria-pressed instead. That way the button state is either pressed/selected or not and the button label remains the same between states. That way things are completely clear and unambiguous for screen reader users?

Does that apply here? Should we consider it?

@andrewmacpherson: It has been more than 5 years since you self-assigned this issue and last commented. I am un-assigning it now.

@benjfisher

...@rkoller suggested using aria-pressed instead of aria-expanded...
Does that apply here? Should we consider it?

The place in the video that @rkoller linked is regarding a button that toggles light / dark mode. aria-expanded wouldn't apply to that, I believe, because in that use-case there's nothing to expand or show.

Later in the video, there's an example of a show / hide toggle button with aria-expanded. The video creator appears to approve of that use.

There's a separate question of whether to change the "label" (visual display) when the toggle is activated. I can't speak to that. I find changing icons for show / hide widgets (like the orientation of a caret) to be helpful.

@kentr:

Thanks for the reply (here and on the related issue).

I will try to find time to add some tests coverage for this issue. From the comments, that is the only thing holding it back. (The Remaining tasks list "Update JS + templates". I think that is already done, although the current MR changes only one JS file, no templates. If I have that right, can someone that item?)

If it's in scope, I think this related test is passing falsely and needs fixing.

I've observed that the condition that appears to symbolize the appearance of the submenu after clicking will be true even when there's no click.

I checked this in the browser when the submenu is collapsed, and in the test by commenting out the click on the submenu.

I believe it needs a check before the click to confirm that the item is not visible, and then a check after the click to confirm that it has become visible. It looks like the checkVisibility() function might work for this.

It might also be checking the wrong submenu. When testing in the browser with that test code and the nightwatch_testing installation profile, I observe that the click is on the Configuration menu rather than the Reports menu. #toolbar-link-user-admin_index is one of the items that become visible.

@kentr:

I confirmed what you said about toolbarApiTest.js, and I agree it looks wrong. But I do think it is out of scope for this issue. Also, I am not sure what the difference is between that test and toolbarTest.js: what does "Tests of the existing Toolbar JS Api." mean? So let’s leave that for a separate issue.

I added a few lines to toolbarTest.js, checking that the aria-expanded attribute changes as expected. I did not check the aria-labelledby attribute, nor that the inner HTML of the <button> element is empty. I can test either or both of those if anyone thinks it is needed.

I also updated the "User interface changes" section of the issue summary.

I am leaving the status at NW for two reasons:

1. I think the "Proposed resolution" needs to be updated. The current version mentions "Use a constant name for the button:"parent-link-title sub-menu"." That is not what the current MR does. I also added the issue tag for that.
2. There is a CSS rule in toolbar.icons.theme.css (in the toolbar module and in two of the themes) that sets text-indent to a large, negative number for the body of the <button> element. Now that the body is empty, I think we can remove that rule.

I updated the proposed resolution and added a todo item for the CSS. I believe this addresses the issue summary updates requested in #28.

I don't have time to do a full review, so I'm not striking out any of the todo items.

Adding Needs change record because the policy requires it for changes that affect UI / UX → .

The MR pipeline failed. It looks like it's from the JS size change and the value for ScriptBytes in AssetAggregationAcrossPagesTest.php needs updating. Removing the CSS may also require a change there.

At my day job, we use the admin_toolbar module, which overrides how the menu links are rendered. The result is that they do not have id attributes, which makes the changes from this issue ineffective.

I added ✨ Menu links should have id attribute Active for the admin_toolbar module, but I am not sure it will be accepted. There may be a reason (performance?) for not including id attributes in the admin_toolbar module. In that case, is there any other value we could use here for the aria-labelledby attribute?

RE #30, I suggest:

1. Don't use aria-labelledby.
2. Revert to having the label component in the button text.
3. Keep the action component out of the button text to resolve the problem described in the IS.
4. Add an assertion in the test to confirm that the button accessible name equals the text of the corresponding link (the link that was used for aria-labelledby).

I think that will also resolve @nod_'s comment in the MR.

I've made some suggestions on the MR for these.

Rationale

I'm guessing that aria-labelledby was a result of @Max Starkenburg's comment in Slack:

After further reflection (and defaulting to trusting Andrew's judgement on something like this), I'm realizing that it probably indeed made sense to try to remove a description of the state from the button, since I believe most screen readers will already announce the change in the aria-expanded value anyway (e.g. VoiceOver seems to announce "[accessible name], collapsed, button" when tabbing to such a button).

That led me to wonder what the accessible name should be. I sought out "what would Adrian Roselli do for such disclosure widgets?" leading to https://adrianroselli.com/2019/06/link-disclosure-widget-navigation.html#Name, in short, to use the name of the sibling link (via aria-labelledby). I think Andrew's patch was to use "[sibling link text] sub-menu", and looks like WAI's example uses "more [sibling link text]". I think neither of those quite contribute to the same type of verbosity problem Adrian reneged on (which seems to have been to toggle "show [sibling link text]" and "hide [sibling link text]").

Sorry, disregard the image attachment in my last comment. It was an accidental holdover from an earlier version of the comment.

I updated the MR, removing the CSS rule that is no longer needed if the button has no text (inner HTML). I remembered (barely) to update the .pcss.css files along with the .css files. I updated toolbar.icons.theme.css in the toolbar module and the claro theme.

After a little discussion on Slack, a few days ago, with @nod_, we agreed that the stable9 theme should not be changed. It already has its own version of toolbar.icons.theme.css. I added a copy of toolbar.menu.js (before any changes from this issue) to the theme, and updated the libraries-override section in its .info.yml file.

Testing: I used drush generate theme to create a sub-theme of stable9. I enabled that and set it as the admin theme. I checked that the old markup was used on admin pages and the new markup was used on non-admin pages.

When I finished that, I saw that @kentr had made some suggestions on the MR, perhaps in response to @nod_'s comment there. I think it will be easier to discuss those suggestions here instead of on the MR. From the issue summary, the current markup (11.x) for the button (when the submenu is collapsed) is

<button class="toolbar-icon toolbar-handle" style=""><span class="action">Extend</span> <span class="label">Structure</span></button>

Using the current MR, that changes to

<button aria-expanded="false" aria-labelledby="toolbar-link-system-admin_structure" class="toolbar-icon toolbar-handle" style=""></button>

I think @kentr proposes changing that to

<button aria-expanded="false" class="toolbar-icon toolbar-handle" style=""><span class="label">Structure</span></button>

That is, remove the aria-labelledby attribute and restore the text inside the button (inner HTML).

I am not an accessibility expert, so I have no opinion on that suggestion. If we can get consensus on what the markup should be, I am happy to update the MR. (Among other things: if we restore the text, then the CSS rule is no longer redundant.)

I'm not an accessibility expert either.

I think the underlying problem with the button text is that there's also text indicating what the button will do (the "action" component) and that it is the opposite of the "state" of the submenu and what's indicated by the icon. From the IS:

the [accessible] name of the button is the opposite of the current state. There's some unnecessary cognitive effort there, to figure out what the current state is.

When the submenu is collapsed, the icon shows that but the text in the button text says "Extend [sub-menu]".

Using aria-labelledby to avoid repeating "[sub-menu]" in the button text would be convenient if it worked, but maybe it's not feasible.

Using the aria-expanded state instead of words like Extend/Collapse in the button name is definitely the right thing to do.

Regarding what the button names should be, we've found in user testing of a main navigation with a similar design that there are some assistive technologies (e.g. ZoomText) that will convey the name but not the role so both the link and button would simply be called "Structure." Therefore, I think including "Menu" at the end of the button names would help differentiate the links and buttons and better convey the button's purpose. Since aria-labelledby can reference multiple ids, " Menu" can be added after the text taken from the link.

However, a reason to use visually hidden text within the buttons to name them is client-side language translation is more likely to be accurate. If the button for "Site settings" uses aria-labelledby to reference the link's text and another element containing just the word " Menu," the name and what a screen reader will say is "Site settings menu." However, if the page is client-side translated to French, the two strings are translated separately so it would say "paramètres du site menu."

If the button instead had the visually hidden text "Site settings menu," the translation would be "menu des paramètres du site" which would be more expected and more easily understood. I think the aria-labelledby version would still be understood, I don't know if there are languages for which the meaning would be lost if the words would not translated all as one string.

I'd add that a reason to not name the buttons using an attribute string like aria-label="Site settings menu" is translation software tends to skip aria-label entirely but does translate both visually hidden text and text hidden from everyone (with the hidden attribute, display: none styles, etc.)

Thanks for the guidance, @cwilcox808. I have updated the MR based on Comment #35. (I hope I got it right.)

I think sentence case (not title case) is the Drupal standard, so I made it "Content menu", "Structure menu", etc, not "Content Menu". Would "submenu" be clearer than "menu", or would that just be an additional, unhelpful syllable?

I am updating the issue description. At the end of the Proposed resolution section, I have this:

Screen reader announcements will be along the lines of "structure menu, expanded, button" and "structure menu, collapsed, button".

Is that right?

The performance test (Drupal\Tests\demo_umami\FunctionalJavascript\AssetAggregationAcrossPagesTest::testFrontAndRecipesPagesEditor()) failed with the message,

Failed asserting that 445441 ( is equal to 445601 or is greater than 445601 ) and ( is equal to 446601 or is less than 446601 ).

I think @kentr already mentioned this on Slack.

This is a case where updating the test is the right thing to do. This MR reduces the size of toolbar.menu.js by 214 bytes. Before that reduction, it was in range. By design, the performance test passes unless the number of bytes changes by more than 500, so I reset it to the current value, 445441.

@benjifisher I don't know if "submenu" would be more clear than "menu." Without additional information, I'd stick with the shorter term.

The current text of the Proposed solution looks good.

Instead of using a large negative text-indent to visually hide the text of the buttons, I would use the .visually-hidden utility class (probably on the inner span.label rather than the button itself) or the same properties that class uses. That collection of properties has proven to be more robust, avoiding issues with right-to-left languages, very large viewports or font-sizes, etc..

@cwilcox808:

Thanks for the review!

I did a little research. The text-indent rule was added in #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop → . Searching that issue, I see one mention of element-invisible, the class that was later renamed to visually-hidden, in Comment #288. But I do not see any argument against using the utility class.

I updated the MR here to use visually-hidden, and I restored the change that removes the obsolete text-indent rule. As I said in #33, the stable9 theme already overrides the CSS file, so I do not have to worry about keeping that theme stable.

... probably on the inner span.label rather than the button itself

We definitely do not want to make the button itself visually-hidden. That would hide the icon.

I am updating the snippets in the issue summary to add the `visually-hidden` CSS class.

I've checked the latest changes on a test install and they look good to me.

I'd love to have an assertion for the (accessible) name of the button because the choice was very deliberate.

After a little discussion on Slack, a few days ago, with @nod_, we agreed that the stable9 theme should not be changed. It already has its own version of toolbar.icons.theme.css.

Wanted to double-check because removing "Extend" and "Collapse" was a big part of the original issue: Was the intent to keep "Extend" and "Collapse" in the names for stable9?

Actually two assertions, to show that the name stays constant with the toggling (i.e., to show that we fixed that problem also).

Wanted to double-check because removing "Extend" and "Collapse" was a big part of the original issue: Was the intent to keep "Extend" and "Collapse" in the names for stable9?

That is my understanding.

Any changes to the stable9 theme have the potential to break custom themes based on it, and perhaps cause automated tests to fail. We normally think in terms of the CSS in the theme, but I think it applies to JS, too.

I am removing the tag for an a11y review, thanks to @cwilcox808's comments.

I am setting the status to NW for additional test coverage (Comments #43, #44). I will try to get that done soon, but I have a 1-week vacation coming up. If I do not find time in the next two days, then it will be at least another week before I do it.

It took two tries, but I added the assertions for the button text, before and after toggling.

Unless I totally misunderstand the nth-child selector, my mistake was to look for the second item in the admin menu after installing the Standard profile. That is Structure. I am not sure which profile the Nightwatch tests use, but it seems that the second item is Configuration.

Back to NR.

I think checking the name with .getAccessibleName() instead of checking the button text would be more robust.

I understand the goal to be presenting a usable name for AT. The name is currently in the button text, but it could get overridden in the future by aria-label or aria-labelledby. Checking with .getAccessibleName() should catch that.

Since @benjifisher said he'll be away, I'll make that change and let it go through review.