From db943b53c781199e4d5ca41cb92b0e92d541a729 Mon Sep 17 00:00:00 2001
From: crisbeto <crisbeto@abv.bg>
Date: Sat, 15 Sep 2018 08:01:36 +0200
Subject: [PATCH] fix(a11y): not being able to escape disabled focus trap using
 arrow keys

Currently when a focus trap is disabled, we set the `tabindex` of the anchors to -1 in order to allow people to tab out of it. This doesn't work if somebody is navigating with the arrow keys using a screen reader, because the element is still focusable which means that the screen reader will focus it eventually, causing focus to be trapped. These changes remove the `tabindex` if the focus trap is disabled instead.

**Note:** An alternate approach to this can be to hide the element using `display: none`, but I opted to remove the `tabindex` in order to avoid a style recalculation.

Fixes #13132.
---
 src/cdk/a11y/focus-trap/focus-trap.spec.ts |  2 +-
 src/cdk/a11y/focus-trap/focus-trap.ts      | 20 ++++++++++++++++----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/src/cdk/a11y/focus-trap/focus-trap.spec.ts b/src/cdk/a11y/focus-trap/focus-trap.spec.ts
index 20fa655523de..9529b77ee1d2 100644
--- a/src/cdk/a11y/focus-trap/focus-trap.spec.ts
+++ b/src/cdk/a11y/focus-trap/focus-trap.spec.ts
@@ -104,7 +104,7 @@ describe('FocusTrap', () => {
       fixture.componentInstance._isFocusTrapEnabled = false;
       fixture.detectChanges();
 
-      expect(anchors.every(current => current.getAttribute('tabindex') === '-1')).toBe(true);
+      expect(anchors.every(current => !current.hasAttribute('tabindex'))).toBe(true);
     });
   });
 
diff --git a/src/cdk/a11y/focus-trap/focus-trap.ts b/src/cdk/a11y/focus-trap/focus-trap.ts
index 4549dd478059..1ecadce016d7 100644
--- a/src/cdk/a11y/focus-trap/focus-trap.ts
+++ b/src/cdk/a11y/focus-trap/focus-trap.ts
@@ -37,11 +37,12 @@ export class FocusTrap {
 
   /** Whether the focus trap is active. */
   get enabled(): boolean { return this._enabled; }
-  set enabled(val: boolean) {
-    this._enabled = val;
+  set enabled(value: boolean) {
+    this._enabled = value;
 
     if (this._startAnchor && this._endAnchor) {
-      this._startAnchor.tabIndex = this._endAnchor.tabIndex = this._enabled ? 0 : -1;
+      this._toggleAnchorTabIndex(value, this._startAnchor);
+      this._toggleAnchorTabIndex(value, this._endAnchor);
     }
   }
   private _enabled: boolean = true;
@@ -278,12 +279,23 @@ export class FocusTrap {
   /** Creates an anchor element. */
   private _createAnchor(): HTMLElement {
     const anchor = this._document.createElement('div');
-    anchor.tabIndex = this._enabled ? 0 : -1;
+    this._toggleAnchorTabIndex(this._enabled, anchor);
     anchor.classList.add('cdk-visually-hidden');
     anchor.classList.add('cdk-focus-trap-anchor');
     return anchor;
   }
 
+  /**
+   * Toggles the `tabindex` of an anchor, based on the enabled state of the focus trap.
+   * @param isEnabled Whether the focus trap is enabled.
+   * @param anchor Anchor on which to toggle the tabindex.
+   */
+  private _toggleAnchorTabIndex(isEnabled: boolean, anchor: HTMLElement) {
+    // Remove the tabindex completely, rather than setting it to -1, because if the
+    // element has a tabindex, the user might still hit it when navigating with the arrow keys.
+    isEnabled ? anchor.setAttribute('tabindex', '0') : anchor.removeAttribute('tabindex');
+  }
+
   /** Executes a function when the zone is stable. */
   private _executeOnStable(fn: () => any): void {
     if (this._ngZone.isStable) {