Removing ability to focus with keyboarding on menu buttons for split buttons and and spin boxes #4222
There are no files selected for viewing
| 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]" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 || this._isSplitButton) ? false : true, | ||
| 'aria-pressed': checked | ||
| } | ||
| ); | ||
|
|
@@ -200,7 +200,9 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| } | ||
|
|
||
| public focus(): void { | ||
| if (this._isSplitButton && this._splitButtonContainer.value) { | ||
| this._splitButtonContainer.value.focus(); | ||
| } else if (this._buttonElement.value) { | ||
| this._buttonElement.value.focus(); | ||
| } | ||
| } | ||
|
|
@@ -423,7 +425,14 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| !!this.state.menuProps, | ||
| !!checked); | ||
|
|
||
| assign( | ||
| buttonProps, | ||
| { | ||
| onClick: undefined, | ||
| tabIndex: -1, | ||
| 'data-is-focusable': false | ||
| } | ||
| ); | ||
|
|
||
| return ( | ||
| <div | ||
|
|
@@ -435,10 +444,11 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| aria-pressed={ this.props.checked } | ||
| aria-describedby={ buttonProps.ariaDescription } | ||
| className={ classNames && classNames.splitButtonContainer } | ||
| onKeyDown={ this._onSplitButtonContainerKeyDown } | ||
| ref={ this._splitButtonContainer } | ||
| data-is-focusable={ true } | ||
| onClick={ !disabled && !primaryDisabled ? onClick : undefined } | ||
| tabIndex={ 0 } | ||
| > | ||
| <span | ||
| style={ { 'display': 'flex' } } | ||
|
|
@@ -482,11 +492,11 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
| 'iconProps': menuIconProps, | ||
| 'ariaLabel': splitButtonAriaLabel, | ||
| 'aria-haspopup': true, | ||
| 'aria-expanded': this._isExpanded, | ||
| 'data-is-focusable': false | ||
| }; | ||
|
|
||
| return <BaseButton {...splitButtonProps} onMouseDown={ this._onMouseDown } tabIndex={ -1 } />; | ||
|
Member
There was a problem hiding this comment. Adding Plus, if something should not be focusable, it shouldn't be rendered as a button. My preference here is to convert the menu click target to a DIV or SPAN which can soak clicks and touches, but not receive focus, and not attempt to render as a BaseButton.
Contributor
There was a problem hiding this comment. Setting the tabIndex=-1 and data-is-focusable=false ensures that the button will not be keyboard focusable when it is or is not inside of a FocusZone. As for if the individual portions of the splitButton should be buttons, I'm working with Narrator on defining a splitButton so that they will always be able to tell that it is in fact a split button. One approach may be a button with x, y, z markup and the fact that it contains two buttons... We could change them to be divs or spans if we want, the only downside of that is now we lose the consistency when the splitButton is rendered in a ContextualMenu. We we want to update that as well then we'll lose the ability to use the default render functionality
Contributor
There was a problem hiding this comment. That being said, I just noticed the span wrapping the splitButton contents here does not have aria-hidden="true" (which it should to align with how splitButtons are represented in ContextualMenus)
Contributor
There was a problem hiding this comment. Chatted with @dzearing and we should go ahead and simplify this to not be created with two buttons
Member
There was a problem hiding this comment. Talking with @jspurlin offline, we were discussing the idea of just removing the wrapping span. now that there is a single hittarget, there is no need for the wrapping span, which would reduce dom (and bundle size, and work from focus zone).
Contributor
Author
There was a problem hiding this comment. @dzearing I've spent a couple of hours trying to make the fix, but there were some not so simple styles that have to be made. Considering that the PR is to add focus on split button containers and some other components, I think that's outside of the scope of this specific issue. However, I have logged a bug for this issue at VSTS: 2209061 and will take a 2nd crack at it some other time. |
||
| } | ||
|
|
||
| private _onMouseDown = (ev: React.MouseEvent<BaseButton>) => { | ||
|
|
@@ -497,7 +507,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); | ||
| } | ||
|
|
@@ -509,13 +535,26 @@ export class BaseButton extends BaseComponent<IBaseButtonProps, IBaseButtonState | |
|
|
||
|
Contributor
There was a problem hiding this comment. It seems strange that this calls onMenuClick (considering that this is keydown)... If this want to call onMenuClick, why isn't this calling _onMenuClick?
Contributor
Author
There was a problem hiding this comment. 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?
Contributor
There was a problem hiding this comment. I think as long as _onToggleMenu is updating the focus correctly this is fine
Contributor
Author
There was a problem hiding this comment. This is resolved, we'll keep this code here and add the focus change to onToggleMenu |
||
| if (!ev.defaultPrevented && | ||
| this.props.menuTriggerKeyCode !== null && | ||
| this._isValidMenuOpenKey(ev)) { | ||
| this._onToggleMenu(); | ||
|
Contributor
There was a problem hiding this comment. 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
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. 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
Contributor
Author
There was a problem hiding this comment. 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; | ||
| } | ||
| } | ||
|
|
||
| private _onMenuClick = (ev: React.MouseEvent<HTMLAnchorElement>) => { | ||
| const { onMenuClick } = this.props; | ||
| if (onMenuClick) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -591,18 +591,21 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
|
|
||
| return ( | ||
| <div | ||
| role={ 'button' } | ||
| aria-labelledby={ item.ariaLabel } | ||
| className={ classNames.splitContainer } | ||
| aria-disabled={ this._isItemDisabled(item) } | ||
| aria-haspopup={ true } | ||
| aria-describedby={ item.ariaDescription } | ||
| aria-checked={ item.isChecked || item.checked } | ||
| aria-posinset={ focusableElementIndex + 1 } | ||
| aria-setsize={ totalItemCount } | ||
| onKeyDown={ this._onSplitContainerItemKeyDown.bind(this, item) } | ||
| onClick={ this._executeItemClick.bind(this, item) } | ||
| tabIndex={ 0 } | ||
| data-is-focusable={ true } | ||
| > | ||
| <span aria-hidden={ true }> | ||
|
||
| { this._renderSplitPrimaryButton(item, classNames, index, hasCheckmarks!, hasIcons!) } | ||
| { this._renderSplitDivider(item) } | ||
| { this._renderSplitIconButton(item, classNames, index) } | ||
|
|
@@ -620,7 +623,6 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
|
|
||
| const itemProps = { | ||
| key: item.key, | ||
| disabled: this._isItemDisabled(item) || item.primaryDisabled, | ||
| name: item.name, | ||
| className: classNames.splitPrimary, | ||
|
|
@@ -629,14 +631,25 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
| isChecked: item.isChecked, | ||
| checked: item.checked, | ||
| icon: item.icon, | ||
| iconProps: item.iconProps, | ||
| 'data-is-focusable': false | ||
| } as IContextualMenuItem; | ||
| return React.createElement('button', | ||
| getNativeProps(itemProps, buttonProperties), | ||
| <ChildrenRenderer item={ itemProps } classNames={ classNames } index={ index } onCheckmarkClick={ hasCheckmarks ? this._onItemClick : undefined } hasIcons={ hasIcons } />, | ||
| ); | ||
| } | ||
|
|
||
| private _onSplitContainerItemKeyDown(item: any, ev: React.KeyboardEvent<HTMLElement>) { | ||
| if (ev.which === KeyCodes.enter) { | ||
| this._executeItemClick(item, ev); | ||
| ev.preventDefault(); | ||
| ev.stopPropagation(); | ||
| } else { | ||
| this._onItemKeyDown(item, ev); | ||
|
Contributor
There was a problem hiding this comment. I'm pretty sure you don't need this else as the event will continue to propagate to the next event handler.
Contributor
Author
There was a problem hiding this comment. That's an interesting point to discuss, if we press enter, on a button, that's the equivalent of pressing the onClick event, but should we also try to handle the onKeyDown event too? @jspurlin what do you think?
Contributor
There was a problem hiding this comment. @chang47 you are correct, in this case (handling enter) we want to tie that with executeItemClick and not also fire onItemKeyDown
Contributor
There was a problem hiding this comment. In the if statement you should prevent default and stop propagation, since it will propagate up to the keyhandler that's on the root unless you stop propagation. |
||
| } | ||
| } | ||
|
|
||
| private _renderSplitIconButton(item: IContextualMenuItem, classNames: IMenuItemClassNames, index: number) { | ||
| const { contextualMenuItemAs: ChildrenRenderer = ContextualMenuItem } = this.props; | ||
| const itemProps = { | ||
|
|
@@ -650,11 +663,11 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
|
|
||
| return React.createElement('button', | ||
| assign({}, getNativeProps(itemProps, buttonProperties), { | ||
|
Contributor
There was a problem hiding this comment. Have splitButtons in menus been updated to behave the same way that splitButtons outside of menus behave? I think they will need to be updated at least to put focus on the container before the menu expands (so that focus will go back to the container when the menu collapses)
Contributor
Author
There was a problem hiding this comment. Style wise, it has the same hover affect, because I did use the getFocusStyle, unfortunately, I introduced a regression somewhere with the styles and it's not looking right. I'm looking back at how to fix it. As for putting the focus back, you're right that's something we should have. I'll add that in.
Contributor
Author
There was a problem hiding this comment. The base button changes should apply to the contextual menu for free. the only possible change might be when we open a sub menu, but I don't think that'll be an issue as we have to focus on the split button in the contextual menu to open it. In the end, after we close all the sub-menu we should put our focus back to the button that opened the contextual menu anyways.
Contributor
Author
There was a problem hiding this comment. I retract my earlier statement, like we discussed offline there was a case where if we keyboard on the menu and then hover over a split button menu and hit escape we would focus on the split button and not whole div. I've fixed that. |
||
| onMouseEnter: this._onItemMouseEnter.bind(this, item), | ||
| onMouseLeave: this._onMouseItemLeave.bind(this, item), | ||
| onMouseDown: (ev: any) => this._onItemMouseDown(item, ev), | ||
| onMouseMove: this._onItemMouseMove.bind(this, item), | ||
| 'data-is-focusable': false | ||
| }), | ||
| <ChildrenRenderer item={ itemProps } classNames={ classNames } index={ index } hasIcons={ false } /> | ||
| ); | ||
|
|
@@ -856,7 +869,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
| ev.stopPropagation(); | ||
| } | ||
|
|
||
| private _executeItemClick(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) { | ||
| if (item.disabled || item.isDisabled) { | ||
| return; | ||
| } | ||
|
|
@@ -870,10 +883,10 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext | |
| (dismiss || !ev.defaultPrevented) && this.dismiss(ev, true); | ||
| } | ||
|
|
||
| private _onItemKeyDown(item: any, ev: React.KeyboardEvent<HTMLElement>) { | ||
| const openKey = getRTL() ? KeyCodes.left : KeyCodes.right; | ||
|
|
||
| if (ev.which === openKey && !item.disabled) { | ||
| this._onItemSubMenuExpand(item, ev.currentTarget as HTMLElement); | ||
| ev.preventDefault(); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
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