[#1724, #1741, #1805] Fix up focus trap for dialogs

CHANGELOG.md

@@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - [1826](https://github.com/microsoft/BotFramework-Emulator/pull/1826)
   - [1827](https://github.com/microsoft/BotFramework-Emulator/pull/1827)
   - [1828](https://github.com/microsoft/BotFramework-Emulator/pull/1828)
+  - [1829](https://github.com/microsoft/BotFramework-Emulator/pull/1829)
   - [1831](https://github.com/microsoft/BotFramework-Emulator/pull/1831)
   - [1832](https://github.com/microsoft/BotFramework-Emulator/pull/1832)
   - [1835](https://github.com/microsoft/BotFramework-Emulator/pull/1835)

packages/app/client/package.json

@@ -14,7 +14,7 @@
     "build": "run-s build:vendors build:shared build:app",
     "lint": "eslint --color --quiet --ext .js,.jsx,.ts,.tsx ./src",
     "lint:fix": "npm run lint -- --fix",
-    "start": "run-s build:vendors:dev build:shared:dev webpackdevServer:dev",
+    "start": "run-s build:shared:dev webpackdevServer:dev",
     "webpackdevServer:dev": "webpack-dev-server --mode development --hot --inline --progress --colors --content-base ./public",
     "test": "jest"
   },

packages/app/client/src/ui/dialogs/host/host.spec.tsx

@@ -78,7 +78,7 @@ describe('<DialogHost>', () => {
   it('should add an event listener on mount, and remove on unmount', () => {
     const mockAddEventListener = jest.fn(() => null);
     const mockRemoveEventLister = jest.fn(() => null);
-    instance._hostRef = {
+    instance.hostRef.current = {
       addEventListener: mockAddEventListener,
       removeEventListener: mockRemoveEventLister,
     };
@@ -108,11 +108,12 @@ describe('<DialogHost>', () => {
   });
 
   it('should save a host element reference', () => {
-    const mockElem = { name: 'I am a fake element!' };
-    instance.saveHostRef(mockElem);
+    const mockElem = { addEventListener: () => null, name: 'I am a fake element!' };
+    instance.hostRef.current = mockElem;
+    instance.componentDidMount();
 
     expect(mockSetHost).toHaveBeenCalledWith(mockElem);
-    expect(instance._hostRef).toBe(mockElem);
+    expect(instance.hostRef.current).toBe(mockElem);
   });
 
   it('should get all focusable elements in the modal', () => {
@@ -131,7 +132,7 @@ describe('<DialogHost>', () => {
     mockDialog.appendChild(mockSpan2);
     hostElement.append(mockDialog);
 
-    instance._hostRef = hostElement;
+    instance.hostRef.current = hostElement;
     const focusableElements = instance.getFocusableElementsInModal();
     expect(focusableElements.length).toBe(3);
   });
@@ -140,8 +141,8 @@ describe('<DialogHost>', () => {
     const mockFocus = jest.fn(() => null);
     const mockGetFocusableElementsInModal = jest.fn(() => {
       return [
-        { elem: 'elem1', focus: mockFocus }, // should be focused
-        { elem: 'elem2' },
+        { elem: 'elem1' }, // should be focused
+        { elem: 'elem2', focus: mockFocus },
         { elem: 'elem3' },
       ];
     });
@@ -158,7 +159,7 @@ describe('<DialogHost>', () => {
     const mockFocusDisabledElement = jest.fn(() => null);
     const mockGetFocusableElementsInModal = jest.fn(() => {
       return [
-        { elem: 'elem1' },
+        { elem: 'elem1', hasAttribute: () => true },
         {
           elem: 'elem2',
           hasAttribute: () => false,
@@ -171,7 +172,8 @@ describe('<DialogHost>', () => {
         },
       ];
     });
-    instance.getFocusableElementsInModal = mockGetFocusableElementsInModal;
+
+    instance.hostRef.current = { querySelectorAll: mockGetFocusableElementsInModal };
 
     instance.onFocusStartingSentinel(mockFocusEvent);
     expect(mockFocusEnabledElement).toHaveBeenCalled();
@@ -195,10 +197,11 @@ describe('<DialogHost>', () => {
           hasAttribute: () => false,
           focus: mockFocusEnabledElement,
         }, // should be focused
-        { elem: 'elem2' },
+        { elem: 'elem2', hasAttribute: () => true },
       ];
     });
-    instance.getFocusableElementsInModal = mockGetFocusableElementsInModal;
+
+    instance.hostRef.current = { querySelectorAll: mockGetFocusableElementsInModal };
 
     instance.onFocusEndingSentinel(mockFocusEvent);
     expect(mockFocusEnabledElement).toHaveBeenCalled();

packages/app/client/src/ui/dialogs/host/host.tsx

@@ -39,19 +39,25 @@ import { DialogService } from '../service';
 import * as styles from './host.scss';
 
 export interface DialogHostProps {
-  saveHostRef?: (elem: HTMLElement) => void;
   showing?: boolean;
 }
 
 export class DialogHost extends React.Component<DialogHostProps, {}> {
-  private _hostRef: HTMLElement;
+  private hostRef: React.RefObject<any>;
+
+  constructor(props) {
+    super(props);
+
+    this.hostRef = React.createRef();
+  }
 
   public componentDidMount() {
-    this._hostRef.addEventListener('dialogRendered', this.initFocusTrap);
+    this.hostRef.current && this.hostRef.current.addEventListener('dialogRendered', this.initFocusTrap);
+    DialogService.setHost(this.hostRef.current);
   }
 
   public componentWillUnmount() {
-    this._hostRef.removeEventListener('dialogRendered', this.initFocusTrap);
+    this.hostRef.current && this.hostRef.current.removeEventListener('dialogRendered', this.initFocusTrap);
   }
 
   public render() {
@@ -62,7 +68,7 @@ export class DialogHost extends React.Component<DialogHostProps, {}> {
     return (
       <div className={`${styles.host} ${visibilityClass}`} onClick={this.handleOverlayClick}>
         <span tabIndex={sentinelTabIndex} onFocus={this.onFocusStartingSentinel} className={styles.focusSentinel} />
-        <div className={styles.dialogHostContent} onClick={this.handleContentClick} ref={this.saveHostRef} />
+        <div className={styles.dialogHostContent} onClick={this.handleContentClick} ref={this.hostRef} />
         <span tabIndex={sentinelTabIndex} onFocus={this.onFocusEndingSentinel} className={styles.focusSentinel} />
       </div>
     );
@@ -78,23 +84,35 @@ export class DialogHost extends React.Component<DialogHostProps, {}> {
     event.stopPropagation();
   };
 
-  private saveHostRef = (elem: HTMLElement) => {
-    DialogService.setHost(elem);
-    this._hostRef = elem;
-  };
+  private getFocusableElementsInModal = (): any[] => {
+    if (this.hostRef.current) {
+      const result = [].filter.call(
+        this.hostRef.current.querySelectorAll('*'),
+        element => ~this.getTabIndex(element) && !element.hasAttribute('disabled')
+      );
 
-  private getFocusableElementsInModal = (): NodeList => {
-    if (this._hostRef) {
-      return this._hostRef.querySelectorAll('[tabIndex]:not([tabIndex="-1"])');
+      return result;
     }
-    return new NodeList();
+    return [];
   };
 
+  private getTabIndex(element) {
+    const { tabIndex } = element;
+    if (!~tabIndex) {
+      const attr = element.getAttribute('tabindex');
+      if (attr === null) {
+        return -1;
+      }
+    }
+
+    return tabIndex;
+  }
+
   private initFocusTrap = () => {
     const allFocusableElements = this.getFocusableElementsInModal();
     if (allFocusableElements.length) {
-      const firstChild: HTMLElement = allFocusableElements[0] as HTMLElement;
-      firstChild.focus();
+      const firstElement: HTMLElement = allFocusableElements[1] as HTMLElement;
+      firstElement && firstElement.focus();
     }
   };
 
@@ -103,44 +121,18 @@ export class DialogHost extends React.Component<DialogHostProps, {}> {
     e.preventDefault();
 
     const allFocusableElements = this.getFocusableElementsInModal();
-    if (allFocusableElements.length) {
-      let lastChild: HTMLElement = allFocusableElements[allFocusableElements.length - 1] as HTMLElement;
-
-      if (lastChild.hasAttribute('disabled')) {
-        // focus the last element in the list that isn't disabled
-        for (let i = allFocusableElements.length - 2; i >= 0; i--) {
-          lastChild = allFocusableElements[i] as HTMLElement;
-          if (!lastChild.hasAttribute('disabled')) {
-            lastChild.focus();
-            break;
-          }
-        }
-      } else {
-        lastChild.focus();
-      }
-    }
+    const lastElement = allFocusableElements.pop();
+
+    lastElement && lastElement.focus();
   };
 
   // Reached end of focusable items inside the modal host; re-focus the first item
   private onFocusEndingSentinel = (e: SyntheticEvent<any>) => {
     e.preventDefault();
 
     const allFocusableElements = this.getFocusableElementsInModal();
-    if (allFocusableElements.length) {
-      let firstChild: HTMLElement = allFocusableElements[0] as HTMLElement;
-
-      if (firstChild.hasAttribute('disabled')) {
-        // focus the first element in the list that isn't disabled
-        for (let i = 1; i <= allFocusableElements.length - 1; i++) {
-          firstChild = allFocusableElements[i] as HTMLElement;
-          if (!firstChild.hasAttribute('disabled')) {
-            firstChild.focus();
-            break;
-          }
-        }
-      } else {
-        firstChild.focus();
-      }
-    }
+    const firstElement = allFocusableElements[0];
+
+    firstElement && firstElement.focus();
   };
 }

packages/app/main/e2e/navigateADialog.spec.ts

@@ -0,0 +1,80 @@
+//
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license.
+//
+// Microsoft Bot Framework: http://botframework.com
+//
+// Bot Framework Emulator Github:
+// https://github.com/Microsoft/BotFramwork-Emulator
+//
+// Copyright (c) Microsoft Corporation
+// All rights reserved.
+//
+// MIT License:
+// Permission is hereby granted, free of charge, to any person obtaining
+// a copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to
+// permit persons to whom the Software is furnished to do so, subject to
+// the following conditions:
+//
+// The above copyright notice and this permission notice shall be
+// included in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED ""AS IS"", WITHOUT WARRANTY OF ANY KIND,
+// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+// LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+
+import { existsSync, unlinkSync } from 'fs';
+import { Application } from 'spectron';
+
+import { appPath, chromeDriverLogPath, electronPath } from './utils';
+
+import { init, setupMock } from './mocks/electronDialog';
+
+describe('Navigating a dialog with focus trap', () => {
+  let app: Application;
+
+  beforeEach(async () => {
+    jest.setTimeout(30000);
+    app = new Application({
+      path: electronPath,
+      args: [appPath],
+      chromeDriverLogPath,
+    });
+    init(app);
+    await app.start();
+  });
+
+  afterEach(async () => {
+    if (app && app.isRunning()) {
+      await app.stop();
+    }
+  });
+
+  it('should open a dialog and focus should be trapped in the dialog', async () => {
+    // wait for Welcome page to load
+    await app.client.waitForExist('button*=Sign in with your Azure');
+
+    // press escape to get out of "First time startup" dialogs.
+    await app.client.keys(['\uE00C', '\uE00C']);
+
+    await app.client.click('button*=Sign in with your Azure');
+    expect(await app.client.hasFocus("a*=Don't have an Azure Account?")).toBeTruthy();
+
+    // press tab to move focus within the dialog
+    await app.client.keys(['\uE004', '\uE004', '\uE004']);
+    expect(await app.client.hasFocus('button=Sign in with Azure')).toBeTruthy();
+
+    // press tab again to circle through the focus trap in the dialog
+    await app.client.keys(['\uE004', '\uE004']);
+    expect(await app.client.hasFocus("a*=Don't have an Azure Account?")).toBeTruthy();
+
+    // TODO: Improving this test would be to include a SHIFT+TAB focus check, but webdriver.io v4 doesn't have the ability to press two buttons simultaneously.
+  });
+});