Skip to content

Removing ability to focus with keyboarding on menu buttons for split buttons and and spin boxes #4222

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 33 commits into from
Mar 23, 2018
Merged

Removing ability to focus with keyboarding on menu buttons for split buttons and and spin boxes #4222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
2ffef62
made spin button and base button menu option un-focusable
Mar 8, 2018
979da51
added spin button test
Mar 9, 2018
e4ab4b4
added split button contextual menu change
Mar 9, 2018
3d95aca
got rid of un-needed code
Mar 9, 2018
64bc57b
added change files
Mar 9, 2018
e08bf24
added abillitiy to not focus on primary button for split button, but …
Mar 9, 2018
c16992b
enter on focus on split button trigger primary action
Mar 9, 2018
7bea2c0
added split button style for disabled
Mar 9, 2018
62cb458
prevent opening submenu if item is disabled
Mar 9, 2018
efb7439
tighten restrictions on opening split buttons to open with alt + down
Mar 9, 2018
f803332
refactored button name and fixed test
Mar 10, 2018
b2a4475
updated bundle size
Mar 13, 2018
b1f2c39
Merge branch 'master' into focus2
chang47 Mar 13, 2018
976c67f
changed on menu item click to bw able to take in a keyboard event
Mar 13, 2018
d3e1426
Merge branch 'focus2' of https://github.com/chang47/office-ui-fabric-…
Mar 13, 2018
0aaefaa
updated package json
Mar 13, 2018
bbc370f
fixed weird tabbing
Mar 13, 2018
0fc2cc6
changed way we structured the focus
Mar 13, 2018
965a8e9
added stop propagation
Mar 14, 2018
2603277
resolved merge conflicts
Mar 15, 2018
aaa77c3
resolved merge conflict
Mar 16, 2018
ad6e5df
changed split button container key down to work with other buttons now
Mar 16, 2018
e01436a
update bundle size
Mar 16, 2018
a33679e
added correct split button container styles
Mar 21, 2018
93ba4ae
removed unnecessary attribute
Mar 21, 2018
d19e1a6
fixed focusing on menu button when primary button is disabled
Mar 21, 2018
24ae278
added change to focus
Mar 21, 2018
0e7d887
resolved merge conflict
Mar 21, 2018
619711d
added aria hidden
Mar 21, 2018
276fe96
fixed span location and moved TODO
Mar 21, 2018
d4436d4
added support to focus on whole container in split button for context…
Mar 22, 2018
843d758
added comment
Mar 22, 2018
9186034
fixed styling issues with buttons
Mar 23, 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/focus2_2018-03-09-17-45.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": "Removed focusability on buttons for split buttons and spin buttons",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "[email protected]"
}
60 changes: 51 additions & 9 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 @@ -163,7 +163,7 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
'aria-label': ariaLabel,
'aria-labelledby': ariaLabelledBy,
'aria-describedby': ariaDescribedBy,
'data-is-focusable': ((this.props as any)['data-is-focusable'] === false || disabled) ? false : true,
'data-is-focusable': ((this.props as any)['data-is-focusable'] === false || disabled || this._isSplitButton) ? false : true,
'aria-pressed': checked
}
);
Expand Down Expand Up @@ -200,7 +200,9 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
}

public focus(): void {
if (this._buttonElement.value) {
if (this._isSplitButton && this._splitButtonContainer.value) {
this._splitButtonContainer.value.focus();
} else if (this._buttonElement.value) {
this._buttonElement.value.focus();
}
}
Expand Down Expand Up @@ -409,6 +411,10 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
}

private _onToggleMenu = (): void => {
if (this._splitButtonContainer.value) {
this._splitButtonContainer.value.focus();
}
const { menuProps } = this.props;
const currentMenuProps = this.state.menuProps;
currentMenuProps ? this._dismissMenu() : this._openMenu();
}
Expand All @@ -432,7 +438,14 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
!!this.state.menuProps,
!!checked);

buttonProps = { ...buttonProps, onClick: undefined };
assign(
buttonProps,
{
onClick: undefined,
tabIndex: -1,
Copy link
Member

@dzearing dzearing Mar 20, 2018

Choose a reason for hiding this comment

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

Tabindex -1 still seems like a bad idea

'data-is-focusable': false
}
);

