Skip to content

Added focus zone around navigation to make it more keyboard accessible #1596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dzearing merged 3 commits into from
Apr 25, 2017
Merged

Added focus zone around navigation to make it more keyboard accessible #1596

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48bcc0a
Added focus zone around navigation to make it more keyboard accessible
micahgodbolt Apr 25, 2017
fcc901d
Remove ref and change role to menu
micahgodbolt Apr 25, 2017
7f74148
removed redundant role attribute
micahgodbolt Apr 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions apps/fabric-website/src/components/Nav/Nav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as React from 'react';
import { css } from 'office-ui-fabric-react/lib/Utilities';
import { FocusZone, FocusZoneDirection } from 'office-ui-fabric-react/lib/FocusZone';
import * as stylesImport from './Nav.module.scss';
const styles: any = stylesImport;
import {
Expand All @@ -24,9 +25,11 @@ export class Nav extends React.Component<INavProps, INavState> {
: null;

return (
<nav className={ styles.nav } role='navigation'>
{ links }
</nav>
<FocusZone ref='focusZone' direction={ FocusZoneDirection.vertical } role='menubar' >

@mdahamiwal mdahamiwal Apr 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding role='menubar'? a menubar requires nested menuitems as per https://www.w3.org/TR/wai-aria-practices-1.1/#menu . Also we should avoid using string refs https://facebook.github.io/react/docs/refs-and-the-dom.html#legacy-api-string-refs

@micahgodbolt micahgodbolt Apr 25, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pulled this implementation over from NavBar. I can remove the ref. As for the role, which should we use?

@mdahamiwal mdahamiwal Apr 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal nav is already assigned navigation (its redundant as well <nav>s are implicit navigation) and focus zone will auto focus the first active element which would be one of the list options. IMO, you won't require any role on FocusZone.

@betrue-final-final betrue-final-final Apr 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that might fix the absence of a label in JAWS for Components and Styles

@mdahamiwal mdahamiwal Apr 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@micahgodbolt I am having trouble understanding the implementation here. We don't really need a role (menubar or menu) on focuszone. check https://www.w3.org/TR/wai-aria-1.1/#navigation and our Nav already follows this guidelines.

@micahgodbolt micahgodbolt Apr 25, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, role is gone. didn't realize how redundant it was in this case

<nav className={ styles.nav } role='navigation'>
{ links }
</nav>
</FocusZone>
);
}

Expand Down