-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Focus Zone: Allow Tab to Skip Selection #4061
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.
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
Changes from 14 commits
9efa1f3
65d118b
06282e1
29ca036
e493f66
6726d5b
a2b3691
3a53eb4
7b0413f
bb3ff0b
9da77fd
e478938
ef7dd1e
a8b146a
cffed1e
e2a1c59
def63ae
7cfcf8b
787fa4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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": "Focus Zone: Add support for tab to skip selection elements", | ||
| "type": "minor" | ||
| } | ||
| ], | ||
| "packageName": "office-ui-fabric-react", | ||
| "email": "chiechan@microsoft.com" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import * as React from 'react'; | ||
| import { | ||
| FocusZoneDirection, | ||
| FocusZoneTabbableElements, | ||
| IFocusZone, | ||
| IFocusZoneProps | ||
| } from './FocusZone.types'; | ||
|
|
@@ -63,6 +64,9 @@ export class FocusZone extends BaseComponent<IFocusZoneProps, {}> implements IFo | |
| private _focusAlignment: IPoint; | ||
| private _isInnerZone: boolean; | ||
|
|
||
| /** Used to allow us to move to next focusable element even when we're focusing on a input element when pressing tab */ | ||
| private _processingTabKey: boolean; | ||
|
|
||
| constructor(props: IFocusZoneProps) { | ||
| super(props); | ||
|
|
||
|
|
@@ -74,6 +78,8 @@ export class FocusZone extends BaseComponent<IFocusZoneProps, {}> implements IFo | |
| left: 0, | ||
| top: 0 | ||
| }; | ||
|
|
||
| this._processingTabKey = false; | ||
| } | ||
|
|
||
| public componentDidMount() { | ||
|
|
@@ -361,21 +367,18 @@ export class FocusZone extends BaseComponent<IFocusZoneProps, {}> implements IFo | |
| return; | ||
|
|
||
| case KeyCodes.tab: | ||
| if (this.props.allowTabKey) { | ||
| if (this.props.tabPermission === FocusZoneTabbableElements.all | ||
| || (this.props.tabPermission === FocusZoneTabbableElements.inputOnly && this._isElementInput(ev.target as HTMLElement))) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. put the || at the end of the previous line, it makes the if statement more readable |
||
| let focusChanged = false; | ||
| this._processingTabKey = true; | ||
| if (direction === FocusZoneDirection.vertical || | ||
| !this._shouldWrapFocus(this._activeElement as HTMLElement, NO_HORIZONTAL_WRAP)) { | ||
| if (ev.shiftKey) { | ||
| this._moveFocusUp(); | ||
| } else { | ||
| this._moveFocusDown(); | ||
| } | ||
| break; | ||
| focusChanged = ev.shiftKey ? this._moveFocusUp() : this._moveFocusDown(); | ||
| } else if (direction === FocusZoneDirection.horizontal || direction === FocusZoneDirection.bidirectional) { | ||
| if (ev.shiftKey) { | ||
| this._moveFocusLeft(); | ||
| } else { | ||
| this._moveFocusRight(); | ||
| } | ||
| focusChanged = ev.shiftKey ? this._moveFocusLeft() : this._moveFocusRight(); | ||
| } | ||
| this._processingTabKey = false; | ||
| if (focusChanged) { | ||
| break; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the code now only breaking if focusChanged? |
||
| } | ||
| } | ||
|
|
@@ -646,7 +649,7 @@ export class FocusZone extends BaseComponent<IFocusZoneProps, {}> implements IFo | |
| return distance; | ||
| }, | ||
| undefined /*ev*/, | ||
| (shouldWrap || !getRTL()) | ||
| shouldWrap | ||
| )) { | ||
| this._setFocusAlignment(this._activeElement as HTMLElement, true, false); | ||
| return true; | ||
|
|
@@ -674,7 +677,7 @@ export class FocusZone extends BaseComponent<IFocusZoneProps, {}> implements IFo | |
| return distance; | ||
| }, | ||
| undefined /*ev*/, | ||
| (shouldWrap || getRTL()) | ||
| shouldWrap | ||
| )) { | ||
| this._setFocusAlignment(this._activeElement as HTMLElement, true, false); | ||
| return true; | ||
|
|
@@ -787,7 +790,13 @@ export class FocusZone extends BaseComponent<IFocusZoneProps, {}> implements IFo | |
| } | ||
|
|
||
| private _shouldInputLoseFocus(element: HTMLInputElement, isForward?: boolean) { | ||
| if (element && | ||
| if (this.props.tabPermission === FocusZoneTabbableElements.inputOnly && !this._processingTabKey) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't know that the element is an input at this point, this check should be moved down to the if statement below.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't know if the element is an input element at this point. This needs to be moved down to the if statement below.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we should. The element we pass in is of type: HTMLInputElement However, I think it would make more sense to add it with the rest of the cases, so I'll move it down.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inputs can be of different types, that's why the check below is set up the way that it is |
||
| return false; | ||
| } | ||
|
|
||
| // If a tab was used, we want to focus on the next element. | ||
| if (!this._processingTabKey && | ||
| element && | ||
| element.type && | ||
| ALLOWED_INPUT_TYPES.indexOf(element.type.toLowerCase()) > -1) { | ||
| const selectionStart = element.selectionStart; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,7 +109,7 @@ export interface IFocusZoneProps extends React.HTMLAttributes<HTMLElement | Focu | |
| * an unfortunate side effect is that users will not be able to tab out of the focus zone | ||
| * and have to hit escape or some other key. | ||
| */ | ||
| allowTabKey?: boolean; | ||
| tabPermission?: FocusZoneTabbableElements; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's go ahead and deprecate
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider updating the prop name, maybe |
||
|
|
||
| /** | ||
| * Whether the to check for data-no-horizontal-wrap or data-no-vertical-wrap attributes | ||
|
|
@@ -119,6 +119,18 @@ export interface IFocusZoneProps extends React.HTMLAttributes<HTMLElement | Focu | |
| checkForNoWrap?: boolean; | ||
| } | ||
|
|
||
| export enum FocusZoneTabbableElements { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| /** Tabbing is not allowed */ | ||
| none = 0, | ||
|
|
||
| /** All tabbing action is allowed */ | ||
| all = 1, | ||
|
|
||
| /** Tabbing is allowed only on input elements */ | ||
| inputOnly = 2 | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you update the prop name to something like |
||
|
|
||
| export enum FocusZoneDirection { | ||
| /** Only react to up/down arrows. */ | ||
| vertical = 0, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| :global { | ||
| .ms-FocusZoneTabbableExample .ms-Row { | ||
| display: block; | ||
| margin: 5px; | ||
| } | ||
|
|
||
| .ms-FocusZoneTabbableExample-textField { | ||
| display: inline-block; | ||
| width: 300px; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* tslint:disable:no-unused-variable */ | ||
| import * as React from 'react'; | ||
| /* tslint:enable:no-unused-variable */ | ||
|
|
||
| import { DefaultButton } from 'office-ui-fabric-react/lib/Button'; | ||
| import { FocusZone, FocusZoneDirection, FocusZoneTabbableElements } from 'office-ui-fabric-react/lib/FocusZone'; | ||
| import { TextField } from 'office-ui-fabric-react/lib/TextField'; | ||
| import './FocusZone.Tabbable.Example.scss'; | ||
|
|
||
| export const FocusZoneTabbableExample = () => ( | ||
| <div className='ms-FocusZoneTabbableExample'> | ||
| <div className='ms-Row'> | ||
| <FocusZone direction={ FocusZoneDirection.horizontal } tabPermission={ FocusZoneTabbableElements.all } isCircularNavigation={ true }> | ||
| <span>Circular Tabbable FocusZone: </span> | ||
| <DefaultButton>Button 1</DefaultButton> | ||
| <DefaultButton>Button 2</DefaultButton> | ||
| <TextField value='FocusZone TextField' className='ms-FocusZoneTabbableExample-textField' /> | ||
| <DefaultButton>Button 3</DefaultButton> | ||
| </FocusZone> | ||
| </div> | ||
| <div className='ms-Row'> | ||
| <FocusZone direction={ FocusZoneDirection.horizontal } tabPermission={ FocusZoneTabbableElements.inputOnly } isCircularNavigation={ false }> | ||
| <span>Input Only FocusZone: </span> | ||
| <DefaultButton>Button 1</DefaultButton> | ||
| <DefaultButton>Button 2</DefaultButton> | ||
| <TextField value='FocusZone TextField' className='ms-FocusZoneTabbableExample-textField' /> | ||
| <DefaultButton>Button 3</DefaultButton> | ||
| </FocusZone> | ||
| </div> | ||
| </div> | ||
| ); |
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.
You should add a new test case for inputOnly