return (
<div
Expand All @@ -444,10 +457,11 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
aria-pressed={ this.props.checked }
aria-describedby={ buttonProps.ariaDescription }
className={ classNames && classNames.splitButtonContainer }
onKeyDown={ this._onMenuKeyDown }
onKeyDown={ this._onSplitButtonContainerKeyDown }
ref={ this._splitButtonContainer }
data-is-focusable={ true }
onClick={ !disabled && !primaryDisabled ? onClick : undefined }
tabIndex={ !disabled ? 0 : undefined }
>
<span
style={ { 'display': 'flex' } }
Expand Down Expand Up @@ -491,11 +505,10 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
'iconProps': menuIconProps,
'ariaLabel': splitButtonAriaLabel,
'aria-haspopup': true,
'aria-expanded': this._isExpanded
'aria-expanded': this._isExpanded,
'data-is-focusable': false
};

return <BaseButton { ...splitButtonProps } onMouseDown={ this._onMouseDown } />;

return <BaseButton {...splitButtonProps} onMouseDown={ this._onMouseDown } tabIndex={ -1 } />;
}

private _onMouseDown = (ev: React.MouseEvent<BaseButton>) => {
Expand All @@ -506,7 +519,23 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState
ev.preventDefault();
}

private _onSplitButtonContainerKeyDown = (ev: React.KeyboardEvent<HTMLDivElement>) => {
if (ev.which === KeyCodes.enter) {
if (this._buttonElement.value) {
this._buttonElement.value.click();
ev.preventDefault();
ev.stopPropagation();
}
} else {
this._onMenuKeyDown(ev);
}
}

private _onMenuKeyDown = (ev: React.KeyboardEvent<HTMLDivElement | HTMLAnchorElement | HTMLButtonElement>) => {
if (this.props.disabled) {
return;
}

if (this.props.onKeyDown) {
this.props.onKeyDown(ev);
}
Expand All @@ -518,13 +547,26 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState

Copy link
Contributor

@jspurlin jspurlin Mar 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It seems strange that this calls onMenuClick (considering that this is keydown)... If this want to call onMenuClick, why isn't this calling _onMenuClick?

Copy link
Contributor Author

@chang47 chang47 Mar 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We want to trigger the primary action when we hit the enter button if we're focusing on the html element. This works for buttons, unfortunately, the container is a div so pressing enter won't trigger the onClick event.

I can move the code in onMenuClick and call it in both places, but they would both also reference onClick events. What do you think?

Copy link
Contributor

@jspurlin jspurlin Mar 21, 2018

Choose a reason for hiding this comment

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

I think as long as _onToggleMenu is updating the focus correctly this is fine

Copy link
Contributor Author

@chang47 chang47 Mar 21, 2018

Choose a reason for hiding this comment

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

This is resolved, we'll keep this code here and add the focus change to onToggleMenu

if (!ev.defaultPrevented &&
this.props.menuTriggerKeyCode !== null &&
ev.which === (this.props.menuTriggerKeyCode === undefined ? KeyCodes.down : this.props.menuTriggerKeyCode)) {
this._isValidMenuOpenKey(ev)) {
this._onToggleMenu();
Copy link
Contributor

@jspurlin jspurlin Mar 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

as part of _onToggleMenu could we set focus to the container before setting the state to toggle the menu? This would allow focus to return to the whole split button once the menu is dismissed

Copy link
Contributor Author

@chang47 chang47 Mar 21, 2018

Choose a reason for hiding this comment

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

Would that be what we want to do? By doing this we would force the focus to go back to the menu in every scenario.

From my understanding, we want focus to go back to the previous focused element, wIth the suggested change, we always focus on the button when we close the menu.

Copy link
Contributor

@jspurlin jspurlin Mar 21, 2018

Choose a reason for hiding this comment

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

The focus should always only be on the container (even if it was fired against the split button portion). We should never be setting focus on the individual portions of the splitButton

Copy link
Contributor Author

@chang47 chang47 Mar 21, 2018

Choose a reason for hiding this comment

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

Added the changes here. Now we only don't focus the split button if everything is disabled.

ev.preventDefault();
ev.stopPropagation();
}
}

/**
* Returns if the user hits a valid keyboard key to open the menu
* @param ev - the keyboard event
* @returns True if user clicks on custom trigger key if enabled or alt + down arrow if not. False otherwise.
*/
private _isValidMenuOpenKey(ev: React.KeyboardEvent<HTMLDivElement | HTMLAnchorElement | HTMLButtonElement>): boolean {
if (this.props.menuTriggerKeyCode) {
return ev.which === this.props.menuTriggerKeyCode;
} else {
return ev.which === KeyCodes.down && (ev.altKey || ev.metaKey);
}
}

