From aca735819a63caed7bfcdf6ce1ebba3611d00c1b Mon Sep 17 00:00:00 2001
From: Ben Yackley <61990921+beyackle@users.noreply.github.com>
Date: Wed, 8 Apr 2020 15:35:33 -0700
Subject: [PATCH] a11y: fix NavItem a11y issues (#2502)

* fix NavItem a11y issues

* Update styles.ts

* fix NavItem a11y issues

* Update styles.ts

* Update styles.ts

* move data-testid

* Fix selected offset in left nav

Co-authored-by: Andy Brown <asbrown002@gmail.com>
Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
---
 .../client/src/components/NavItem/index.tsx   | 42 ++++++-----
 .../client/src/components/NavItem/styles.ts   | 71 ++++++-------------
 2 files changed, 48 insertions(+), 65 deletions(-)

diff --git a/Composer/packages/client/src/components/NavItem/index.tsx b/Composer/packages/client/src/components/NavItem/index.tsx
index d6a8e487dd..7490ca894b 100644
--- a/Composer/packages/client/src/components/NavItem/index.tsx
+++ b/Composer/packages/client/src/components/NavItem/index.tsx
@@ -2,15 +2,15 @@
 // Licensed under the MIT License.
 
 /** @jsx jsx */
-import { jsx } from '@emotion/core';
+import { jsx, css } from '@emotion/core';
 import { useCallback, useContext } from 'react';
 import { Link } from '@reach/router';
-import { CommandBarButton } from 'office-ui-fabric-react/lib/Button';
+import { Icon } from 'office-ui-fabric-react/lib/Icon';
 
 import { StoreContext } from '../../store';
 import { useLocation } from '../../utils/hooks';
 
-import { link, outer, commandBarButton } from './styles';
+import { link, icon } from './styles';
 
 /**
  * @param to The string URI to link to. Supports relative and absolute URIs.
@@ -40,26 +40,34 @@ export const NavItem: React.FC<INavItemProps> = props => {
 
   const addRef = useCallback(ref => onboardingAddCoachMarkRef({ [`nav${labelName.replace(' ', '')}`]: ref }), []);
 
+  const activeArea = (
+    <div css={link(active, disabled)} aria-hidden="true" tabIndex={-1}>
+      <Icon data-testid={'LeftNav-CommandBarButton' + labelName} iconName={iconName} styles={icon(active, disabled)} />
+      {labelName}
+    </div>
+  );
+
+  if (disabled) {
+    // make it so we can't even click them by accident and lead to the error page
+    return activeArea;
+  }
+
   return (
     <Link
       to={to}
-      css={link(active, disabled)}
-      data-testid={'LeftNav-CommandBarButton' + labelName}
       aria-disabled={disabled}
-      aria-label={labelName}
+      aria-label={labelName + (active ? '; selected' : '')}
       ref={addRef}
+      css={css`
+        :link {
+          text-decoration: none;
+        }
+        :visited {
+          text-decoration: none;
+        }
+      `}
     >
-      <div css={outer} aria-hidden="true" tabIndex={-1}>
-        <CommandBarButton
-          iconProps={{
-            iconName,
-          }}
-          text={labelName}
-          styles={commandBarButton(active)}
-          disabled={disabled}
-          ariaHidden
-        />
-      </div>
+      {activeArea}
     </Link>
   );
 };
diff --git a/Composer/packages/client/src/components/NavItem/styles.ts b/Composer/packages/client/src/components/NavItem/styles.ts
index dea80eab7c..ec7de0f0f5 100644
--- a/Composer/packages/client/src/components/NavItem/styles.ts
+++ b/Composer/packages/client/src/components/NavItem/styles.ts
@@ -6,24 +6,27 @@ import { FontSizes } from '@uifabric/fluent-theme';
 import { NeutralColors, CommunicationColors } from '@uifabric/fluent-theme';
 import { IButtonStyles } from 'office-ui-fabric-react/lib/Button';
 
-export const link = (active, disabled) => css`
-  display: block;
+export const link = (active: boolean, disabled: boolean) => css`
+  display: flex;
+  align-items: center;
   text-decoration: none;
-  color: #4f4f4f;
+  color: ${disabled ? '#999' : '#4f4f4f'};
   position: relative;
 
-  ${disabled && `pointer-events: none;`}
-  ${!disabled &&
-    `&::after {
-      content: '';
-      position: absolute;
-      top: 0px;
-      right: 1px;
-      bottom: 0px;
-      left: 1px;
-    }
+  width: 220px;
 
-    &:hover {
+  ${active
+    ? `background-color: ${NeutralColors.white};
+ 
+     border-left: 3px solid ${CommunicationColors.primary};
+    `
+    : `
+     background-color: transparent;
+    `}
+
+  ${disabled
+    ? `pointer-events: none;`
+    : `&:hover {
       background-color: ${NeutralColors.gray50};
     }
 
@@ -38,46 +41,18 @@ export const link = (active, disabled) => css`
         outline: rgb(102, 102, 102) solid 1px;
       }
     }
-
-    ${active &&
-      `background-color: ${NeutralColors.white};
-
-      &::after {
-        border-left: 3px solid ${CommunicationColors.primary};
-      }`}
   `}
 `;
 
-export const outer = css`
-  display: flex;
-  width: 220px;
-  background-color: transparent;
-`;
-
-export const commandBarButton = active =>
+export const icon = (active: boolean, disabled: boolean) =>
   ({
     root: {
-      color: active ? '#000' : '#4f4f4f',
-      height: '36px',
-      width: '220px',
-      fontSize: `${FontSizes.size14}`,
-      paddingLeft: '0px',
-      paddingRight: '0px',
-      marginLeft: '0px',
-      backgroundColor: 'transparent',
-    },
-    rootDisabled: {
-      backgroundColor: 'transparent',
-    },
-    icon: {
-      color: active ? '#000' : '#4f4f4f',
-      padding: '0 16px',
-      marginLeft: '0px',
+      color: active ? '#000' : disabled ? '#999' : '#4f4f4f',
+      padding: '8px 12px',
+      marginLeft: active ? '1px' : '4px',
+      marginRight: '12px',
       boxSizing: 'border-box',
       fontSize: `${FontSizes.size16}`,
-    },
-    textContainer: {
-      textAlign: 'left',
-      zIndex: 1,
+      width: '40px',
     },
   } as IButtonStyles);