From db25dc13e1493a38980eb242a607374888bfa13b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 26 Sep 2021 18:18:32 +0200 Subject: [PATCH] Use buttons to open/close admin navigation submenus We were using Foundation's accordion menu to open/close nested lists of links. Unfortunately, Foundation's accordion makes it impossible to access links in nested links using the keyboard [1] (note the issue is closed, but in the latest version of Foundation, 6.8.1, it's still present, and Foundation's development is mostly discontinued). Furtheremore, it adds the `menuitem` role to links, but ARIA menus are not ment for navigation but for application behavior and, since it doesn't add the `menubar` or `menu` roles to the parent elements, it results in accessibility issues for people using screen readers (also reported by the Axe accessibility testing engine). So we need to implement our own solution. We're using the most commonly used pattern: a buttton with the `aria-expanded` attribute. And, for people using browsers where JavaScript hasn't loaded, we're keeping the submenus open at all times (just like we were doing until now), and we're disabling the buttons (since they do nothing without JavaScript). This might not be an ideal solution, but it's probably good enough, and way better than what we had until now. We've also considered using the
and elements instead of using buttons to open/close items on the list. However, these elements still present some accessibility issues [2], and the transition between open and closed can't be animated unless we overwrite the `click` event with JavaScript. The pattern of using these elements to open/close a nested list of links isn't common either, and some people using screen readers might get confused when entering/leaving the nested list. We tried other approaches to get the animation effect, all of them based on adding `[aria-expanded="false"]:not([disabled]) + * { display: none; }` to the CSS file. Unfortunately, animation using CSS isn't feasible right now because browsers can't animate a change form `height: 0` to `height: auto`. There are some hacks like animating the `max-height` or the `flex-grow` property, but the resulting animation is inconsistent. A perfect animation can be done using the `grid-template-rows` property [3], but it requires adding a grid container and only works in Firefox and recent versions of Chrome and similar browsers. Getting to a solution with JavaScript was also tricky. With the following approach, `slideToggle()` opened the menu the first time, even if it was already open (not sure why): ``` toggle_buttons.on("click", function() { $(this).attr("aria-expanded", !JSON.parse($(this).attr("aria-expanded"))); $(this).next().slideToggle(); }); ``` This made the arrow turn after the menu had slided instead of doing it at the same time: ``` toggle_buttons.on("click", function() { var button = $(this); button.next().slideToggle(function() { button.attr("aria-expanded", !JSON.parse(button.attr("aria-expanded"))); }); } ``` With this, everything disappeared quickly: ``` toggle_buttons.on("click", function() { var expanded = JSON.parse($(this).attr("aria-expanded")); if (expanded) { $(this).next().slideUp(); } else { $(this).next().slideDown(); } $(this).attr("aria-expanded", !expanded); } ``` So, in the end, we're hiding the nested link lists with JavaScript instead of CSS. [1] Issue 12046 in https://github.com/foundation/foundation-sites [2] https://www.scottohara.me/blog/2022/09/12/details-summary.html [3] https://css-tricks.com/css-grid-can-do-auto-height-transitions --- app/assets/javascripts/admin/menu.js | 16 +++++++ app/assets/javascripts/application.js | 1 + app/assets/stylesheets/admin/menu.scss | 35 ++++++++------- app/components/admin/menu_component.html.erb | 2 +- app/components/admin/menu_component.rb | 44 +++++++++---------- .../management/menu_component.html.erb | 2 +- app/components/management/menu_component.rb | 9 ++-- spec/components/admin/menu_component_spec.rb | 22 ++++++++++ spec/system/admin/banners_spec.rb | 4 +- spec/system/admin/geozones_spec.rb | 2 +- spec/system/admin/poll/booths_spec.rb | 6 +-- .../site_customization/content_blocks_spec.rb | 6 +-- .../site_customization/documents_spec.rb | 2 +- .../admin/site_customization/images_spec.rb | 2 +- .../admin/site_customization/pages_spec.rb | 4 +- spec/system/admin/tenants_spec.rb | 2 +- spec/system/admin_spec.rb | 2 +- 17 files changed, 99 insertions(+), 62 deletions(-) create mode 100644 app/assets/javascripts/admin/menu.js create mode 100644 spec/components/admin/menu_component_spec.rb diff --git a/app/assets/javascripts/admin/menu.js b/app/assets/javascripts/admin/menu.js new file mode 100644 index 000000000..f6d91978a --- /dev/null +++ b/app/assets/javascripts/admin/menu.js @@ -0,0 +1,16 @@ +(function() { + "use strict"; + App.AdminMenu = { + initialize: function() { + var toggle_buttons = $(".admin-sidebar [aria-expanded]"); + + toggle_buttons.removeAttr("disabled"); + toggle_buttons.filter("[aria-expanded='false']").find("+ *").hide(); + + toggle_buttons.on("click", function() { + $(this).attr("aria-expanded", !JSON.parse($(this).attr("aria-expanded"))); + $(this).next().slideToggle(); + }); + } + }; +}).call(this); diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 86a0f8243..9e6031dd4 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -174,6 +174,7 @@ var initialize_modules = function() { App.AdminMachineLearningScripts.initialize(); App.AdminTenantsForm.initialize(); App.AdminVotationTypesFields.initialize(); + App.AdminMenu.initialize(); App.BudgetEditAssociations.initialize(); App.BudgetHideMoney.initialize(); App.Datepicker.initialize(); diff --git a/app/assets/stylesheets/admin/menu.scss b/app/assets/stylesheets/admin/menu.scss index 3a6cd7514..1993ff53a 100644 --- a/app/assets/stylesheets/admin/menu.scss +++ b/app/assets/stylesheets/admin/menu.scss @@ -10,7 +10,7 @@ padding: 0; } - > ul > li a { + > ul > li > :first-child { display: flex; font-weight: bold; @@ -124,16 +124,9 @@ border-left: 2px solid $sidebar-active; font-weight: bold; } - - &[aria-expanded="true"] { - - > a::after { - transform: rotate(180deg); - } - } } - li a { + li > :first-child { color: inherit; display: block; padding: calc(#{$line-height} / 2); @@ -146,19 +139,27 @@ } } - .is-accordion-submenu-parent { + [aria-expanded] { + @include has-fa-icon(chevron-down, solid, after); + line-height: inherit; + text-align: inherit; + width: 100%; - > a { - @include has-fa-icon(chevron-down, solid, after); + &::after { + margin-#{$global-left}: auto; + transition: 0.25s; + } - &::after { - margin-#{$global-left}: auto; - transition: 0.25s; - } + &:not([disabled]) { + cursor: pointer; } } - .submenu { + [aria-expanded="true"]::after { + transform: rotate(180deg); + } + + ul ul { border-bottom: 0; margin-left: $line-height; diff --git a/app/components/admin/menu_component.html.erb b/app/components/admin/menu_component.html.erb index 5b4a80597..0edc468b4 100644 --- a/app/components/admin/menu_component.html.erb +++ b/app/components/admin/menu_component.html.erb @@ -1 +1 @@ -<%= link_list(*links, id: "admin_menu", data: { "accordion-menu": true, "multi-open": true }) %> +<%= link_list(*links, id: "admin_menu") %> diff --git a/app/components/admin/menu_component.rb b/app/components/admin/menu_component.rb index bc4791c3a..480be1b89 100644 --- a/app/components/admin/menu_component.rb +++ b/app/components/admin/menu_component.rb @@ -60,7 +60,9 @@ class Admin::MenuComponent < ApplicationComponent def customization? controllers_names = ["pages", "banners", "information_texts", "documents", "images", "content_blocks"] - controllers_names.include?(controller_name) || homepage? || pages? + + (controllers_names.include?(controller_name) || homepage? || pages?) && + controller.class.module_parent != Admin::Poll::Questions::Answers end def homepage? @@ -151,13 +153,13 @@ class Admin::MenuComponent < ApplicationComponent end def booths_links - section(t("admin.menu.title_booths"), class: "booths-link") do + section(t("admin.menu.title_booths"), active: booths?, class: "booths-link") do link_list( officers_link, booths_link, booth_assignments_link, shifts_link, - id: "booths_menu", class: ("is-active" if booths?) + id: "booths_menu" ) end end @@ -205,13 +207,13 @@ class Admin::MenuComponent < ApplicationComponent end def messages_links - section(t("admin.menu.messaging_users"), class: "messages-link") do + section(t("admin.menu.messaging_users"), active: messages_menu_active?, class: "messages-link") do link_list( newsletters_link, admin_notifications_link, system_emails_link, emails_download_link, - id: "messaging_users_menu", class: ("is-active" if messages_menu_active?) + id: "messaging_users_menu" ) end end @@ -249,7 +251,8 @@ class Admin::MenuComponent < ApplicationComponent end def site_customization_links - section(t("admin.menu.title_site_customization"), class: "site-customization-link") do + section(t("admin.menu.title_site_customization"), + active: customization?, class: "site-customization-link") do link_list( homepage_link, pages_link, @@ -257,9 +260,7 @@ class Admin::MenuComponent < ApplicationComponent information_texts_link, documents_link, images_link, - content_blocks_link, - class: ("is-active" if customization? && - controller.class.module_parent != Admin::Poll::Questions::Answers) + content_blocks_link ) end end @@ -305,7 +306,8 @@ class Admin::MenuComponent < ApplicationComponent end def moderated_content_links - section(t("admin.menu.title_moderated_content"), class: "moderated-content-link") do + section(t("admin.menu.title_moderated_content"), + active: moderated_content?, class: "moderated-content-link") do link_list( (hidden_proposals_link if feature?(:proposals)), (hidden_debates_link if feature?(:debates)), @@ -313,8 +315,7 @@ class Admin::MenuComponent < ApplicationComponent hidden_comments_link, hidden_proposal_notifications_link, hidden_users_link, - activity_link, - class: ("is-active" if moderated_content?) + activity_link ) end end @@ -376,7 +377,7 @@ class Admin::MenuComponent < ApplicationComponent end def profiles_links - section(t("admin.menu.title_profiles"), class: "profiles-link") do + section(t("admin.menu.title_profiles"), active: profiles?, class: "profiles-link") do link_list( administrators_link, organizations_link, @@ -385,8 +386,7 @@ class Admin::MenuComponent < ApplicationComponent valuators_link, managers_link, (sdg_managers_link if feature?(:sdg)), - users_link, - class: ("is-active" if profiles?) + users_link ) end end @@ -457,14 +457,13 @@ class Admin::MenuComponent < ApplicationComponent end def settings_links - section(t("admin.menu.title_settings"), class: "settings-link") do + section(t("admin.menu.title_settings"), active: settings?, class: "settings-link") do link_list( settings_link, tenants_link, tags_link, geozones_link, - local_census_records_link, - class: ("is-active" if settings?) + local_census_records_link ) end end @@ -528,11 +527,10 @@ class Admin::MenuComponent < ApplicationComponent end def dashboard_links - section(t("admin.menu.dashboard"), class: "dashboard-link") do + section(t("admin.menu.dashboard"), active: dashboard?, class: "dashboard-link") do link_list( dashboard_actions_link, - administrator_tasks_link, - class: ("is-active" if dashboard?) + administrator_tasks_link ) end end @@ -574,7 +572,7 @@ class Admin::MenuComponent < ApplicationComponent section_opener(title, **) + content.call end - def section_opener(title, **options) - link_to(title, "#", options) + def section_opener(title, active:, **options) + button_tag(title, { type: "button", disabled: "disabled", "aria-expanded": active }.merge(options)) end end diff --git a/app/components/management/menu_component.html.erb b/app/components/management/menu_component.html.erb index 5c125bf08..0edc468b4 100644 --- a/app/components/management/menu_component.html.erb +++ b/app/components/management/menu_component.html.erb @@ -1 +1 @@ -<%= link_list(*links, id: "admin_menu", data: { "accordion-menu": true }) %> +<%= link_list(*links, id: "admin_menu") %> diff --git a/app/components/management/menu_component.rb b/app/components/management/menu_component.rb index 1fdb87897..be11c0fd4 100644 --- a/app/components/management/menu_component.rb +++ b/app/components/management/menu_component.rb @@ -13,7 +13,7 @@ class Management::MenuComponent < ApplicationComponent private def user_links - section(t("management.menu.users"), class: "users-link") do + section(t("management.menu.users"), active: true, class: "users-link") do link_list( select_user_link, (reset_password_email_link if managed_user.email), @@ -21,8 +21,7 @@ class Management::MenuComponent < ApplicationComponent create_proposal_link, support_proposals_link, (create_budget_investment_link if Setting["process.budgets"]), - (support_budget_investments_link if Setting["process.budgets"]), - class: "is-active" + (support_budget_investments_link if Setting["process.budgets"]) ) end end @@ -157,7 +156,7 @@ class Management::MenuComponent < ApplicationComponent section_opener(title, **) + content.call end - def section_opener(title, **options) - link_to(title, "#", options) + def section_opener(title, active:, **options) + button_tag(title, { type: "button", disabled: "disabled", "aria-expanded": active }.merge(options)) end end diff --git a/spec/components/admin/menu_component_spec.rb b/spec/components/admin/menu_component_spec.rb new file mode 100644 index 000000000..0a7ccf82f --- /dev/null +++ b/spec/components/admin/menu_component_spec.rb @@ -0,0 +1,22 @@ +require "rails_helper" + +describe Admin::MenuComponent, controller: Admin::NewslettersController do + it "disables all buttons when JavaScript isn't available" do + render_inline Admin::MenuComponent.new + + expect(page).to have_button disabled: true + expect(page).not_to have_button disabled: false + end + + it "expands the current section" do + render_inline Admin::MenuComponent.new + + expect(page).to have_css "button[aria-expanded='true']", exact_text: "Messages to users" + end + + it "does not expand other sections" do + render_inline Admin::MenuComponent.new + + expect(page).to have_css "button[aria-expanded='false']", exact_text: "Settings" + end +end diff --git a/spec/system/admin/banners_spec.rb b/spec/system/admin/banners_spec.rb index 0ac2a009e..c604f6d6e 100644 --- a/spec/system/admin/banners_spec.rb +++ b/spec/system/admin/banners_spec.rb @@ -64,7 +64,7 @@ describe "Admin banners magement", :admin do visit admin_root_path within("#side_menu") do - click_link "Site content" + click_button "Site content" click_link "Manage banners" end @@ -133,7 +133,7 @@ describe "Admin banners magement", :admin do visit admin_root_path within("#side_menu") do - click_link "Site content" + click_button "Site content" click_link "Manage banners" end diff --git a/spec/system/admin/geozones_spec.rb b/spec/system/admin/geozones_spec.rb index 95c862baf..384cccc73 100644 --- a/spec/system/admin/geozones_spec.rb +++ b/spec/system/admin/geozones_spec.rb @@ -15,7 +15,7 @@ describe "Admin geozones", :admin do visit admin_root_path within("#side_menu") do - click_link "Settings" + click_button "Settings" click_link "Manage geozones" end diff --git a/spec/system/admin/poll/booths_spec.rb b/spec/system/admin/poll/booths_spec.rb index 61b440bc0..392f01b62 100644 --- a/spec/system/admin/poll/booths_spec.rb +++ b/spec/system/admin/poll/booths_spec.rb @@ -5,7 +5,7 @@ describe "Admin booths", :admin do visit admin_root_path within("#side_menu") do - click_link "Voting booths" + click_button "Voting booths" click_link "Booths location" end @@ -18,7 +18,7 @@ describe "Admin booths", :admin do visit admin_root_path within("#side_menu") do - click_link "Voting booths" + click_button "Voting booths" click_link "Booths location" end @@ -38,7 +38,7 @@ describe "Admin booths", :admin do visit admin_root_path within("#side_menu") do - click_link "Voting booths" + click_button "Voting booths" click_link "Manage shifts" end diff --git a/spec/system/admin/site_customization/content_blocks_spec.rb b/spec/system/admin/site_customization/content_blocks_spec.rb index 012fc28ee..6f77b1c31 100644 --- a/spec/system/admin/site_customization/content_blocks_spec.rb +++ b/spec/system/admin/site_customization/content_blocks_spec.rb @@ -23,7 +23,7 @@ describe "Admin custom content blocks", :admin do visit admin_root_path within("#side_menu") do - click_link "Site content" + click_button "Site content" click_link "Custom content blocks" end @@ -48,7 +48,7 @@ describe "Admin custom content blocks", :admin do visit admin_root_path within("#side_menu") do - click_link "Site content" + click_button "Site content" click_link "Custom content blocks" end @@ -75,7 +75,7 @@ describe "Admin custom content blocks", :admin do visit admin_root_path within("#side_menu") do - click_link "Site content" + click_button "Site content" click_link "Custom content blocks" end diff --git a/spec/system/admin/site_customization/documents_spec.rb b/spec/system/admin/site_customization/documents_spec.rb index 76fff934b..3f4ec063c 100644 --- a/spec/system/admin/site_customization/documents_spec.rb +++ b/spec/system/admin/site_customization/documents_spec.rb @@ -5,7 +5,7 @@ describe "Documents", :admin do visit admin_root_path within("#side_menu") do - click_link "Site content" + click_button "Site content" click_link "Custom documents" end diff --git a/spec/system/admin/site_customization/images_spec.rb b/spec/system/admin/site_customization/images_spec.rb index 11cf9fc0e..801e0576a 100644 --- a/spec/system/admin/site_customization/images_spec.rb +++ b/spec/system/admin/site_customization/images_spec.rb @@ -5,7 +5,7 @@ describe "Admin custom images", :admin do visit admin_root_path within("#side_menu") do - click_link "Site content" + click_button "Site content" click_link "Custom images" end diff --git a/spec/system/admin/site_customization/pages_spec.rb b/spec/system/admin/site_customization/pages_spec.rb index 6192d5632..19a084ce6 100644 --- a/spec/system/admin/site_customization/pages_spec.rb +++ b/spec/system/admin/site_customization/pages_spec.rb @@ -33,7 +33,7 @@ describe "Admin custom pages", :admin do visit admin_root_path within("#side_menu") do - click_link "Site content" + click_button "Site content" click_link "Custom pages" end @@ -63,7 +63,7 @@ describe "Admin custom pages", :admin do visit admin_root_path within("#side_menu") do - click_link "Site content" + click_button "Site content" click_link "Custom pages" end diff --git a/spec/system/admin/tenants_spec.rb b/spec/system/admin/tenants_spec.rb index acabe6464..0616eaa58 100644 --- a/spec/system/admin/tenants_spec.rb +++ b/spec/system/admin/tenants_spec.rb @@ -8,7 +8,7 @@ describe "Tenants", :admin, :seed_tenants do visit admin_root_path within("#side_menu") do - click_link "Settings" + click_button "Settings" click_link "Multitenancy" end diff --git a/spec/system/admin_spec.rb b/spec/system/admin_spec.rb index a0f31c04d..d09d9bdeb 100644 --- a/spec/system/admin_spec.rb +++ b/spec/system/admin_spec.rb @@ -80,7 +80,7 @@ describe "Admin" do within("#admin_menu") do expect(page).to have_link "Participatory budgets" - click_link "Site content" + click_button "Site content" expect(page).to have_link "Participatory budgets" end