From 9de67763d2237ba6125e79546acabecf465b60eb Mon Sep 17 00:00:00 2001
From: Boaz Shuster <boaz.shuster.github@gmail.com>
Date: Mon, 7 Jan 2019 23:38:32 +0200
Subject: [PATCH] fix(Page): improve a11y in sidebar and header

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
---
 .../react-core/src/components/Page/PageHeader.d.ts   |  1 +
 .../react-core/src/components/Page/PageHeader.js     |  9 +++++++--
 .../react-core/src/components/Page/PageSidebar.js    |  5 +++--
 .../components/Page/__snapshots__/Page.test.js.snap  | 12 +++++++++---
 .../Page/__snapshots__/PageHeader.test.js.snap       |  1 +
 .../src/components/Page/examples/VerticalPage.js     |  1 +
 .../PageLayout/examples/PageLayoutDefaultNav.js      |  1 +
 .../PageLayout/examples/PageLayoutExpandableNav.js   |  1 +
 .../demos/PageLayout/examples/PageLayoutGroupsNav.js |  1 +
 .../demos/PageLayout/examples/PageLayoutSimpleNav.js |  1 +
 10 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/packages/patternfly-4/react-core/src/components/Page/PageHeader.d.ts b/packages/patternfly-4/react-core/src/components/Page/PageHeader.d.ts
index a4e609d210b..fbd5da942d0 100644
--- a/packages/patternfly-4/react-core/src/components/Page/PageHeader.d.ts
+++ b/packages/patternfly-4/react-core/src/components/Page/PageHeader.d.ts
@@ -8,6 +8,7 @@ export interface PageHeaderProps extends HTMLProps<HTMLDivElement> {
   avatar?: ReactNode;
   topNav?: ReactNode;
   showNavToggle?: boolean;
+  isNavOpen?: boolean;
   onNavToggle?: Function;
 }
 
diff --git a/packages/patternfly-4/react-core/src/components/Page/PageHeader.js b/packages/patternfly-4/react-core/src/components/Page/PageHeader.js
index 2c431245325..315f722ab0b 100644
--- a/packages/patternfly-4/react-core/src/components/Page/PageHeader.js
+++ b/packages/patternfly-4/react-core/src/components/Page/PageHeader.js
@@ -20,6 +20,8 @@ const propTypes = {
   topNav: PropTypes.node,
   /** True to show the nav toggle button (toggles side nav) */
   showNavToggle: PropTypes.bool,
+  /** True if the side nav is shown  */
+  isNavOpen: PropTypes.bool,
   /** Callback function to handle the side nav toggle button */
   onNavToggle: PropTypes.func
 };
@@ -32,10 +34,11 @@ const defaultProps = {
   avatar: null,
   topNav: null,
   showNavToggle: false,
+  isNavOpen: true,
   onNavToggle: () => undefined
 };
 
