Fix sidebar a11y issues

src/app/admin/admin-sidebar/admin-sidebar-section/admin-sidebar-section.component.html

@@ -1,10 +1,23 @@
-<li class="sidebar-section">
-    <a href="javascript:void(0);" class="nav-item nav-link shortcut-icon" attr.aria-labelledby="sidebarName-{{section.id}}" [title]="('menu.section.icon.' + section.id) | translate" [routerLink]="itemModel.link">
+<div class="sidebar-section">
+    <a class="nav-item nav-link d-flex flex-row flex-nowrap"
+       [ngClass]="{ disabled: !hasLink }"
+       [attr.aria-disabled]="!hasLink"
+       [attr.aria-labelledby]="'sidebarName-' + section.id"
+       [title]="('menu.section.icon.' + section.id) | translate"
+       [routerLink]="itemModel.link"
+       (keyup.space)="navigate($event)"
+       (keyup.enter)="navigate($event)"
+       href="javascript:void(0);"
+    >
+      <div class="shortcut-icon">
         <i class="fas fa-{{section.icon}} fa-fw"></i>
+      </div>
+      <div class="sidebar-collapsible">
+        <div class="toggle">
+          <span id="sidebarName-{{section.id}}" class="section-header-text">
+            {{itemModel.text | translate}}
+          </span>
+        </div>
+      </div>
     </a>
-    <div class="sidebar-collapsible">
-        <span id="sidebarName-{{section.id}}" class="section-header-text">
-        <a class="nav-item nav-link"  tabindex="-1" [routerLink]="itemModel.link">{{itemModel.text | translate}}</a>
-        </span>
-    </div>
-</li>
+</div>

src/app/admin/admin-sidebar/admin-sidebar-section/admin-sidebar-section.component.ts

@@ -5,12 +5,15 @@ import { MenuService } from '../../../shared/menu/menu.service';
 import { rendersSectionForMenu } from '../../../shared/menu/menu-section.decorator';
 import { LinkMenuItemModel } from '../../../shared/menu/menu-item/models/link.model';
 import { MenuSection } from '../../../shared/menu/menu.reducer';
