[DURACOM-195] Fix menu bar accessibility issues WIP

DSpace · Dec 1, 2023 · 741be06 · 741be06
1 parent 4e550a3
commit 741be06
Show file tree

Hide file tree

Showing 12 changed files with 105 additions and 81 deletions.
diff --git a/...-sidebar/expandable-admin-sidebar-section/expandable-admin-sidebar-section.component.html b/...-sidebar/expandable-admin-sidebar-section/expandable-admin-sidebar-section.component.html
@@ -34,7 +34,7 @@
     <div class="sidebar-fixed-element-wrapper"></div>
     <div class="sidebar-collapsible-element-outer-wrapper">
       <div class="sidebar-collapsible-element-inner-wrapper">
-        <ul class="sidebar-sub-level-item-list" role="group" [id]="adminMenuSectionId(section.id)" [attr.aria-label]="('menu.section.' + section.id) | translate">
+        <ul class="sidebar-sub-level-item-list" role="menu" [id]="adminMenuSectionId(section.id)" [attr.aria-label]="('menu.section.' + section.id) | translate">
           <li class="sidebar-item" role="presentation" *ngFor="let subSection of (subSections$ | async)">
             {{ (sectionMap$ | async).get(subSection.id).component | json}}
             <ng-container

diff --git a/src/app/navbar/expandable-navbar-section/expandable-navbar-section.component.ts b/src/app/navbar/expandable-navbar-section/expandable-navbar-section.component.ts
@@ -107,22 +107,6 @@ export class ExpandableNavbarSectionComponent extends NavbarSectionComponent imp
     });
   }
 
-  /**
-   * Overrides the super function that toggles this section (triggered on click)
-   * Has an extra check to make sure the section can only be toggled on mobile devices
-   * @param {Event} event The user event that triggered this function
-   */
-  toggleSection(event): void {
-    event.preventDefault();
-    this.windowService.isXsOrSm().pipe(
-      first()
-    ).subscribe((isMobile) => {
-      if (isMobile) {
-        super.toggleSection(event);
-      }
-    });
-  }
-
   expandableNavbarSectionId(sectionId: string) {
     return `expandable-navbar-section-${sectionId}-dropdown`;
   }

diff --git a/src/app/navbar/navbar.component.html b/src/app/navbar/navbar.component.html
@@ -1,18 +1,21 @@
-<nav [ngClass]="{'open': !(menuCollapsed | async)}" [@slideMobileNav]="!(windowService.isXsOrSm() | async) ? 'default' : ((menuCollapsed | async) ? 'collapsed' : 'expanded')"
-    class="navbar navbar-light navbar-expand-md px-md-0 navbar-container" role="navigation" [attr.aria-label]="'nav.main.description' | translate" id="main-navbar">
-    <!-- TODO remove navbar-container class when https://github.com/twbs/bootstrap/issues/24726 is fixed -->
-    <div class="navbar-inner-container w-100" [class.container]="!(isMobile$ | async)">
-        <div class="reset-padding-md w-100">
-            <div id="collapsingNav">
-                <ul class="navbar-nav align-items-md-center mr-auto shadow-none">
-                    <li *ngIf="(isMobile$ | async) && (isAuthenticated$ | async)">
-                        <ds-themed-user-menu [inExpandableNavbar]="true"></ds-themed-user-menu>
-                    </li>
-                    <li *ngFor="let section of (sections | async)">
-                        <ng-container *ngComponentOutlet="(sectionMap$ | async).get(section.id)?.component; injector: (sectionMap$ | async).get(section.id)?.injector;"></ng-container>
-                    </li>
-                </ul>
-            </div>
-        </div>
+<nav [ngClass]="{'open': !(menuCollapsed | async)}"
+     [@slideMobileNav]="!(windowService.isXsOrSm() | async) ? 'default' : ((menuCollapsed | async) ? 'collapsed' : 'expanded')"
+     class="navbar navbar-light navbar-expand-md px-md-0 pt-md-0 pt-3 navbar-container" role="navigation"
+     [attr.aria-label]="'nav.main.description' | translate" id="main-navbar">
+  <!-- TODO remove navbar-container class when https://github.com/twbs/bootstrap/issues/24726 is fixed -->
+  <div class="navbar-inner-container w-100" [class.container]="!(isMobile$ | async)">
+    <div class="reset-padding-md w-100">
+      <div id="collapsingNav">
+        <ng-container *ngIf="(isMobile$ | async) && (isAuthenticated$ | async)">
+          <ds-themed-user-menu [inExpandableNavbar]="true"></ds-themed-user-menu>
+        </ng-container>
+        <ul class="navbar-nav align-items-md-center mr-auto shadow-none">
+          <ng-container *ngFor="let section of (sections | async)">
+            <ng-container
+              *ngComponentOutlet="(sectionMap$ | async).get(section.id)?.component; injector: (sectionMap$ | async).get(section.id)?.injector;"></ng-container>
+          </ng-container>
+        </ul>
+      </div>
     </div>
