refactor navbar to be menubar (#3300)

Refactor navigation bar to follow the menubar pattern for accessibility reasons. This changes the semantic structure of the navigation bar to be ul, li and a tags. --------- Co-authored-by: Aday Bujeda <[email protected]>
OSC · Jan 24, 2024 · 55bf3f0 · 55bf3f0
1 parent 2e3dcc2
commit 55bf3f0
Show file tree

Hide file tree

Showing 17 changed files with 211 additions and 181 deletions.
diff --git a/apps/dashboard/app/helpers/application_helper.rb b/apps/dashboard/app/helpers/application_helper.rb
@@ -23,11 +23,11 @@ def restart_url
   # @param role [String] app role i.e. "vdi", "shell", etc.
   # @param data [Hash] data parameter for the link_to helper.
   # @return nil if url not set or the HTML string for the bootstrap nav link
-  def nav_link(title, icon, url, new_tab: false, role: nil, data: nil)
+  def nav_link(title, icon, url, new_tab: false, data: nil)
     if url
       icon_uri = URI("fa://#{icon}")
       render partial: 'layouts/nav/link',
-             locals:  { title: title, class: 'dropdown-item', icon_uri: icon_uri, url: url.to_s, new_tab: new_tab, role: role, data: data }
+             locals:  { title: title, class: 'dropdown-item', icon_uri: icon_uri, url: url.to_s, new_tab: new_tab, data: data }
     end
   end
 

diff --git a/apps/dashboard/app/views/layouts/application.html.erb b/apps/dashboard/app/views/layouts/application.html.erb
@@ -28,18 +28,14 @@
 <body>
   <header>
     <nav class="navbar navbar-expand-md shadow-sm navbar-color navbar-<%= @user_configuration.navbar_type %>">
-    <% if @user_configuration.dashboard_header_img_logo %>
-      <a class="navbar-brand navbar-brand-logo" href="<%= root_path %>"><img class="img-fluid" src="<%= @user_configuration.dashboard_header_img_logo %>" alt="<%= @user_configuration.dashboard_title %>"></a>
-    <% else %>
-      <a class="navbar-brand" href="<%= root_path %>"><%= @user_configuration.dashboard_title %></a>
-    <% end %>
+      <ul class="navbar-nav w-100 align-items-center" role="menubar">
+        <%= render partial: 'layouts/nav/logo' %>
 
-      <button class="navbar-toggler" type="button" data-toggle="collapse" data-target="#navbar" aria-controls="navbar" aria-expanded="false" aria-label="Toggle navigation">
-        <span class="navbar-toggler-icon"></span>
-      </button>
+        <button class="navbar-toggler" type="button" data-toggle="collapse" data-target="#navbar" aria-controls="navbar" aria-expanded="false" aria-label="Toggle navigation">
+          <span class="navbar-toggler-icon"></span>
+        </button>
 
-      <div class="collapse navbar-collapse" id="navbar">
-        <ul class="navbar-nav mr-auto">
+        <div class="collapse navbar-collapse" id="navbar">
           <% if @nav_bar.empty? %>
             <%= render partial: 'layouts/nav/featured_apps' %>
             <%= render partial: 'layouts/nav/group', collection: @nav_groups %>
@@ -48,9 +44,10 @@
           <% else %>
             <%= render partial: 'layouts/nav/custom_navigation', locals: { navigation: @nav_bar }%>
           <% end %>
-        </ul>
 
-        <ul class="navbar-nav">
+          <!-- add some space between nav_bar and help_menu -->
+          <div class="ml-auto"></div>
+
           <% if @help_bar.empty? %>
             <%= render partial: 'layouts/nav/develop_dropdown' %>
             <%= render partial: 'layouts/nav/help_dropdown' %>
@@ -59,8 +56,8 @@
           <% else %>
             <%= render partial: 'layouts/nav/custom_navigation', locals: { navigation: @help_bar, menu_alignment: 'dropdown-menu-right' }%>
           <% end %>
-        </ul>
-      </div>
+        </div>
+      </ul>
     </nav>
   </header>
 

diff --git a/apps/dashboard/app/views/layouts/nav/_all_apps.html.erb b/apps/dashboard/app/views/layouts/nav/_all_apps.html.erb
@@ -1,5 +1,17 @@
-<li class="nav-item" title="<%= t('dashboard.nav_all_apps') %>">
-  <%= tag.a class: 'nav-link', href: apps_index_path, aria: ({ current: ('page' if (current_page?(controller: '/apps', action: 'index' ))) }) do %>
-    <i class="fas fa-th" aria-hidden="true"></i><span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_all_apps') %></span>
-  <% end %>
+<%-
+  aria =  if current_page?(controller: '/apps', action: 'index')
+            "aria-current=page"
+          else
+            ""
+          end
+-%>
+
+<li class="nav-item" role="none">
+  <a title="<%= t('dashboard.nav_all_apps') %>" class="nav-link"
+     role="menuitem" href="<%= apps_index_path %>"
+     <%= aria %>
+    >
+    <i class="fas fa-th" aria-hidden="true"></i>
+    <span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_all_apps') %></span>
+  </a>
 </li>
diff --git a/apps/dashboard/app/views/layouts/nav/_develop_dropdown.html.erb b/apps/dashboard/app/views/layouts/nav/_develop_dropdown.html.erb
@@ -1,6 +1,6 @@
 <% if Configuration.app_development_enabled? %>
-  <li class="nav-item dropdown" title="<%= t('dashboard.nav_develop_title') %>">
-    <a href="#" class="nav-link dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
+  <li class="nav-item dropdown" role="none">
+    <a role='menuitem' title="<%= t('dashboard.nav_develop_title') %>" href="#" class="nav-link dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
       <i class="fas fa-code" aria-hidden="true"></i><span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_develop_title') %></span><span class="caret"></span>
     </a>
 

diff --git a/apps/dashboard/app/views/layouts/nav/_featured_apps.html.erb b/apps/dashboard/app/views/layouts/nav/_featured_apps.html.erb
@@ -1,20 +1,21 @@
 <% if @featured_group.present? %>
   <% group = @featured_group %>
-  <li class="nav-item dropdown" title="<%= group.title %>" >
-    <a href="#" class="nav-link dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false"><%= group.title %><span class="caret"></span></a>
+  <li class="nav-item dropdown" role="none">
+    <a role="menuitem" href="#" class="nav-link dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false" role="menuitem" title="<%= group.title %>"><%= group.title %><span class="caret"></span></a>
 
-    <ul class="dropdown-menu">
+    <ul class="dropdown-menu" role="menu" title="<%= group.title %>">
       <% OodAppGroup.groups_for(apps: group.apps, group_by: :subcategory, nav_limit: group.nav_limit).each_with_index do |g, index| %>
         <%= content_tag "li", "", class: "dropdown-divider", role: "separator" if index > 0 %>
         <%= content_tag "li", g.title_with_nav_limit_caption, class: "dropdown-header" unless g.title_with_nav_limit_caption.empty? %>
         <% g.apps.take(g.nav_limit).map { |app| app.links.first }.compact.each do |link| %>
-            <li>
+            <li role="none">
               <%=
                 link_to(
                   link.url.to_s,
                   title: link.title,
                   class: "dropdown-item",
                   target: link.new_tab? ? "_blank" : nil,
+                  role: 'menuitem',
                   data: link.data
                 ) do
               %>
@@ -28,8 +29,8 @@
       <% end %>
 
       <%= content_tag "li", "", class: "dropdown-divider", role: "separator" %>
-      <li title="<%= t('dashboard.nav_all_apps') %>">
-        <a href="<%= apps_index_path %>" class="dropdown-item">
+      <li title="<%= t('dashboard.nav_all_apps') %>" role="none">
+        <a href="<%= apps_index_path %>" class="dropdown-item" role="menuitem">
           <i class="fas fa-th" aria-hidden="true"></i> <%= t('dashboard.nav_all_apps') %>
         </a>
       </li>

diff --git a/apps/dashboard/app/views/layouts/nav/_group.html.erb b/apps/dashboard/app/views/layouts/nav/_group.html.erb
@@ -1,9 +1,9 @@
-<li class="nav-item dropdown" title="<%= group.title %>">
-  <a href="#" class="nav-link dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
+<li class="nav-item dropdown" role="none">
+  <a href="#" class="nav-link dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false" role="menuitem" title="<%= group.title %>">
     <%= icon_tag(group.icon_uri, classes: 'menu-icon') if group.icon_uri %><span> <%= group.title %></span><span class="caret"></span>
   </a>
 
-  <ul class="dropdown-menu <%= local_assigns.fetch(:menu_alignment, '') %>">
+  <ul class="dropdown-menu <%= local_assigns.fetch(:menu_alignment, '') %>" title="<%= group.title %>" role="menu">
     <%= render partial: 'layouts/nav/group_items', locals: local_assigns %>
   </ul>
 </li>
diff --git a/apps/dashboard/app/views/layouts/nav/_group_items.html.erb b/apps/dashboard/app/views/layouts/nav/_group_items.html.erb
@@ -3,14 +3,15 @@
   <%= content_tag(:li, g.title, class: ["dropdown-header"]) unless g.title.empty? %>
   <% g.apps.each do |app| %>
     <% app.links.each do |link| %>
-      <li>
+      <li role="none">
         <%=
           link_to(
             link.url.to_s,
             title: link.title,
             class: "dropdown-item",
             target: link.new_tab? ? "_blank" : nil,
             aria: ({ current: ('page' if (current_page?(link.url))) }),
+            role: 'menuitem',
             data: link.data
           ) do
         %>

diff --git a/apps/dashboard/app/views/layouts/nav/_help_dropdown.html.erb b/apps/dashboard/app/views/layouts/nav/_help_dropdown.html.erb
@@ -1,5 +1,7 @@
-<li class="nav-item dropdown" title="<%= t('dashboard.nav_help_title') %>" >
-  <a href="#" class="nav-link dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
+<li class="nav-item dropdown" role="none">
+  <a href="#" title="<%= t('dashboard.nav_help_title') %>" class="nav-link dropdown-toggle"
+    data-toggle="dropdown" aria-haspopup="true" aria-expanded="false"
+    role="menuitem">
     <i class="fas fa-question-circle" aria-hidden="true"></i><span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_help_title') %></span><span class="caret"></span>
   </a>
 

diff --git a/apps/dashboard/app/views/layouts/nav/_link.html.erb b/apps/dashboard/app/views/layouts/nav/_link.html.erb
@@ -13,18 +13,21 @@ data - optional, data parameter for the link_to helper
   url = local_assigns.fetch(:url)
   icon_uri = local_assigns.fetch(:icon_uri, URI("fas://cog"))
 %>
+<li role="none">
 <%=
   link_to(
     url,
     title: title,
-    class: "#{local_assigns.fetch(:class, 'nav-link')} #{local_assigns.fetch(:role, nil)}",
+    class: "#{local_assigns.fetch(:class, 'nav-link')}",
     target: local_assigns.fetch(:new_tab, false) ? "_blank" : nil,
     aria: ({ current: ('page' if (current_page?(url))) }),
+    role: 'menuitem',
     data: local_assigns.fetch(:data, nil)
   ) do
 %>
   <%= icon_tag(icon_uri) unless icon_uri.to_s.blank? %>
   <%= tag.span do %>
     <%= title %>
   <% end %>
-<% end %>
+<% end %>
+</li>
diff --git a/apps/dashboard/app/views/layouts/nav/_log_out.html.erb b/apps/dashboard/app/views/layouts/nav/_log_out.html.erb
@@ -1,3 +1,6 @@
-<li class="nav-item">
-  <a class="nav-link" href="/logout"><i class="fas fa-sign-out-alt" aria-hidden="true"></i><span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_logout') %></span></a>
+<li class="nav-item" role="none">
+  <a class="nav-link" href="/logout" title="<%= t('dashboard.nav_logout') %>" role="menuitem">
+    <i class="fas fa-sign-out-alt" aria-hidden="true"></i>
+    <span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_logout') %></span>
+  </a>
 </li>
diff --git a/apps/dashboard/app/views/layouts/nav/_logo.html.erb b/apps/dashboard/app/views/layouts/nav/_logo.html.erb
@@ -0,0 +1,14 @@
+
+<%- 
+  aria = current_page?(root_path) ? "aria-current=page" : ''
+-%>
+
+<li role="none" >
+<% if @user_configuration.dashboard_header_img_logo %>
+  <a class="navbar-brand navbar-brand-logo" href="<%= root_path %>" <%= aria %> role='menuitem'>
+    <img class="img-fluid" src="<%= @user_configuration.dashboard_header_img_logo %>" alt="<%= @user_configuration.dashboard_title %>">
+  </a>
+<% else %>
+  <a class="navbar-brand" <%= aria %> role='menuitem' href="<%= root_path %>"><%= @user_configuration.dashboard_title %></a>
+<% end %>
+</li>
diff --git a/apps/dashboard/app/views/layouts/nav/_sessions.html.erb b/apps/dashboard/app/views/layouts/nav/_sessions.html.erb
@@ -1,5 +1,17 @@
-<li class="nav-item" title="<%= t('dashboard.breadcrumbs_my_sessions') %>">
-  <%= tag.a class: 'nav-link', href: batch_connect_sessions_path, aria: ({ current: ('page' if (current_page?(controller: 'batch_connect/sessions', action: 'index'))) }) do %>
-    <i class="fas fa-window-restore" aria-hidden="true"></i><span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_sessions') %></span>
-  <% end %>
+<%-
+  aria =  if current_page?(controller: 'batch_connect/sessions', action: 'index')
+            "aria-current=page"
+          else
+            ""
+          end
+-%>
+
+<li class="nav-item" role="none">
+  <a title="<%= t('dashboard.breadcrumbs_my_sessions') %>" class="nav-link"
+     role="menuitem" href="<%= batch_connect_sessions_path %>"
+     <%= aria %>
+    >
+    <i class="fas fa-window-restore" aria-hidden="true"></i>
+    <span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_sessions') %></span>
+  </a>
 </li>
diff --git a/apps/dashboard/app/views/layouts/nav/_user.html.erb b/apps/dashboard/app/views/layouts/nav/_user.html.erb
@@ -1,5 +1,7 @@
-<li class="nav-item" data-container="body" data-toggle="popover" data-content="<%= t('dashboard.nav_user', username: @user.name) %>" data-placement="bottom">
-  <a class="nav-link disabled">
-    <i class="fas fa-user" aria-hidden="true" title="<%= t('dashboard.nav_user', username: @user.name) %>" aria-hidden="true"></i><span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_user', username: @user.name) %></span>
+<li class="nav-item" data-container="body" data-toggle="popover"
+  data-content="<%= t('dashboard.nav_user', username: @user.name) %>" data-placement="bottom"
+  role="none">
+  <a class="nav-link disabled" role="menuitem" title="<%= t('dashboard.nav_user', username: @user.name) %>" href="#">
+    <i class="fas fa-user" aria-hidden="true" aria-hidden="true"></i><span class="d-sm-none d-md-none d-lg-inline"> <%= t('dashboard.nav_user', username: @user.name) %></span>
   </a>
 </li>
diff --git a/apps/dashboard/test/html_helper.rb b/apps/dashboard/test/html_helper.rb
@@ -2,12 +2,20 @@
 # integration tests.
 class ActiveSupport::TestCase
 
+  def nav_link(title)
+    css_select("nav a.nav-link[title='#{title}']")
+  end
+
   def dropdown_list(title)
-    css_select("li.dropdown[title='#{title}'] ul")
+    css_select("nav a.nav-link.dropdown-toggle[title='#{title}'] + ul")
+  end
+
+  def dropdown_links
+    'nav a.nav-link.dropdown-toggle[title]'
   end
 
   def dropdown_link(order)
-    ".navbar-expand-md > #navbar li.dropdown:nth-of-type(#{order}) a"
+    "nav #navbar li.dropdown:nth-of-type(#{order}) a[title]"
   end
 
   # given a dropdown list, return the list items as an array of strings