FocusTrapZone does not correctly trap focus when last child is FocusZ…

…one (microsoft#4172)

* When a FocusTrapZone has a FocusZone as a last child, focus escapes the trap zone when tabbing through. When calculating last tabbable element, consider the tabbable argument when navigating inside focus zones.

* Adding change files.

* Addressing comments.

common/changes/@uifabric/utilities/focustrapzone-last-child-focuszone_2018-03-04-09-07.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "@uifabric/utilities",
+      "comment": "Focus utility getPreviousElement did not correctly consider the tabbable argument when considering the current node. This can affect how FocusZones are processed, since only one element in a zone will have tab index set. This, in turn, affects how FocusTrapZone traps focus, since getPreviousElement is used during trapping focus.",
+      "type": "patch"
+    }
+  ],
+  "packageName": "@uifabric/utilities",
+  "email": "[email protected]"
+}

common/changes/office-ui-fabric-react/focustrapzone-last-child-focuszone_2018-03-04-09-07.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "packageName": "office-ui-fabric-react",
+      "comment": "Updating a test to ensure that focus is trapped in a FocusTrapZone when it has a FocusZone as the last element",
+      "type": "patch"
+    }
+  ],
+  "packageName": "office-ui-fabric-react",
+  "email": "[email protected]"
+}

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

@@ -239,4 +239,75 @@ describe('FocusTrapZone', () => {
     ReactTestUtils.Simulate.keyDown(buttonA, { which: KeyCodes.tab });
     expect(lastFocusedElement).toBe(buttonX);
   });
+
+  it('can tab across FocusZones with different button structures', () => {
+    const component = ReactTestUtils.renderIntoDocument(
+      <div { ...{ onFocusCapture: _onFocus } }>
+        <FocusTrapZone forceFocusInsideTrap={ false }>
+          <button className='a'>a</button>
+          <FocusZone direction={ FocusZoneDirection.horizontal } data-is-visible={ true }>
+            <button className='d'>d</button>
+            <button className='e'>e</button>
+            <button className='f'>f</button>
+          </FocusZone>
+        </FocusTrapZone>
+      </div>
+    );
+
+    const focusTrapZone = ReactDOM.findDOMNode(component as React.ReactInstance) as Element;
+
+    const buttonA = focusTrapZone.querySelector('.a') as HTMLElement;
+    const buttonD = focusTrapZone.querySelector('.d') as HTMLElement;
+    const buttonE = focusTrapZone.querySelector('.e') as HTMLElement;
+    const buttonF = focusTrapZone.querySelector('.f') as HTMLElement;
+
+    // Assign bounding locations to buttons.
+    setupElement(buttonA, {
+      clientRect: {
+        top: 0,
+        bottom: 30,
+        left: 0,
+        right: 30
+      }
+    });
+
+    setupElement(buttonD, {
+      clientRect: {
+        top: 30,
+        bottom: 60,
+        left: 0,
+        right: 30
+      }
+    });
+
+    setupElement(buttonE, {
+      clientRect: {
+        top: 30,
+        bottom: 60,
+        left: 30,
+        right: 60
+      }
+    });
+
+    setupElement(buttonF, {
+      clientRect: {
+        top: 30,
+        bottom: 60,
+        left: 60,
+        right: 90
+      }
+    });
+
+    // Focus the first button.
+    ReactTestUtils.Simulate.focus(buttonD);
+    expect(lastFocusedElement).toBe(buttonD);
+
+    // Pressing tab should go to a.
+    ReactTestUtils.Simulate.keyDown(buttonD, { which: KeyCodes.tab });
+    expect(lastFocusedElement).toBe(buttonA);
+
+    // Pressing shift + tab should go to a.
+    ReactTestUtils.Simulate.keyDown(buttonA, { which: KeyCodes.tab, shiftKey: true });
+    expect(lastFocusedElement).toBe(buttonD);
+  });
 });

packages/utilities/src/focus.ts

@@ -77,7 +77,7 @@ export function getPreviousElement(
   traverseChildren?: boolean,
   includeElementsInFocusZones?: boolean,
   allowFocusRoot?: boolean,
-  tabbable?: boolean): HTMLElement | null {
+  elementShouldBeTabbable?: boolean): HTMLElement | null {
 
   if (!currentElement ||
     (!allowFocusRoot && currentElement === rootElement)) {
@@ -97,10 +97,10 @@ export function getPreviousElement(
       true,
       includeElementsInFocusZones,
       allowFocusRoot,
-      tabbable);
+      elementShouldBeTabbable);
 
     if (childMatch) {
-      if ((tabbable && (isElementTabbable(childMatch, true))) || !tabbable) {
+      if ((elementShouldBeTabbable && isElementTabbable(childMatch, true)) || !elementShouldBeTabbable) {
         return childMatch;
       }
 
@@ -112,7 +112,7 @@ export function getPreviousElement(
         true,
         includeElementsInFocusZones,
         allowFocusRoot,
-        tabbable
+        elementShouldBeTabbable
       );
       if (childMatchSiblingMatch) {
         return childMatchSiblingMatch;
@@ -133,7 +133,7 @@ export function getPreviousElement(
           true,
           includeElementsInFocusZones,
           allowFocusRoot,
-          tabbable
+          elementShouldBeTabbable
         );
 
         if (childMatchParentMatch) {
@@ -146,7 +146,8 @@ export function getPreviousElement(
   }
 
   // Check the current node, if it's not the first traversal.
-  if (checkNode && isCurrentElementVisible && isElementTabbable(currentElement)) {
+  if (checkNode && isCurrentElementVisible &&
+    ((elementShouldBeTabbable && isElementTabbable(currentElement, true)) || !elementShouldBeTabbable)) {
     return currentElement;
   }
 
@@ -159,7 +160,7 @@ export function getPreviousElement(
     true,
     includeElementsInFocusZones,
     allowFocusRoot,
-    tabbable);
+    elementShouldBeTabbable);
 
   if (siblingMatch) {
     return siblingMatch;
@@ -168,7 +169,7 @@ export function getPreviousElement(
   // Check its parent.
   if (!suppressParentTraversal) {
     return getPreviousElement(rootElement, currentElement.parentElement, true, false, false, includeElementsInFocusZones,
-      allowFocusRoot, tabbable);
+      allowFocusRoot, elementShouldBeTabbable);
   }
 
   return null;