Skip to content

Fixes focus issue for contextual menus #4744

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
joschect merged 17 commits into from
May 7, 2018
Merged

Fixes focus issue for contextual menus #4744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9f877be
Best behavior for keyboarding
rebeccaballantyne01 May 1, 2018
2947a4d
Adds in new parameter, and basics of different aria roles
rebeccaballantyne01 May 1, 2018
3e99a25
Removing unnecessary group role for when focus moves to first menu item
rebeccaballantyne01 May 1, 2018
8432116
Adding in differences between keyboard and mouse/click action in Base…
rebeccaballantyne01 May 1, 2018
f29bfd4
Fixes issue with Edge + Narrator combination
rebeccaballantyne01 May 2, 2018
fde5885
Fixing up comment
rebeccaballantyne01 May 2, 2018
19e4d27
Moving fix for Edge + Narrator to ContextualMenu.tsx
rebeccaballantyne01 May 2, 2018
fd759fb
Removing mistaken commit for testing
rebeccaballantyne01 May 2, 2018
2205afc
Rush change file
rebeccaballantyne01 May 2, 2018
a6a7095
Avoid modifying state directly
rebeccaballantyne01 May 3, 2018
c929768
moving prop to proper place
rebeccaballantyne01 May 3, 2018
34c3bb2
Adding in comment in ContextualMenu.tsx
rebeccaballantyne01 May 3, 2018
3ef3590
Updating package.json bundle size
rebeccaballantyne01 May 3, 2018
c871515
Merge branch 'master' into rebeba/contextual-menu-focus
dzearing May 7, 2018
0fe31a7
Fixing accidental removal of path
rebeccaballantyne01 May 7, 2018
222a46b
Deleting old rush change file, should be minor, not patch
rebeccaballantyne01 May 7, 2018
0a8c819
New rush change file for minor change
rebeccaballantyne01 May 7, 2018
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
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/rebeba-contextual-menu-focus_2018-05-07-21-16.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "Fixes issue where focus isn't displayed correctly on the contextual mneu",
"type": "minor"
}
],
"packageName": "office-ui-fabric-react",
"email": "[email protected]"
}
28 changes: 18 additions & 10 deletions packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
id={ this._labelId + '-menu' }
directionalHint={ DirectionalHint.bottomLeftEdge }
{ ...menuProps }
shouldFocusOnContainer={ this.state.menuProps ? this.state.menuProps.shouldFocusOnContainer : undefined }
className={ 'ms-BaseButton-menuhost ' + menuProps.className }
target={ this._isSplitButton ? this._splitButtonContainer.current : this._buttonElement.current }
labelElementId={ this._labelId }
Expand All @@ -447,26 +448,26 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
this.setState({ menuProps: menuProps });
}

private _openMenu = (): void => {
private _openMenu = (shouldFocusOnContainer?: boolean): void => {
if (this.props.menuProps) {
const menuProps = this.props.menuProps;
const menuProps = {...this.props.menuProps, shouldFocusOnContainer: shouldFocusOnContainer };
if (this.props.persistMenu) {
menuProps.hidden = false;
}
this.setState({ menuProps: menuProps });
}
}

private _onToggleMenu = (): void => {
private _onToggleMenu = (shouldFocusOnContainer: boolean): void => {
if (this._splitButtonContainer.current) {
this._splitButtonContainer.current.focus();
}

const currentMenuProps = this.state.menuProps;
if (this.props.persistMenu) {
currentMenuProps && currentMenuProps.hidden ? this._openMenu() : this._dismissMenu();
currentMenuProps && currentMenuProps.hidden ? this._openMenu(shouldFocusOnContainer) : this._dismissMenu();
} else {
currentMenuProps ? this._dismissMenu() : this._openMenu();
currentMenuProps ? this._dismissMenu() : this._openMenu(shouldFocusOnContainer);
}
}

Expand Down Expand Up @@ -628,14 +629,13 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
this.props.onKeyDown(ev);
}

if (!ev.defaultPrevented &&
this._isValidMenuOpenKey(ev)) {
if (!ev.defaultPrevented && this._isValidMenuOpenKey(ev)) {
const { onMenuClick } = this.props;
if (onMenuClick) {
onMenuClick(ev, this);
}

this._onToggleMenu();
this._onToggleMenu(false);
ev.preventDefault();
ev.stopPropagation();
}
Expand Down Expand Up @@ -680,7 +680,11 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
if (this.props.menuTriggerKeyCode) {
return ev.which === this.props.menuTriggerKeyCode;
} else {
return ev.which === KeyCodes.down && (ev.altKey || ev.metaKey);
if (this._isSplitButton) {
return ev.which === KeyCodes.down && (ev.altKey || ev.metaKey);
} else {
return ev.which === KeyCodes.enter;
}
}
}

Expand All @@ -691,7 +695,11 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
}

if (!ev.defaultPrevented) {
this._onToggleMenu();
// When Edge + Narrator are used together (regardless of if the button is in a form or not), pressing
// "Enter" fires this method and not _onMenuKeyDown. Checking ev.nativeEvent.detail differentiates
// between a real click event and a keypress event.
const shouldFocusOnContainer = ev.nativeEvent.detail !== 0;
this._onToggleMenu(shouldFocusOnContainer);
ev.preventDefault();
ev.stopPropagation();
}
Expand Down
27 changes: 23 additions & 4 deletions packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { ContextualMenuSplitButton } from './ContextualMenuSplitButton';

