Problem/Motivation

Secondary menu toggles all have the same text: "Toggle sub-navigation", which doesn't describe the purpose as well as it could for the following reasons:

  • Depending on the assistive tech used, the toggle may not be associated by the user with the menu item that is next to it. Only sighted users are assured to know that the toggle is associated with "articles" in some capacity

You can reference the menu toggles at https://www.adobe.com/. Unlike Olivero, these are visible, but the same principle applies - the button text should inform the user/assitive-tech of what it will be toggling.

Proposed resolution

Use different names for the top level buttons. Use the link name, and append " sub-navigation". Note the hyphen.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 3190140-8.patch833 bytesnmorin
#5 Screenshot_2_20_21__1_57_PM.png114.35 KBmherchel
#4 3190140.patch827 bytesnmorin
Screen Shot 2020-12-28 at 2.54.35 PM.png8.84 KBbnjmnm

Comments

bnjmnm created an issue. See original summary.

andrewmacpherson’s picture

andrewmacpherson commented
Issue summary: View changes

Particularly when #3190137: Refactor aria implementation of collapsible nav lands, the aria implementation will already be letting assistive tech know menu items are toggled by the button.

That was a red herring. I've closed #3190137: Refactor aria implementation of collapsible nav, so I'm removing references to that from this issue summary.

You can reference the menu toggles at https://www.adobe.com/

The navigation at adobe.com isn't a good match for what we're building. That navigation consists of a row of buttons only. However, we have alternating links and buttons.

The button name I would suggest is "top-level-link-namesub-menu". So there would be a link called "articles", followed by a button called "articles sub-menu".

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue tags: +FLDC2021
nmorin’s picture

nmorin commented
Status: Active » Needs review
StatusFileSize
new 3190140.patch827 bytes

Patch attached

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot_2_20_21__1_57_PM.png114.35 KB

This looks perfect. Screenshot attached!

mherchel credited nmorin.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Disregard this comment

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Discussed this with @andrewmacpherson and @lauriii. We debated the use of the word "sub-menu". There was discussion about whether this was a menu or collection of links in a navigation. And also of sub-menu vs submenu. Believe it or not submenu is in the dictionary. See https://www.merriam-webster.com/dictionary/submenu - and has usage in https://www.smashingmagazine.com/2017/11/building-accessible-menu-systems/

This should either be changed to:
'@title submenu'
Or
'@title sub-navigation'

I think I prefer the second one because this avoids the discussion of what is a menu and this is part of the site's navigation.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: +Novice

Tagging with 'Novice' since addressing #8 would be a good task for a new contributor.

nmorin’s picture

nmorin commented
StatusFileSize
new 3190140-8.patch833 bytes
nmorin’s picture

nmorin commented

Added patch, '3190140-8.patch' for Comment #8

nmorin’s picture

nmorin commented
Status: Needs work » Needs review

Changed to "Needs review" after adding patch for Comment #8

(P.S. Sorry for multiple messages; Novice contributor here.)

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community

#10 looks perfect. Thanks @nmorin!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.2.x-dev » 9.1.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed and pushed 209dcea4fa to 9.2.x and a76c74c84e to 9.1.x. Thanks!

Backported to 9.1.x as olivero is experimental.

diff --git a/core/themes/olivero/templates/navigation/menu--primary-menu.html.twig b/core/themes/olivero/templates/navigation/menu--primary-menu.html.twig
index 89350997b6..e1217db352 100644
--- a/core/themes/olivero/templates/navigation/menu--primary-menu.html.twig
+++ b/core/themes/olivero/templates/navigation/menu--primary-menu.html.twig
@@ -84,7 +84,7 @@
                 drop menu).
               #}
               <button class="primary-nav__button-toggle" aria-controls="{{ aria_id }}" aria-expanded="false" aria-hidden="true" tabindex="-1">
-                <span class="visually-hidden">{{ '@title sub-navigation' |t({'@title': item.title}) }}</span>
+                <span class="visually-hidden">{{ '@title sub-navigation'|t({'@title': item.title}) }}</span>
                 <span class="icon--menu-toggle"></span>
               </button>

I removed the space between the ' and the | - I checked core and it's not normally there.

  • alexpott committed 209dcea on 9.2.x 
    Issue #3190140 by nmorin, mherchel, bnjmnm, andrewmacpherson:...

  • alexpott committed a76c74c on 9.1.x 
    Issue #3190140 by nmorin, mherchel, bnjmnm, andrewmacpherson:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.