From e3c5a7e35ded8d33c47250ac2e019517dceae63e Mon Sep 17 00:00:00 2001
From: Jim Rice <hitjim@gmail.com>
Date: Mon, 31 Jan 2022 15:48:25 -0600
Subject: [PATCH] feat(card-cluster, tabs, table): better ARIA support (#305)

- screen reader reads card cluster selection and optionally also the longform  description
- possible aria fix by adding optional  param for tablist, which allows use of <button> as tab element instead  of <a>
- screen reader reads table sort type via adding proper enums, table element  roles
---
 .../card-cluster/card-cluster.component.html  | 14 +++--
 .../card-cluster/card-cluster.component.ts    | 14 +++++
 .../card-cluster/card-cluster.models.ts       |  3 +
 .../card-cluster/card-cluster.stories.ts      | 33 +++++++++++
 src/app/modules/table2/table.component.html   |  7 ++-
 src/app/modules/table2/table.models.ts        | 12 ++++
 src/app/modules/tabs/tabs.component.html      | 55 +++++++++++++------
 src/app/modules/tabs/tabs.component.scss      | 10 +++-
 src/app/modules/tabs/tabs.component.ts        |  6 ++
 src/app/modules/tabs/tabs.stories.ts          | 42 +++++++++++++-
 10 files changed, 169 insertions(+), 27 deletions(-)

diff --git a/src/app/modules/card-cluster/card-cluster.component.html b/src/app/modules/card-cluster/card-cluster.component.html
index eefb3849..79492ed0 100644
--- a/src/app/modules/card-cluster/card-cluster.component.html
+++ b/src/app/modules/card-cluster/card-cluster.component.html
@@ -52,18 +52,20 @@
     class="ds-l-md-col--12 ds-l-sm-col--12"
     id="followCard"
   >
-    <div class="ds-l-row">
+    <div class="ds-l-row" [attr.role]="cardArray.showRadioButton ? 'radiogroup' : undefined">
       <ng-container *ngFor="let card of cardArray.cluster; let x = index">
         <div class="ds-l-col" [ngClass]="(x + 1) % 2 === 0 ? 'padding-L-0' : ''">
           <div
-            id="{{ 'CardCluster' + (x + 1) }}"
-            [ngClass]="{ active: cardArray.buttonClicked === 'CardCluster' + (x + 1), disabled: card.disabled }"
+            id="{{ card.id }}"
+            [ngClass]="{ active: cardArray.buttonClicked === card.id, disabled: card.disabled }"
             class="{{ 'pointer card cardHeight ds-u-padding--1 ds-u-margin-bottom--2 ' + card.classButton }}"
             (click)="passAction($event)"
             (keyup.enter)="passAction($event)"
-            role="button"
+            [attr.role]="cardArray.showRadioButton ? 'radio' : 'button'"
             [attr.tabindex]="card.disabled ? null : 0"
             [attr.aria-label]="card.ariaLabel || card.name"
+            [attr.aria-describedby]="card.ariaDescribedByName ? card.id + '-name' : undefined"
+            [attr.aria-checked]="cardArray.showRadioButton ? (cardArray.buttonClicked === card.id) : undefined"
           >
             <div class="cardClusterContent ds-l-row ds-u-margin-x--0">
               <div *ngIf="cardArray.showRadioButton" class="ds-l-col--auto ds-u-padding-x--0">
@@ -73,7 +75,7 @@
                   size="small"
                   [value]="card.name"
                   groupName="cardClusterCheckbox"
-                  [isChecked]="cardArray.buttonClicked === 'CardCluster' + (x + 1)"
+                  [isChecked]="cardArray.buttonClicked === card.id"
                   [showTitle]="true"
                   [label]="card.ariaLabel"
                   [ariaLabel]="card.ariaLabel"
@@ -88,7 +90,7 @@
                   <fa-icon [icon]="card.valueIcon" *ngIf="card.valueIcon"></fa-icon>
                   {{ card.value === undefined || card.value === '' ? '&nbsp;' : card.value }}
                 </div>
-                <div class="ds-text noPoint" [ngClass]="card.className">
+                <div class="ds-text noPoint" [ngClass]="card.className" id="{{ card.id }}-name">
                   <fa-icon [icon]="card.nameIcon" *ngIf="card.nameIcon"></fa-icon>
                   {{ !card.name ? '&nbsp;' : card.name }}
                 </div>
diff --git a/src/app/modules/card-cluster/card-cluster.component.ts b/src/app/modules/card-cluster/card-cluster.component.ts
index af657be0..e457a193 100644
--- a/src/app/modules/card-cluster/card-cluster.component.ts
+++ b/src/app/modules/card-cluster/card-cluster.component.ts
@@ -94,6 +94,20 @@ export class AppCardClusterComponent implements OnInit {
       }
     }
     this.resize();