-const PageHeader = ({ className, logo, logoProps, toolbar, avatar, topNav, showNavToggle, onNavToggle, ...props }) => (
+const PageHeader = ({ className, logo, logoProps, toolbar, avatar, topNav, isNavOpen, showNavToggle, onNavToggle, ...props }) => (
   <header role="banner" className={css(styles.pageHeader, className)} {...props}>
     <div className={css(styles.pageHeaderBrand)}>
       {showNavToggle && (
@@ -43,7 +46,9 @@ const PageHeader = ({ className, logo, logoProps, toolbar, avatar, topNav, showN
           <Button
             id="nav-toggle"
             onClick={onNavToggle}
-            aria-label="Toggle primary navigation"
+            aria-label="Toggle global navigation"
+            aria-controls="page-sidebar"
+            aria-expanded={isNavOpen ? "true" : "false"}
             variant={ButtonVariant.plain}
           >
             <BarsIcon />
diff --git a/packages/patternfly-4/react-core/src/components/Page/PageSidebar.js b/packages/patternfly-4/react-core/src/components/Page/PageSidebar.js
index 92c5b348cad..27a1c204e08 100644
--- a/packages/patternfly-4/react-core/src/components/Page/PageSidebar.js
+++ b/packages/patternfly-4/react-core/src/components/Page/PageSidebar.js
@@ -19,7 +19,8 @@ const defaultProps = {
 };
 
 const PageSidebar = ({ className, nav, isNavOpen, ...props }) => (
-  <aside
+  <div
+    id="page-sidebar"
     className={css(
       styles.pageSidebar,
       isNavOpen && styles.modifiers.expanded,
@@ -29,7 +30,7 @@ const PageSidebar = ({ className, nav, isNavOpen, ...props }) => (
     {...props}
   >
     {nav}
-  </aside>
+  </div>
 );
 
 PageSidebar.propTypes = propTypes;
diff --git a/packages/patternfly-4/react-core/src/components/Page/__snapshots__/Page.test.js.snap b/packages/patternfly-4/react-core/src/components/Page/__snapshots__/Page.test.js.snap
index 36a8bccb902..e1f765263f0 100644
--- a/packages/patternfly-4/react-core/src/components/Page/__snapshots__/Page.test.js.snap
+++ b/packages/patternfly-4/react-core/src/components/Page/__snapshots__/Page.test.js.snap
@@ -79,6 +79,7 @@ exports[`Check page horizontal layout example against snapshot 1`] = `
     <PageHeader
       avatar=" | Avatar"
       className=""
+      isNavOpen={true}
       logo="Logo"
       logoProps={null}
       nav="Navigation"
@@ -107,6 +108,7 @@ exports[`Check page horizontal layout example against snapshot 1`] = `
     <PageHeader
       avatar=" | Avatar"
       className=""
+      isNavOpen={true}
       logo="Logo"
       logoProps={null}
       nav="Navigation"
@@ -142,8 +144,9 @@ exports[`Check page horizontal layout example against snapshot 1`] = `
       isNavOpen={true}
       nav={null}
     >
-      <aside
+      <div
         className="pf-c-page__sidebar pf-m-expanded"
+        id="page-sidebar"
       />
     </PageSidebar>
     <main
@@ -274,6 +277,7 @@ exports[`Check page vertical layout example against snapshot 1`] = `
     <PageHeader
       avatar=" | Avatar"
       className=""
+      isNavOpen={true}
       logo="Logo"
       logoProps={null}
       onNavToggle={[Function]}
@@ -299,6 +303,7 @@ exports[`Check page vertical layout example against snapshot 1`] = `
     <PageHeader
       avatar=" | Avatar"
       className=""
+      isNavOpen={true}
       logo="Logo"
       logoProps={null}
       onNavToggle={[Function]}
@@ -332,11 +337,12 @@ exports[`Check page vertical layout example against snapshot 1`] = `
       isNavOpen={true}
       nav="Navigation"
     >
-      <aside
+      <div
         className="pf-c-page__sidebar pf-m-expanded"
+        id="page-sidebar"
       >
         Navigation
-      </aside>
+      </div>
     </PageSidebar>
     <main
       className="pf-c-page__main"
diff --git a/packages/patternfly-4/react-core/src/components/Page/__snapshots__/PageHeader.test.js.snap b/packages/patternfly-4/react-core/src/components/Page/__snapshots__/PageHeader.test.js.snap
index 36fcf71b26c..3d51f4f31dd 100644
--- a/packages/patternfly-4/react-core/src/components/Page/__snapshots__/PageHeader.test.js.snap
+++ b/packages/patternfly-4/react-core/src/components/Page/__snapshots__/PageHeader.test.js.snap
@@ -27,6 +27,7 @@ exports[`Check page vertical layout example against snapshot 1`] = `
 <PageHeader
   avatar=" | Avatar"
   className=""
+  isNavOpen={true}
   logo="Logo"
   logoProps={null}
   onNavToggle={[Function]}
diff --git a/packages/patternfly-4/react-core/src/components/Page/examples/VerticalPage.js b/packages/patternfly-4/react-core/src/components/Page/examples/VerticalPage.js
index 82fec5bd913..c2c1a530e7a 100644
--- a/packages/patternfly-4/react-core/src/components/Page/examples/VerticalPage.js
+++ b/packages/patternfly-4/react-core/src/components/Page/examples/VerticalPage.js
@@ -30,6 +30,7 @@ class VerticalPage extends React.Component {
         toolbar="Toolbar"
         avatar=" | Avatar"
         showNavToggle
+        isNavOpen={isNavOpen}
         onNavToggle={this.onNavToggle}
       />
     );
diff --git a/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutDefaultNav.js b/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutDefaultNav.js
index c40bb78d36c..1d2addfe900 100644
--- a/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutDefaultNav.js
+++ b/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutDefaultNav.js
@@ -184,6 +184,7 @@ class PageLayoutDefaultNav extends React.Component {
         avatar={<Avatar src={avatarImg} alt="Avatar image" />}
         showNavToggle
         onNavToggle={this.onNavToggle}
+        isNavOpen={isNavOpen}
       />
     );
     const Sidebar = <PageSidebar nav={PageNav} isNavOpen={isNavOpen} />;
diff --git a/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutExpandableNav.js b/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutExpandableNav.js
index 0d4ba86cc08..74493b93fc1 100644
--- a/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutExpandableNav.js
+++ b/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutExpandableNav.js
@@ -208,6 +208,7 @@ class PageLayoutExpandableNav extends React.Component {
         avatar={<Avatar src={avatarImg} alt="Avatar image" />}
         showNavToggle
         onNavToggle={this.onNavToggle}
+        isNavOpen={isNavOpen}
       />
     );
     const Sidebar = <PageSidebar nav={PageNav} isNavOpen={isNavOpen} />;
diff --git a/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutGroupsNav.js b/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutGroupsNav.js
index c383e872bcd..bb3364ee99d 100644
--- a/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutGroupsNav.js
+++ b/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutGroupsNav.js
@@ -194,6 +194,7 @@ class PageLayoutGroupsNav extends React.Component {
         avatar={<Avatar src={avatarImg} alt="Avatar image" />}
         showNavToggle
         onNavToggle={this.onNavToggle}
+        isNavOpen={isNavOpen}
       />
     );
     const Sidebar = <PageSidebar nav={PageNav} isNavOpen={isNavOpen} />;
diff --git a/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutSimpleNav.js b/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutSimpleNav.js
index 25f27ff87f3..6bf06be195b 100644
--- a/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutSimpleNav.js
+++ b/packages/patternfly-4/react-core/src/demos/PageLayout/examples/PageLayoutSimpleNav.js
@@ -185,6 +185,7 @@ class PageLayoutSimpleNav extends React.Component {
         avatar={<Avatar src={avatarImg} alt="Avatar image" />}
         showNavToggle
         onNavToggle={this.onNavToggle}
+        isNavOpen={isNavOpen}
       />
     );
     const Sidebar = <PageSidebar nav={PageNav} isNavOpen={isNavOpen} />;