+import { isNotEmpty } from '../../../shared/empty.util';
+import { Router } from '@angular/router';
 
 /**
  * Represents a non-expandable section in the admin sidebar
  */
 @Component({
-  selector: 'ds-admin-sidebar-section',
+  /* tslint:disable:component-selector */
+  selector: 'li[ds-admin-sidebar-section]',
   templateUrl: './admin-sidebar-section.component.html',
   styleUrls: ['./admin-sidebar-section.component.scss'],
 
@@ -23,12 +26,26 @@ export class AdminSidebarSectionComponent extends MenuSectionComponent implement
    */
   menuID: MenuID = MenuID.ADMIN;
   itemModel;
-  constructor(@Inject('sectionDataProvider') menuSection: MenuSection, protected menuService: MenuService, protected injector: Injector,) {
+  hasLink: boolean;
+  constructor(
+    @Inject('sectionDataProvider') menuSection: MenuSection,
+    protected menuService: MenuService,
+    protected injector: Injector,
+    protected router: Router,
+  ) {
     super(menuSection, menuService, injector);
     this.itemModel = menuSection.model as LinkMenuItemModel;
   }
 
   ngOnInit(): void {
+    this.hasLink = isNotEmpty(this.itemModel?.link);
     super.ngOnInit();
   }
+
+  navigate(event: any): void {
+    event.preventDefault();
+    if (this.hasLink) {
+      this.router.navigate(this.itemModel.link);
+    }
+  }
 }

src/app/admin/admin-sidebar/admin-sidebar.component.html

@@ -4,24 +4,26 @@
       value: (!(sidebarExpanded | async) ? 'collapsed' : 'expanded'),
       params: {sidebarWidth: (sidebarWidth | async)}
      }" (@slideSidebar.done)="finishSlide($event)" (@slideSidebar.start)="startSlide($event)"
-     *ngIf="menuVisible | async" (mouseenter)="expandPreview($event)"
-     (mouseleave)="collapsePreview($event)"
+     *ngIf="menuVisible | async"
+     (mouseenter)="handleMouseEnter($event)"
+     (mouseleave)="handleMouseLeave($event)"
      role="navigation" [attr.aria-label]="'menu.header.admin.description' |translate">
     <div class="sidebar-top-level-items">
         <ul class="navbar-nav">
-            <li class="admin-menu-header sidebar-section">
-                <a class="shortcut-icon navbar-brand mr-0" href="javascript:void(0);">
-                <span class="logo-wrapper">
+            <li class="admin-menu-header">
+              <div class="sidebar-section">
+                <div href="javascript:void(0);" class="nav-item d-flex flex-row flex-nowrap py-0">
+                  <div class="shortcut-icon navbar-brand logo-wrapper">
                     <img class="admin-logo" src="assets/images/dspace-logo-mini.svg"
                          [alt]="('menu.header.image.logo') | translate">
-                </span>
-                </a>
-                <div class="sidebar-collapsible">
-                    <a class="navbar-brand mr-0" href="javascript:void(0);">
-                        <h4 class="section-header-text mb-0">{{'menu.header.admin' |
-                            translate}}</h4>
-                    </a>
+                  </div>
+                  <div class="sidebar-collapsible navbar-brand">
+                    <div class="mr-0">
+                      <h4 class="section-header-text mb-0">{{ 'menu.header.admin' | translate }}</h4>
+                    </div>
+                  </div>
                 </div>
+              </div>
             </li>
 
             <ng-container *ngFor="let section of (sections | async)">
@@ -32,22 +34,22 @@ <h4 class="section-header-text mb-0">{{'menu.header.admin' |
     </div>
     <div class="navbar-nav">
         <div class="sidebar-section" id="sidebar-collapse-toggle">
-            <a class="nav-item nav-link shortcut-icon"
+            <a class="nav-item nav-link sidebar-section d-flex flex-row flex-nowrap"
                href="javascript:void(0);"
-               (click)="toggle($event)">
+               (click)="toggle($event)"
+               (keyup.space)="toggle($event)"
+            >
+              <div class="shortcut-icon">
                 <i *ngIf="(menuCollapsed | async)" class="fas fa-fw fa-angle-double-right"
                    [title]="'menu.section.icon.pin' | translate"></i>
                 <i *ngIf="!(menuCollapsed | async)" class="fas fa-fw fa-angle-double-left"
                    [title]="'menu.section.icon.unpin' | translate"></i>
+              </div>
+              <div class="sidebar-collapsible">
+                <span *ngIf="menuCollapsed | async" class="section-header-text">{{'menu.section.pin' | translate }}</span>
+                <span *ngIf="!(menuCollapsed | async)" class="section-header-text">{{'menu.section.unpin' | translate }}</span>
+              </div>
             </a>
-            <div class="sidebar-collapsible">
-                <a class="nav-item nav-link sidebar-section"
-                   href="javascript:void(0);"
-                   (click)="toggle($event)">
-                    <span *ngIf="menuCollapsed | async" class="section-header-text">{{'menu.section.pin' | translate }}</span>
-                    <span *ngIf="!(menuCollapsed | async)" class="section-header-text">{{'menu.section.unpin' | translate }}</span>
-                </a>
-            </div>
         </div>
     </div>
 </nav>

src/app/admin/admin-sidebar/admin-sidebar.component.scss

@@ -25,6 +25,11 @@
         .navbar-nav {
             .admin-menu-header {
                 background-color: var(--ds-admin-sidebar-header-bg);
+
+                .sidebar-section {
+                  background-color: inherit;
+                }
+
                 .logo-wrapper {
                     img {
                         height: 20px;
@@ -34,6 +39,10 @@
                     line-height: 1.5;
                 }
 
+                .navbar-brand {
+                  margin-right: 0;
+                }
+
             }
         }
 
@@ -44,26 +53,64 @@
                     display: flex;
                     align-content: stretch;
                     background-color: var(--ds-admin-sidebar-bg);
+                    overflow-x: visible;
+
                     .nav-item {
                         padding-top: var(--bs-spacer);
                         padding-bottom: var(--bs-spacer);
+                        background-color: inherit;
+
+                      &:focus-visible {
+                        // since links fill the whole sidebar, we _inset_ the outline
+                        outline-offset: -4px;
+
+                        // replace padding with margins so it doesn't extend over the :focus-visible outline
+                        //  → can't remove the padding altogether; the icon needs to fill out
+                        //    the collapsed width of the sidebar for the slide animation to look decent.
+                        .shortcut-icon {
+                          padding-left: 0;
+                          padding-right: 0;
+                          margin-left: var(--ds-icon-padding);
+                          margin-right: var(--ds-icon-padding);
+                        }
+                        .logo-wrapper {
+                          margin-right: var(--bs-navbar-padding-x) !important;
+                        }
+                        .navbar-brand {
+                          padding-top: 0;
+                          padding-bottom: 0;
+                          margin-top: var(--bs-navbar-brand-padding-y);
+                          margin-bottom: var(--bs-navbar-brand-padding-y);
+                        }
+                      }
                     }
+
                     .shortcut-icon {
+                        background-color: inherit;
                         padding-left: var(--ds-icon-padding);
                         padding-right: var(--ds-icon-padding);
-                    }
-                    .shortcut-icon, .icon-wrapper {
-                        background-color: inherit;
                         z-index: var(--ds-icon-z-index);
+                        align-self: baseline;
                     }
+
                     .sidebar-collapsible {
+                        padding-left: 0;
+                        padding-right: var(--bs-spacer);
                         width: var(--ds-sidebar-items-width);
                         position: relative;
-                        a {
-                            padding-right: var(--bs-spacer);
-                            width: 100%;
+                        .toggle {
+                          width: 100%;
+                        }
+
+                        ul {
+                          padding-top: var(--bs-spacer);
+
+                          li a {
+                            padding-left: var(--bs-spacer);
+                          }
                         }
                     }
+
                     &.active > .sidebar-collapsible > .nav-link {
                         color: var(--bs-navbar-dark-active-color);
                     }

src/app/admin/admin-sidebar/admin-sidebar.component.spec.ts

@@ -113,25 +113,10 @@ describe('AdminSidebarComponent', () => {
     });
   });
 
-  describe('when the collapse icon is clicked', () => {
-    beforeEach(() => {
-      spyOn(menuService, 'toggleMenu');
-      const sidebarToggler = fixture.debugElement.query(By.css('#sidebar-collapse-toggle')).query(By.css('a.shortcut-icon'));
-      sidebarToggler.triggerEventHandler('click', {
-        preventDefault: () => {/**/
-        }
-      });
-    });
-
-    it('should call toggleMenu on the menuService', () => {
-      expect(menuService.toggleMenu).toHaveBeenCalled();
-    });
-  });
-
   describe('when the collapse link is clicked', () => {
     beforeEach(() => {
       spyOn(menuService, 'toggleMenu');
-      const sidebarToggler = fixture.debugElement.query(By.css('#sidebar-collapse-toggle')).query(By.css('.sidebar-collapsible')).query(By.css('a'));
+      const sidebarToggler = fixture.debugElement.query(By.css('#sidebar-collapse-toggle > a'));
       sidebarToggler.triggerEventHandler('click', {
         preventDefault: () => {/**/
         }

src/app/admin/admin-sidebar/admin-sidebar.component.ts

@@ -1,7 +1,7 @@
-import { Component, Injector, OnInit } from '@angular/core';
+import { Component, HostListener, Injector, OnInit } from '@angular/core';
 import { NgbModal } from '@ng-bootstrap/ng-bootstrap';
-import { combineLatest, combineLatest as observableCombineLatest, Observable } from 'rxjs';
-import { first, map, take } from 'rxjs/operators';
+import { combineLatest, combineLatest as observableCombineLatest, Observable, BehaviorSubject } from 'rxjs';
+import { debounceTime, first, map, take, distinctUntilChanged, withLatestFrom } from 'rxjs/operators';
 import { AuthService } from '../../core/auth/auth.service';
 import { ScriptDataService } from '../../core/data/processes/script-data.service';
 import { slideHorizontal, slideSidebar } from '../../shared/animations/slide';
@@ -60,6 +60,8 @@ export class AdminSidebarComponent extends MenuComponent implements OnInit {
    */
   sidebarExpanded: Observable<boolean>;
 
+  inFocus$: BehaviorSubject<boolean>;
+
   constructor(protected menuService: MenuService,
               protected injector: Injector,
               private variableService: CSSVariableService,
@@ -69,6 +71,7 @@ export class AdminSidebarComponent extends MenuComponent implements OnInit {
               private scriptDataService: ScriptDataService,
   ) {
     super(menuService, injector);
+    this.inFocus$ = new BehaviorSubject(false);
   }
 
   /**
@@ -89,10 +92,25 @@ export class AdminSidebarComponent extends MenuComponent implements OnInit {
         this.sidebarOpen = !collapsed;
         this.sidebarClosed = collapsed;
       });
-    this.sidebarExpanded = observableCombineLatest(this.menuCollapsed, this.menuPreviewCollapsed)
+    this.sidebarExpanded = combineLatest([this.menuCollapsed, this.menuPreviewCollapsed])
       .pipe(
         map(([collapsed, previewCollapsed]) => (!collapsed || !previewCollapsed))
       );
+    this.inFocus$.pipe(
+      debounceTime(50),
+      distinctUntilChanged(),  // disregard focusout in situations like --(focusout)-(focusin)--
+      withLatestFrom(
+        combineLatest([this.menuCollapsed, this.menuPreviewCollapsed])
+      ),
+    ).subscribe(([inFocus, [collapsed, previewCollapsed]]) => {
+      if (collapsed) {
+        if (inFocus && previewCollapsed) {
+          this.expandPreview(new Event('focusin → expand'));
+        } else if (!inFocus && !previewCollapsed) {
+          this.collapsePreview(new Event('focusout → collapse'));
+        }
+      }
+    });
   }
 
   /**
@@ -590,6 +608,32 @@ export class AdminSidebarComponent extends MenuComponent implements OnInit {
     });
   }
 
+  @HostListener('focusin')
+  public handleFocusIn() {
+    this.inFocus$.next(true);
+  }
+
+  @HostListener('focusout')
+  public handleFocusOut() {
+    this.inFocus$.next(false);
+  }
+
+  public handleMouseEnter(event: any) {
+    if (!this.inFocus$.getValue()) {
+      this.expandPreview(event);
+    } else {
+      event.preventDefault();
+    }
+  }
+
+  public handleMouseLeave(event: any) {
+    if (!this.inFocus$.getValue()) {
+      this.collapsePreview(event);
+    } else {
+      event.preventDefault();
+    }
+  }
+
   /**
    * Method to change this.collapsed to false when the slide animation ends and is sliding open
    * @param event The animation event