Focus Zone: Allow Tab to Skip Selection #4061

chang47 · 2018-02-21T22:17:53Z

Problem:
If tabbing is enabled, we have a problem where we can't tab through input html elements and we would be stuck on the input button

Solution:
The reason for this problem is, because when we're trying to switch focus on an input element, we would stop the focus. For arrow keys that would be the correct thing to do, however if we're tabbing, we want to ignore that and focus on the next element.

I also changed the way we handle tab to be more like arrow keys where we only stop if we change focus otherwise we'll let the tab event to continue to propagate.

Validation:
I added a new example component that allows tabbing and circular navigation along with an text input so that we can more easily test to see if have a regression in the focus zone where we would get stuck an an input element.

Here is what it is now:

Here is what it was before where it get stuck:

jspurlin · 2018-02-21T23:42:33Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx

  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 _tabPressActive: boolean;


maybe a better name is _processingTabKey or something like that since this flag is for when the tab key press is being processed

jspurlin · 2018-02-21T23:44:59Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx

+            }
+            this._tabPressActive = false;
+            if (focusChanged) {
              break;


Why is the code now only breaking if focusChanged?

jspurlin · 2018-02-21T23:47:17Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx


    if (this._isElementInput(element)) {
-      if (!this._shouldInputLoseFocus(element as HTMLInputElement, isForward)) {
+      if (!this._tabPressActive && !this._shouldInputLoseFocus(element as HTMLInputElement, isForward)) {


Consider moving this check into shouldInputLoseFocus since it's directly related to if the input should lose focus when tabbing

jspurlin

A few minor changes are needed but looks good overall

jspurlin · 2018-02-22T14:45:06Z

Pull the latest changes from master, this PR will need to be updated with the changes made in #3962

…t elements

jspurlin · 2018-02-22T23:47:59Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.types.ts

   * and have to hit escape or some other key.
   */
-  allowTabKey?: boolean;
+  tabPermission?: FocusZoneTabbableElements;


Let's go ahead and deprecate allowTabKey instead of removing it. Remove all references to it (setting the value to tabPermission if it is passed in), put at @deprecated in the jsdoc comment, and add a _warnDeprecations for that prop referencing tabPermission

Consider updating the prop name, maybe handleTabKey

joschect · 2018-02-22T23:47:35Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.types.ts

  checkForNoWrap?: boolean;
 }

+export enum FocusZoneTabbableElements {


export const enum ... it will help keep size down.

joschect · 2018-02-22T23:50:48Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx

  }

  private _shouldInputLoseFocus(element: HTMLInputElement, isForward?: boolean) {
+    if (this.props.tabPermission === FocusZoneTabbableElements.inputOnly && !this._processingTabKey) {


You don't know that the element is an input at this point, this check should be moved down to the if statement below.

joschect · 2018-02-22T23:59:55Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx


  private _shouldInputLoseFocus(element: HTMLInputElement, isForward?: boolean) {
-    if (element &&
+    if (this.props.tabPermission === FocusZoneTabbableElements.inputOnly && !this._processingTabKey) {


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.

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.

Inputs can be of different types, that's why the check below is set up the way that it is

jspurlin · 2018-02-23T00:03:26Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.types.ts

+
+  // allowTabKeyOnInput is true and allowTabKey is false
+  inputOnly = 2
+}


If you update the prop name to something like handleTabKey the enum name and string values should be updated... for the values maybe never = 0, always= 1, inputOnly = 2

jspurlin · 2018-02-23T00:09:25Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.test.tsx

    const component = ReactTestUtils.renderIntoDocument(
      <div { ...{ onFocusCapture: _onFocus, onKeyDown: tabDownListener } }>
-        <FocusZone {...{ allowTabKey: true, isCircularNavigation: true }}>
+        <FocusZone {...{ tabPermission: FocusZoneTabbableElements.all, isCircularNavigation: true }}>


You should add a new test case for inputOnly

jspurlin · 2018-02-23T00:18:30Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx

        case KeyCodes.tab:
-          if (this.props.allowTabKey) {
+          if (this.props.tabPermission === FocusZoneTabbableElements.all
+            || (this.props.tabPermission === FocusZoneTabbableElements.inputOnly && this._isElementInput(ev.target as HTMLElement))) {


put the || at the end of the previous line, it makes the if statement more readable

jspurlin

some minor updates need to be made

jspurlin · 2018-02-23T01:46:58Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.test.tsx

        <FocusZone>
          <button className='a'>a</button>
-        </FocusZone>
+        </FocusZone>g


Did you mean to have this 'g' there?

jspurlin

Looks good

joschect · 2018-02-23T02:14:51Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx

-          if (this.props.allowTabKey) {
+          if (this.props.allowTabKey ||
+            this.props.handleTabKey === FocusZoneTabbableElements.all ||
+            (this.props.handleTabKey === FocusZoneTabbableElements.inputOnly && this._isElementInput(ev.target as HTMLElement))) {


Put this onto multiple lines

* master: (28 commits) Scrollable pane sort stickies (microsoft#4111) Allow ScrollablePane to accept native properties. (microsoft#4095) Sticky (microsoft#4091) Applying package updates. [ColorRectangle, Sticky] Fixed null root refs (microsoft#4099) DatePicker: order of callbacks for onSelectDate and onAfterMenuDismiss (microsoft#4092) Applying package updates. Alhenry fix split button props (microsoft#4090) Fixing ComboBox styling by reverting button classname move (microsoft#4088) Update CODEOWNERS Undoing terrible change. [DetailsList] Fixed focus test (microsoft#4087) Added icons package screener test (microsoft#4082) ContextualMenu: Fix ContextualMenuUtility imports (microsoft#4085) [ContextualMenu] Made disabled buttons focusable (microsoft#4074) Convert Check to mergeStyles (microsoft#3880) [DetailsList] Add public focusIndex function (microsoft#3852) ComboBox button should have data-is-focusable="false" (microsoft#4070) Applying package updates. Focus Zone: Allow Tab to Skip Selection (microsoft#4061) ...

REDMOND\chiechan added 7 commits February 20, 2018 12:53

added return for focus zone when focus change fail

9efa1f3

add tab override for focus zone

65d118b

added support for skipping selection with tab

06282e1

add change files

29ca036

added tabbable focus zone example

e493f66

got rid of unnecessary return

6726d5b

used member instead of passing varaables down

a2b3691

jspurlin reviewed Feb 21, 2018

View reviewed changes

REDMOND\chiechan added 3 commits February 21, 2018 15:57

changed tab field name; moved tab location

3a53eb4

removed duplicate component; fixed lint problems

7b0413f

added actual tabbable example scss to fix build crash

bb3ff0b

REDMOND\chiechan added 2 commits February 22, 2018 13:05

resolved merge conflicts

9da77fd

Adding FocusZoneTabbableElements enum

e478938

dzearing assigned jspurlin Feb 22, 2018

add new input only mode that allows skipping of focus on tab for inpu…

ef7dd1e

…t elements

chang47 requested a review from joschect as a code owner February 22, 2018 23:39

cleaned things up and added better comments

a8b146a

jspurlin reviewed Feb 22, 2018

View reviewed changes

joschect reviewed Feb 22, 2018

View reviewed changes

jspurlin reviewed Feb 23, 2018

View reviewed changes

updated pr for everything except test

cffed1e

jspurlin reviewed Feb 23, 2018

View reviewed changes

added unit test

e2a1c59

jspurlin approved these changes Feb 23, 2018

View reviewed changes

optimized if statement; cleaned up test

def63ae

joschect reviewed Feb 23, 2018

View reviewed changes

joschect approved these changes Feb 23, 2018

View reviewed changes

REDMOND\chiechan added 2 commits February 22, 2018 18:19

separated ampersand for if statement

7cfcf8b

update all lint probelms

787fa4a

jspurlin merged commit 1de56b1 into microsoft:master Feb 23, 2018

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Focus Zone: Allow Tab to Skip Selection #4061

Focus Zone: Allow Tab to Skip Selection #4061

Uh oh!

Conversation

chang47 commented Feb 21, 2018

Uh oh!

jspurlin Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspurlin left a comment

Choose a reason for hiding this comment

Uh oh!

jspurlin commented Feb 22, 2018

Uh oh!

jspurlin Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspurlin Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspurlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspurlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jspurlin Feb 21, 2018 •

edited

Loading

jspurlin Feb 22, 2018 •

edited

Loading

jspurlin Feb 22, 2018 •

edited

Loading