+
+    // Handle optional parameters appropriately
+    let idPrefix: string;
+    if (this.cardArray.cluster.length > 0) {
+      if (this.cardArray.clusterIdPrefix) {
+        idPrefix = this.cardArray.clusterIdPrefix;
+      }
+      this.cardArray.cluster.forEach(function(card, index) {
+        // Set unique IDs for each card, if not already explicitely defined.
+        if (!card.id) {
+          card.id = idPrefix ? `${idPrefix}${index}` : `CardCluster${index + 1}`;
+        }
+      });
+    }
   }
 
   resize() {
diff --git a/src/app/modules/card-cluster/card-cluster.models.ts b/src/app/modules/card-cluster/card-cluster.models.ts
index dc10f37c..ee85ee09 100644
--- a/src/app/modules/card-cluster/card-cluster.models.ts
+++ b/src/app/modules/card-cluster/card-cluster.models.ts
@@ -5,8 +5,10 @@ export class CardClusterClusterModel extends AngularDesignSystemModel {
   value: string | number;
   name?: string;
   nameIcon?: IconDefinition;
+  id?: string;
   valueIcon?: IconDefinition;
   ariaLabel?: string;
+  ariaDescribedByName?: false;
   classValue?: string;
   classButton?: string;
   className?: string;
@@ -31,6 +33,7 @@ export class CardClusterModel extends AngularDesignSystemModel {
   buttonClicked?: string;
   dataAutoId?: string;
   resizeTimeout?: number;
+  clusterIdPrefix?: string;
   cluster: CardClusterClusterModel[];
 
   constructor(options?) {
diff --git a/src/app/modules/card-cluster/card-cluster.stories.ts b/src/app/modules/card-cluster/card-cluster.stories.ts
index 9bed30b8..73c85037 100644
--- a/src/app/modules/card-cluster/card-cluster.stories.ts
+++ b/src/app/modules/card-cluster/card-cluster.stories.ts
@@ -8,6 +8,14 @@ import { AppCardClusterComponent } from './card-cluster.component';
 import { CardClusterModel } from './card-cluster.models';
 import { CardClusterModule } from './card-cluster.module';
 
+// NOTE: when running the storybook on localhost, you may find that while running
+// from the "Docs" view, the cards eventually stop rendering. This is a known issue.
+// The current implementatinon of Card Cluster, and the Stories here, do not ensure that
+// every `id` is unique. Whether that's a root cause for this Storybook behavior, it would
+// be a great enhancement.
+//
+// Either way, the workaround with Storybook is to stay in the "Canvas" view, and view each
+// story individually.
 const cardClusterDataNormal = new CardClusterModel({
   mainCard: true,
   mainTitle: 'STK-2',
@@ -90,12 +98,14 @@ const cardClusterDataShowRadioButtonsAndIcons = new CardClusterModel({
   mainCard: false,
   showRadioButton: true,
   rowMaxItems: 2,
+  clusterIdPrefix: 'userSelectCard',
   cluster: [
     {
       value: 'Basic User',
       name: 'A Basic User is an Organization Role with Read/Write Access to the Organization(s) in their system.',
       valueIcon: faEye,
       ariaLabel: 'Activate enter to select Basic User',
+      ariaDescribedByName: true,
       classButton: 'ds-u-padding--2',
     },
     {
@@ -103,6 +113,7 @@ const cardClusterDataShowRadioButtonsAndIcons = new CardClusterModel({
       name: 'A Security Administrator is a person who manages User Roles & Permissions for their Organization.',
       valueIcon: faUserShield,
       ariaLabel: 'Activate enter to select Security Administrator',
+      ariaDescribedByName: true,
       classButton: 'ds-u-padding--2',
     },
   ],
@@ -183,6 +194,14 @@ export const Intro: Story<ComponentIntroComponent> = () => ({
             value:
               'The total to show in the main card.  Leave blank to calculate based on the sum of all values in the cluster.',
           },
+          {
+            name: 'clusterIdPrefix',
+            type: 'string',
+            optional: true,
+            // tslint:disable-next-line: max-line-length
+            value:
+              'NOTE: Overrides card cluster array item `id` field. This is a common string prefix to be enumerated in each cluster card element id. I.e. `role` for an array of 2 cards would generate ids `role1, role2`. Leaving blank will use `CardCluster` as the prefix.',
+          },
           {
             name: 'ariaLabel',
             type: 'string',
@@ -228,6 +247,12 @@ export const Intro: Story<ComponentIntroComponent> = () => ({
                 type: 'number | string',
                 value: 'The value to show in the card inside the cluster',
               },
+              {
+                name: 'id',
+                type: 'string',
+                optional: true,
+                value: 'NOTE: This is overridden when using `clusterIdPrefix`. Manually define the card element id. ',
+              },
               {
                 name: 'nameIcon',
                 type: 'IconDefinition',
@@ -248,6 +273,14 @@ export const Intro: Story<ComponentIntroComponent> = () => ({
                 value:
                   "Use this to further specify main card to the screen reader.  Leave blank to use the card's name.",
               },
+              {
+                name: 'ariaDescribedByName',
+                type: 'string',
+                optional: true,
+                // tslint:disable-next-line: max-line-length
+                value:
+                  'Allows `name` text to get read by a screen reader in addition to `ariaLabel`. Best used when a card has lengthy extra text provided by `name`, or when you want that text',
+              },
               {
                 name: 'classButton',
                 optional: true,
diff --git a/src/app/modules/table2/table.component.html b/src/app/modules/table2/table.component.html
index b2ebd25d..b734a4ef 100644
--- a/src/app/modules/table2/table.component.html
+++ b/src/app/modules/table2/table.component.html
@@ -9,14 +9,15 @@
   class="table-background ds-c-table ds-c-table--borderless"
   [ngClass]="tableModel.class"
   [id]="tableModel.id"
+  role="table"
 >
   <caption *ngIf="tableModel.summary" class="tableHeading sr-only">
     {{
       tableModel.summary
     }}
   </caption>
-  <thead *ngIf="tableModel.headerRows.length" class="tableHeading" [ngClass]="tableModel.headerClass">
-    <tr *ngFor="let headerRow of tableModel.headerRows" [attr.class]="headerRow.class">
+  <thead *ngIf="tableModel.headerRows.length" class="tableHeading" [ngClass]="tableModel.headerClass" role ="rowgroup">
+    <tr *ngFor="let headerRow of tableModel.headerRows" [attr.class]="headerRow.class" role="row">
       <th
         *ngFor="let header of headerRow.cells"
         scope="col"
@@ -26,6 +27,8 @@
         [attr.rowspan]="header.rowspan"
         [attr.scope]="header.colspan > 1 ? 'colgroup' : null"
         [attr.title]="header.label"
+        role="columnheader"
+        [attr.aria-sort]="header.ariaSort ? header.ariaSort : undefined"
       >
         <app-table-header-2 [tableHeaderModel]="header" (headerClick)="headerClick($event)"></app-table-header-2>
       </th>
diff --git a/src/app/modules/table2/table.models.ts b/src/app/modules/table2/table.models.ts
index 7833dfdc..dc6e5d0b 100644
--- a/src/app/modules/table2/table.models.ts
+++ b/src/app/modules/table2/table.models.ts
@@ -18,6 +18,14 @@ export enum TableHeaderSortEnum {
   DESC = 'desc',
 }
 
+// https://developer.mozilla.org/en-US/docs/Web/API/Element/ariaSort
+export enum TableHeaderSortAriaEnum {
+  NONE = 'none',
+  ASCENDING = 'ascending',
+  DESCENDING = 'descending',
+  OTHER = 'other',
+}
+
 export class TableHeaderModel extends AngularDesignSystemModel {
   columnKey: string;
   label: string;
@@ -32,6 +40,7 @@ export class TableHeaderModel extends AngularDesignSystemModel {
 
   // Sort Properties
   sort?: TableHeaderSortEnum = TableHeaderSortEnum.NONE;
+  ariaSort?: TableHeaderSortAriaEnum = TableHeaderSortAriaEnum.NONE;
 
   // Checkbox Properties
   isChecked? = false;
@@ -185,11 +194,14 @@ export class TableModel extends AngularDesignSystemModel {
         if (header.columnKey === columnKey) {
           if (header.sort === TableHeaderSortEnum.NONE || header.sort === TableHeaderSortEnum.ASC) {
             header.sort = TableHeaderSortEnum.DESC;
+            header.ariaSort = TableHeaderSortAriaEnum.DESCENDING;
           } else {
             header.sort = TableHeaderSortEnum.ASC;
+            header.ariaSort = TableHeaderSortAriaEnum.ASCENDING
           }
         } else {
           header.sort = TableHeaderSortEnum.NONE;
+          header.ariaSort = TableHeaderSortAriaEnum.NONE;
         }
       }
     }
diff --git a/src/app/modules/tabs/tabs.component.html b/src/app/modules/tabs/tabs.component.html
index 2e695d1a..5a643650 100644
--- a/src/app/modules/tabs/tabs.component.html
+++ b/src/app/modules/tabs/tabs.component.html
@@ -1,17 +1,38 @@
-<div class="ds-c-tabs {{ tablistClassName }}" role="tablist">
-    <a
-        *ngFor="let tab of tabs"
-        href="{{ generateLink(tab) }}"
-        [id]="tab.id" 
-        class="ds-c-tabs__item {{ tab.className }}" 
-        [attr.aria-label]="tab.ariaLabel" 
-        [attr.aria-disabled]="tab.disabled"
-        [attr.aria-hidden]="tab.visible"
-        [attr.aria-selected]="tab.selected"
-        [attr.data-auto-id]="tab.dataAutoId"
-        (click)="passAction($event)" 
-        (keyup.enter)="passAction($event)" 
-    >
-        {{tab.title}}
-    </a>
-</div>
\ No newline at end of file
+<div *ngIf="tabsAsButtons; else anchorElse">
+    <div class="ds-c-tabs {{ tablistClassName }}" role="tablist">
+        <button
+            *ngFor="let tab of tabs"
+            role="tab"
+            [id]="tab.id"
+            class="ds-c-tabs__item {{ tab.className }}"
+            [attr.aria-label]="tab.ariaLabel"
+            [attr.aria-disabled]="tab.disabled"
+            [attr.aria-hidden]="tab.visible"
+            [attr.aria-selected]="tab.selected"
+            [attr.data-auto-id]="tab.dataAutoId"
+            (click)="passAction($event)"
+            (keyup.enter)="passAction($event)"
+        >
+            {{tab.title}}
+        </button>
+    </div>
+</div>
+<ng-template #anchorElse>
+    <div class="ds-c-tabs {{ tablistClassName }}" role="tablist">
+        <a
+            *ngFor="let tab of tabs"
+            href="{{ generateLink(tab) }}"
+            [id]="tab.id"
+            class="ds-c-tabs__item {{ tab.className }}"
+            [attr.aria-label]="tab.ariaLabel"
+            [attr.aria-disabled]="tab.disabled"
+            [attr.aria-hidden]="tab.visible"
+            [attr.aria-selected]="tab.selected"
+            [attr.data-auto-id]="tab.dataAutoId"
+            (click)="passAction($event)"
+            (keyup.enter)="passAction($event)"
+        >
+            {{tab.title}}
+        </a>
+    </div>
+</ng-template>
diff --git a/src/app/modules/tabs/tabs.component.scss b/src/app/modules/tabs/tabs.component.scss
index 4bec0a29..17e12445 100644
--- a/src/app/modules/tabs/tabs.component.scss
+++ b/src/app/modules/tabs/tabs.component.scss
@@ -1,4 +1,12 @@
+$tab-border-color: #d6d7d9;
+
 .ds-c-tabs__item {
     color: #323A45;
     font-weight: normal;
-}
\ No newline at end of file
+    border-left: 1px solid $tab-border-color;
+    border-right: 0px;
+}
+
+.ds-c-tabs__item:last-child {
+    border-right: 1px solid $tab-border-color
+}
diff --git a/src/app/modules/tabs/tabs.component.ts b/src/app/modules/tabs/tabs.component.ts
index 85805bf6..17787364 100644
--- a/src/app/modules/tabs/tabs.component.ts
+++ b/src/app/modules/tabs/tabs.component.ts
@@ -16,10 +16,15 @@ export class AppTabsComponent implements OnInit {
     @Input() tabs: TabModel[];
     @Input() defaultSelectedId: string;
     @Input() tablistClassName: string;
+    // Setting to true should let screen reader properly read when a tab is selected
+    @Input() tabsAsButtons?: boolean;
     @Output() onChange = new EventEmitter<any>();
 
     /*
      * ngOnInit is used to select a default tab when the component loads
+     * NOTE: We offer the option to use <button> instead of <a> for tabs, as shown in Mozilla and
+     *   w3schools examples, because tab content is not meant to be opened in
+     *   a new browser window or tab.
      */
     ngOnInit() {
         if (!this.tabs) {
@@ -38,6 +43,7 @@ export class AppTabsComponent implements OnInit {
         this.selectTab(this.defaultSelectedId);
     }
 
+    // Selects tab and passes along event to user-provided event handler, as assigned to onChange.
     passAction(e) {
         this.selectTab(e.srcElement.id);
         this.onChange.emit(e);
diff --git a/src/app/modules/tabs/tabs.stories.ts b/src/app/modules/tabs/tabs.stories.ts
index e8b54b16..a850bb09 100644
--- a/src/app/modules/tabs/tabs.stories.ts
+++ b/src/app/modules/tabs/tabs.stories.ts
@@ -20,7 +20,7 @@ const tabs = [
   {
     title: 'tab3',
     ariaLabel: 'tab3 mellow',
-    link: 'tab3gi',
+    link: 'tab3',
   },
 ];
 
@@ -29,6 +29,28 @@ const props = {
   tabs,
 };
 
+const buttons = [
+  {
+    title: 'tab1',
+    ariaLabel: 'tab1 hello',
+  },
+  {
+    title: 'tab2',
+    ariaLabel: 'tab1 yellow',
+  },
+  {
+    title: 'tab3',
+    ariaLabel: 'tab3 mellow',
+  },
+];
+
+const useButtons = true;
+const buttonsProps = {
+  ...defaultProps,
+  tabs: buttons,
+  useButtons: true,
+};
+
 export default {
   title: 'Components/Tabs',
   decorators: [
@@ -82,6 +104,12 @@ export const Intro: Story<ComponentIntroComponent> = () => ({
         value:
           'A javascript function that will be called when a tab is clicked. It takes one argument, which is a click event.',
       },
+      {
+        name: 'tabsAsButtons',
+        type: 'boolean',
+        optional: true,
+        value: 'Uses <button> instead of <a> for tab element for better ARIA support.',
+      },
     ],
     notes: [
       "Expected format for 'tabs'",
@@ -121,3 +149,15 @@ export const Normal: Story<AppTabsComponent> = () => ({
         `,
   props,
 });
+
+export const UsesButtonElement: Story<AppTabsComponent> = () => ({
+  template: `
+            <app-tabs
+                [tabsAsButtons]="useButtons"
+                [tabs]="tabs"
+                defaultSelectedId="tab2"
+                (onChange)="handleEvent($event)">
+            </app-tabs>
+        `,
+  props: buttonsProps,
+});