+  </div>
 </nav>
diff --git a/src/app/navbar/navbar.component.scss b/src/app/navbar/navbar.component.scss
@@ -41,11 +41,15 @@
   }
 
   #main-navbar ::ng-deep {
-    a.ds-menu-item {
+    .ds-menu-item, .ds-navbar-link {
+      white-space: nowrap;
+      text-decoration: none;
+    }
+
+    .ds-menu-item {
       display: block;
       color: var(--ds-navbar-link-color);
-      padding: 0.5rem 1rem;
-      text-decoration: none;
+      padding: 0.5rem 0;
 
       &:hover, &:focus {
         color: var(--ds-navbar-link-color-hover);

diff --git a/src/app/root/root.component.html b/src/app/root/root.component.html
@@ -1,4 +1,4 @@
-<button (click)="skipToMainContent()" class="btn btn-primary" id="skip-to-main-content">
+<button (click)="skipToMainContent()" class="sr-only" id="skip-to-main-content">
   {{ 'root.skip-to-content' | translate }}
 </button>
 

diff --git a/src/app/shared/auth-nav-menu/auth-nav-menu.component.html b/src/app/shared/auth-nav-menu/auth-nav-menu.component.html
@@ -1,37 +1,39 @@
-<ul class="navbar-nav" [ngClass]="{'mr-auto': (isXsOrSm$ | async)}">
-  <li *ngIf="!(isAuthenticated | async) && !(isXsOrSm$ | async) && (showAuth | async)" class="nav-item"
+<ul class="navbar-nav mr-auto" *ngIf="!(isMobile$ | async); else mobileButtons">
+  <li *ngIf="!(isAuthenticated | async) && !(isMobile$ | async) && (showAuth | async)" class="nav-item"
       (click)="$event.stopPropagation();">
     <div ngbDropdown #loginDrop display="dynamic" placement="bottom-right" class="d-inline-block" @fadeInOut>
       <a href="javascript:void(0);" class="dropdownLogin px-0.5" [attr.aria-label]="'nav.login' |translate"
          (click)="$event.preventDefault()" [attr.data-test]="'login-menu' | dsBrowserOnly"
          ngbDropdownToggle>{{ 'nav.login' | translate }}</a>
-      <div class="loginDropdownMenu" [ngClass]="{'pl-3 pr-3': (loading | async)}" ngbDropdownMenu
+      <div id="loginDropdownMenu" [ngClass]="{'pl-3 pr-3': (loading | async)}" ngbDropdownMenu
            [attr.aria-label]="'nav.login' | translate">
         <ds-themed-log-in
           [isStandalonePage]="false"></ds-themed-log-in>
       </div>
     </div>
   </li>
-  <li *ngIf="!(isAuthenticated | async) && (isXsOrSm$ | async)" class="nav-item">
-    <a routerLink="/login" routerLinkActive="active" class="loginLink px-0.5">
-      {{ 'nav.login' | translate }}<span class="sr-only">(current)</span>
-    </a>
-  </li>
-  <li *ngIf="(isAuthenticated | async) && !(isXsOrSm$ | async) && (showAuth | async)" class="nav-item">
+  <li *ngIf="(isAuthenticated | async) && !(isMobile$ | async) && (showAuth | async)" class="nav-item">
     <div ngbDropdown display="dynamic" placement="bottom-right" class="d-inline-block" @fadeInOut>
       <a href="javascript:void(0);" role="button" [attr.aria-label]="'nav.user-profile-menu-and-logout' |translate" (click)="$event.preventDefault()" [title]="'nav.user-profile-menu-and-logout' | translate" class="dropdownLogout px-1" [attr.data-test]="'user-menu' | dsBrowserOnly" ngbDropdownToggle>
         <i class="fas fa-user-circle fa-lg fa-fw"></i></a>
-      <div class="logoutDropdownMenu" ngbDropdownMenu [attr.aria-label]="'nav.user-profile-menu-and-logout' |translate">
+      <ul id="logoutDropdownMenu" class="p-3" ngbDropdownMenu [attr.aria-label]="'nav.user-profile-menu-and-logout' |translate">
         <ds-themed-user-menu></ds-themed-user-menu>
-      </div>
+      </ul>
     </div>
   </li>
-  <li *ngIf="(isAuthenticated | async) && (isXsOrSm$ | async)" class="nav-item">
+</ul>
+
+
+<ng-template #mobileButtons>
+  <div *ngIf="!(isAuthenticated | async)">
+    <a routerLink="/login" routerLinkActive="active" class="loginLink px-0.5" role="button">
+      {{ 'nav.login' | translate }}<span class="sr-only">(current)</span>
+    </a>
+  </div>
+  <div *ngIf="(isAuthenticated | async)">
     <a role="button" [attr.aria-label]="'nav.logout' |translate" [title]="'nav.logout' | translate" routerLink="/logout" routerLinkActive="active" class="logoutLink px-1">
       <i class="fas fa-sign-out-alt fa-lg fa-fw"></i>
       <span class="sr-only">(current)</span>
     </a>
-  </li>
-</ul>
-
-
+  </div>
+</ng-template>
diff --git a/src/app/shared/auth-nav-menu/auth-nav-menu.component.scss b/src/app/shared/auth-nav-menu/auth-nav-menu.component.scss
@@ -1,9 +1,9 @@
-.loginDropdownMenu, .logoutDropdownMenu {
+#loginDropdownMenu, #logoutDropdownMenu {
   min-width: 330px;
   z-index: 1002;
 }
 
-.loginDropdownMenu {
+#loginDropdownMenu {
   min-height: 260px;
 }
 

diff --git a/src/app/shared/auth-nav-menu/auth-nav-menu.component.ts b/src/app/shared/auth-nav-menu/auth-nav-menu.component.ts
@@ -32,7 +32,7 @@ export class AuthNavMenuComponent implements OnInit {
    */
   public loading: Observable<boolean>;
 
-  public isXsOrSm$: Observable<boolean>;
+  public isMobile$: Observable<boolean>;
 
   public showAuth = observableOf(false);
 
@@ -44,7 +44,7 @@ export class AuthNavMenuComponent implements OnInit {
               private windowService: HostWindowService,
               private authService: AuthService
   ) {
-    this.isXsOrSm$ = this.windowService.isXsOrSm();
+    this.isMobile$ = this.windowService.isMobile();
   }
 
   ngOnInit(): void {

diff --git a/src/app/shared/auth-nav-menu/user-menu/user-menu.component.html b/src/app/shared/auth-nav-menu/user-menu/user-menu.component.html
@@ -1,15 +1,28 @@
 <ds-themed-loading *ngIf="(loading$ | async)"></ds-themed-loading>
-<div *ngIf="!(loading$ | async)">
-  <span class="dropdown-item-text" [class.pl-0]="inExpandableNavbar">
+<ul *ngIf="!(loading$ | async)" class="user-menu" role="menu">
+  <li class="menu-item-wrapper" role="presentation">
     {{dsoNameService.getName(user$ | async)}}<br>
     <span class="text-muted">{{(user$ | async)?.email}}</span>
-  </span>
-  <a [ngClass]="inExpandableNavbar ? 'nav-item nav-link' : 'dropdown-item'" [routerLink]="[profileRoute]" routerLinkActive="active">{{'nav.profile' | translate}}</a>
-  <a [ngClass]="inExpandableNavbar ? 'nav-item nav-link' : 'dropdown-item'" [routerLink]="[mydspaceRoute]" routerLinkActive="active">{{'nav.mydspace' | translate}}</a>
-  <a [ngClass]="inExpandableNavbar ? 'nav-item nav-link' : 'dropdown-item'" [routerLink]="[subscriptionsRoute]" routerLinkActive="active">{{'nav.subscriptions' | translate}}</a>
-
-  <div class="dropdown-divider"></div>
-  <ds-log-out *ngIf="!inExpandableNavbar" data-test="log-out-component"></ds-log-out>
-</div>
+  </li>
+  <li class="menu-item-wrapper" role="presentation">
+    <a class="ds-menu-item" role="menuitem"
+       [routerLink]="[profileRoute]"
+       routerLinkActive="active">{{'nav.profile' | translate}}</a>
+  </li>
+  <li class="menu-item-wrapper" role="presentation">
+    <a class="ds-menu-item" role="menuitem"
+       [routerLink]="[mydspaceRoute]"
+       routerLinkActive="active">{{'nav.mydspace' | translate}}</a>
+  </li>
+  <li class="menu-item-wrapper" role="presentation">
+    <a class="ds-menu-item" role="menuitem"
+       [routerLink]="[subscriptionsRoute]"
+       routerLinkActive="active">{{'nav.subscriptions' | translate}}</a>
+  </li>
+  <li class="dropdown-divider" aria-hidden="true"></li>
+  <li class="menu-item-wrapper" role="presentation">
+    <ds-log-out *ngIf="!inExpandableNavbar" data-test="log-out-component"></ds-log-out>
+  </li>
+</ul>
 
 
diff --git a/src/app/shared/auth-nav-menu/user-menu/user-menu.component.scss b/src/app/shared/auth-nav-menu/user-menu/user-menu.component.scss
@@ -0,0 +1,12 @@
+:host {
+  ul.user-menu  {
+    list-style: none;
+    margin: 0;
+    padding: 0;
+  }
+
+  .ds-menu-item {
+    display: inline-block;
+    padding: 0.25rem 0;
+  }
+}
diff --git a/src/themes/dspace/app/header/header.component.html b/src/themes/dspace/app/header/header.component.html
@@ -1,23 +1,29 @@
 <header id="main-site-header">
   <div class="container h-100 d-flex flex-row flex-nowrap justify-content-between gapx-3">
     <!-- Logo and navbar wrapper -->
-    <div id="header-left" class="h-100 flex-fill d-flex flex-row flex-nowrap justify-content-start align-items-center gapx-3">
+    <div id="header-left"
+         [attr.role]="(isMobile$ | async) ? 'navigation' : 'presentation'"
+         [attr.aria-label]="(isMobile$ | async) ? ('nav.main.description' | translate) : null"
+         class="h-100 flex-fill d-flex flex-row flex-nowrap justify-content-start align-items-center gapx-3">
       <a class="d-block" routerLink="/home" [attr.aria-label]="'home.title' | translate">
-        <img id="header-logo" src="assets/images/dspace-logo.svg" [attr.alt]="'menu.header.image.logo' | translate" role="presentation"/>
+        <img id="header-logo" src="assets/images/dspace-logo.svg" [attr.alt]="'menu.header.image.logo' | translate"/>
       </a>
-      <nav class="navbar navbar-expand px-0 align-self-stretch" id="desktop-navbar" [attr.aria-label]="'nav.main.description' | translate">
-        <ds-themed-navbar *ngIf="!(isMobile$ | async)"></ds-themed-navbar>
+      <nav *ngIf="!(isMobile$ | async)" class="navbar navbar-expand px-0 align-self-stretch" id="desktop-navbar" [attr.aria-label]="'nav.main.description' | translate">
+        <ds-themed-navbar></ds-themed-navbar>
       </nav>
     </div>
     <!-- Search bar and other menus -->
-    <div role="menubar" id="header-right" class="h-100 d-flex flex-row flex-nowrap justify-content-end align-items-center gapx-1">
+    <div id="header-right" class="h-100 d-flex flex-row flex-nowrap justify-content-end align-items-center gapx-1">
       <!-- TODO: add ARIA attribute to subcomponents -->
-      <!-- dropdown menus should have aria-haspopup=menu -->
-      <ds-themed-search-navbar></ds-themed-search-navbar>
-      <ds-themed-lang-switch></ds-themed-lang-switch>
-      <ds-context-help-toggle></ds-context-help-toggle>
-      <ds-themed-auth-nav-menu></ds-themed-auth-nav-menu>
-      <ds-impersonate-navbar></ds-impersonate-navbar>
+      <div role="menubar" class="h-100 d-flex flex-row flex-nowrap align-items-center gapx-1">
+        <ds-themed-search-navbar></ds-themed-search-navbar>
+        <ds-themed-lang-switch></ds-themed-lang-switch>
+        <ds-context-help-toggle></ds-context-help-toggle>
+        <ds-impersonate-navbar></ds-impersonate-navbar>
+        <ds-themed-auth-nav-menu *ngIf="isMobile$ | async"></ds-themed-auth-nav-menu>
+      </div>
+      <ds-themed-auth-nav-menu *ngIf="!(isMobile$ | async)"></ds-themed-auth-nav-menu>
+      <!-- On mobile the auth-nav menu only contains the logout button, which is better placed inside the menubar -->
 
       <div id="mobile-navbar-toggler" class="d-block d-lg-none ml-3" *ngIf="(isMobile$ | async)">
         <button id="navbar-toggler" class="btn" type="button" (click)="toggleNavbar()"

diff --git a/src/themes/dspace/app/navbar/navbar.component.html b/src/themes/dspace/app/navbar/navbar.component.html
@@ -1,8 +1,8 @@
   <div>
+    <ng-container *ngIf="(isMobile$ | async) && (isAuthenticated$ | async)">
+      <ds-user-menu [inExpandableNavbar]="true"></ds-user-menu>
+    </ng-container>
     <ul class="navbar-nav align-items-md-center gapx-3" role="menu">
-      <li *ngIf="(isMobile$ | async) && (isAuthenticated$ | async)">
-        <ds-user-menu [inExpandableNavbar]="true"></ds-user-menu>
-      </li>
       <ng-container *ngFor="let section of (sections | async)">
         <ng-container *ngComponentOutlet="(sectionMap$ | async).get(section.id)?.component; injector: (sectionMap$ | async).get(section.id)?.injector;"></ng-container>
       </ng-container>