private _onMenuClick = (ev: React.MouseEvent<HTMLAnchorElement>) => {
const { onMenuClick } = this.props;
if (onMenuClick) {
Expand Down
5 changes: 3 additions & 2 deletions packages/office-ui-fabric-react/src/components/Button/Button.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ describe('Button', () => {
expect(didClick).toEqual(true);
});

it('Pressing down on SplitButton triggers menu', () => {
it('Pressing alt + down on SplitButton triggers menu', () => {
const renderedDOM: HTMLElement = renderIntoDocument(
<DefaultButton
data-automation-id='test'
Expand Down Expand Up @@ -519,7 +519,8 @@ describe('Button', () => {

ReactTestUtils.Simulate.keyDown(menuButtonElement,
{
which: KeyCodes.down
which: KeyCodes.down,
altKey: true
});
expect(renderedDOM.getAttribute('aria-expanded')).toEqual('true');
});
Expand Down
25 changes: 14 additions & 11 deletions packages/office-ui-fabric-react/src/components/Button/SplitButton/SplitButton.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,21 @@ export const getStyles = memoizeFunction((
};

const splitButtonStyles: IButtonStyles = {
splitButtonContainer: {
position: 'relative',
display: 'inline-block',
border: '1px solid transparent'
},
splitButtonContainer: [
getFocusStyle(theme, 0, 'relative', buttonHighContrastFocus),
{
display: 'inline-block'
}
],
splitButtonContainerFocused: {
outline: 'none!important',
border: '1px solid'
},
splitButtonMenuButton: [
getFocusStyle(theme, -1, 'relative', buttonHighContrastFocus),
splitButtonMenuButton:
{
padding: 6,
height: 'auto',
boxSizing: 'border-box',
border: '1px solid transparent',
border: 0,
borderRadius: 0,
outline: 'transparent',
userSelect: 'none',
Expand All @@ -45,8 +44,7 @@ export const getStyles = memoizeFunction((
verticalAlign: 'top',
width: 32,
marginLeft: -1
}
],
},

splitButtonDivider: {
position: 'absolute',
Expand All @@ -72,6 +70,11 @@ export const getStyles = memoizeFunction((
justifyContent: 'center',
alignItems: 'center'
},

splitButtonContainerDisabled: {
outline: 'none',
border: 'none'
}
};

return concatStyleSets(splitButtonStyles, customStyles)!;
Expand Down
27 changes: 27 additions & 0 deletions packages/office-ui-fabric-react/src/components/Button/examples/Button.Split.Example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,33 @@ export class ButtonSplitExample extends React.Component<IButtonProps> {
} }
/>
</div>
<div>
<Label>Button Disabled</Label>
<DefaultButton
primary
data-automation-id='test'
disabled={ true }
checked={ checked }
text='Create account'
onClick={ alertClicked }
split={ true }
style={ { height: '35px' } }
menuProps={ {
items: [
{
key: 'emailMessage',
name: 'Email message',
icon: 'Mail'
},
{
key: 'calendarEvent',
name: 'Calendar event',
icon: 'Calendar'
}
]
} }
/>
</div>
</div>
);
}
Expand Down
9 changes: 8 additions & 1 deletion packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.classNames.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,13 @@ export const getItemClassNames = memoizeFunction((
'ms-ContextualMenu-itemText',
styles.label
],
splitContainer: styles.splitButtonFlexContainer,
splitContainer: [
styles.splitButtonFlexContainer,
!disabled && !checked && [{
selectors: {
'.ms-Fabric.is-focusVisible &:focus, .ms-Fabric.is-focusVisible &:focus:hover': styles.rootFocused,
}
}]
],
});
});
15 changes: 8 additions & 7 deletions packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,13 +168,14 @@ export const getMenuItemStyles = memoizeFunction((
flexShrink: '0',
fontSize: FontSizes.mini
},
splitButtonFlexContainer: {
display: 'flex',
height: ContextualMenuItemHeight,
flexWrap: 'nowrap',
justifyContent: 'center',
alignItems: 'center'
},
splitButtonFlexContainer: [
getFocusStyle(theme), {
display: 'flex',
height: ContextualMenuItemHeight,
flexWrap: 'nowrap',
justifyContent: 'center',
alignItems: 'center',
}],
splitButtonSeparator: {}
};

Expand Down
Loading