export interface IContextualMenuState {
expandedMenuItemKey?: string;
expandedByMouseClick?: boolean;
dismissedMenuItemKey?: string;
contextualMenuItems?: IContextualMenuItem[];
contextualMenuTarget?: Element;
Expand Down Expand Up @@ -193,6 +194,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
useTargetAsMinWidth,
directionalHintFixed,
shouldFocusOnMount,
shouldFocusOnContainer,
title,
theme,
calloutProps,
Expand Down Expand Up @@ -276,14 +278,14 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
hidden={ this.props.hidden }
>
<div
role='menu'
role={ shouldFocusOnContainer ? 'menu' : undefined }
Copy link
Member

@ThomasMichon ThomasMichon May 2, 2018

Choose a reason for hiding this comment

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

Why should the role change based on whether or not it is focusable? What gets labeled as a menu in the other case?

Copy link
Author

@rebeccaballantyne rebeccaballantyne May 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So when focus is NOT on the container (and is instead on the first menu item), adding role='menu' to the container stopped Narrator from reading the aria-label on the menu. Role='group' was fine but so was no role at all for narration behavior. Currently nothing gets labeled as a menu in the case where focus is on the first menu item (and narration works optimally). Is it a big accessibility no-no to not have role='menu' somewhere on a menu?

aria-label={ ariaLabel }
aria-labelledby={ labelElementId }
style={ contextMenuStyle }
ref={ (host: HTMLDivElement) => this._host = host }
id={ id }
className={ this._classNames.container }
tabIndex={ 0 }
tabIndex={ shouldFocusOnContainer ? 0 : undefined }
Copy link
Contributor

@chang47 chang47 May 3, 2018

Choose a reason for hiding this comment

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

For my understanding, this is the part of the code where we prevent focus being added to the container for the contextual menu?

Copy link
Author

@rebeccaballantyne rebeccaballantyne May 3, 2018

Choose a reason for hiding this comment

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

Yes.

onKeyDown={ this._onMenuKeyDown }
>
{ title && <div className={ this._classNames.title } role='heading' aria-level={ 1 }> { title } </div> }
Expand All @@ -295,7 +297,6 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
handleTabKey={ FocusZoneTabbableElements.all }
>
<ul
role='presentation'
Copy link
Contributor

@chang47 chang47 May 2, 2018

Choose a reason for hiding this comment

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

Why are we getting rid of the role?

At the very least shouldn't it be something like:

role = { shouldFocusOnContainer ? 'presentation' : undefined }

to match with your changes above.

Copy link
Author

@rebeccaballantyne rebeccaballantyne May 3, 2018

Choose a reason for hiding this comment

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

This role='presentation' was useless and it's presence didn't impact screen reader behavior at all.

className={ this._classNames.list }
onKeyDown={ this._onKeyDown }
>
Expand Down Expand Up @@ -814,6 +815,9 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
ev.stopPropagation();
this._enterTimerId = this._async.setTimeout(() => {
targetElement.focus();
this.setState({
expandedByMouseClick: true
});
this._onItemSubMenuExpand(item, targetElement);
this._enterTimerId = undefined;
}, NavigationIdleDelay);
Expand Down Expand Up @@ -849,6 +853,12 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
if (item.key === this.state.expandedMenuItemKey) { // This has an expanded sub menu. collapse it.
this._onSubMenuDismiss(ev);
} else { // This has a collapsed sub menu. Expand it.
this.setState({
// When Edge + Narrator are used together (regardless of if the button is in a form or not), pressing
// "Enter" fires this method and not _onMenuKeyDown. Checking ev.nativeEvent.detail differentiates
// between a real click event and a keypress event.
expandedByMouseClick: (ev.nativeEvent.detail !== 0)
});
this._onItemSubMenuExpand(item, target);
}
}
Expand Down Expand Up @@ -878,9 +888,17 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}

private _onItemKeyDown = (item: any, ev: React.KeyboardEvent<HTMLElement>): void => {
const openKey = getRTL() ? KeyCodes.left : KeyCodes.right;
let openKey;
if (item.split) {
openKey = getRTL() ? KeyCodes.left : KeyCodes.right;
} else {
openKey = KeyCodes.enter;
}

if (ev.which === openKey && !item.disabled) {
this.setState({
expandedByMouseClick: false
});
this._onItemSubMenuExpand(item, ev.currentTarget as HTMLElement);
ev.preventDefault();
}
Expand Down Expand Up @@ -924,6 +942,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
isSubMenu: true,
id: this.state.subMenuId,
shouldFocusOnMount: true,
shouldFocusOnContainer: this.state.expandedByMouseClick,
directionalHint: getRTL() ? DirectionalHint.leftTopEdge : DirectionalHint.rightTopEdge,
className: this.props.className,
gapSpace: 0,
Expand Down
6 changes: 6 additions & 0 deletions packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,12 @@ export interface IContextualMenuProps extends React.Props<ContextualMenu>, IWith
*/
shouldFocusOnMount?: boolean;

/**
* Whether to focus on the contextual menu container (as opposed to the first menu item).
* @default null
*/
shouldFocusOnContainer?: boolean;

/**
* Callback when the ContextualMenu tries to close. If dismissAll is true then all
* submenus will be dismissed.
Expand Down