fix: address feedback

Author: crisbeto

src/lib/core/a11y/focus-trap.spec.ts

@@ -1,6 +1,6 @@
 import {ComponentFixture, TestBed, async} from '@angular/core/testing';
 import {Component, ViewChild} from '@angular/core';
-import {FocusTrapService, FocusTrapDirective, FocusTrap} from './focus-trap';
+import {FocusTrapFactory, FocusTrapDirective, FocusTrap} from './focus-trap';
 import {InteractivityChecker} from './interactivity-checker';
 import {Platform} from '../platform/platform';
 
@@ -16,7 +16,7 @@ describe('FocusTrap', () => {
     beforeEach(async(() => {
       TestBed.configureTestingModule({
         declarations: [FocusTrapDirective, FocusTrapTestApp],
-        providers: [InteractivityChecker, Platform, FocusTrapService]
+        providers: [InteractivityChecker, Platform, FocusTrapFactory]
       });
 
       TestBed.compileComponents();
@@ -65,7 +65,7 @@ describe('FocusTrap', () => {
 
       expect(anchors.every(current => current.getAttribute('tabindex') === '0')).toBe(true);
 
-      fixture.componentInstance.isFocusTrapDisabled = true;
+      fixture.componentInstance.isFocusTrapEnabled = false;
       fixture.detectChanges();
 
       expect(anchors.every(current => current.getAttribute('tabindex') === '-1')).toBe(true);
@@ -79,7 +79,7 @@ describe('FocusTrap', () => {
     beforeEach(async(() => {
       TestBed.configureTestingModule({
         declarations: [FocusTrapDirective, FocusTrapTargetTestApp],
-        providers: [InteractivityChecker, Platform, FocusTrapService]
+        providers: [InteractivityChecker, Platform, FocusTrapFactory]
       });
 
       TestBed.compileComponents();
@@ -108,27 +108,27 @@ describe('FocusTrap', () => {
 
 @Component({
   template: `
-    <cdk-focus-trap *ngIf="renderFocusTrap" [disabled]="isFocusTrapDisabled">
+    <div *ngIf="renderFocusTrap" [cdkTrapFocus]="isFocusTrapEnabled">
       <input>
       <button>SAVE</button>
-    </cdk-focus-trap>
+    </div>
     `
 })
 class FocusTrapTestApp {
   @ViewChild(FocusTrapDirective) focusTrapDirective: FocusTrapDirective;
   renderFocusTrap = true;
-  isFocusTrapDisabled = false;
+  isFocusTrapEnabled = true;
 }
 
 
 @Component({
   template: `
-    <cdk-focus-trap>
+    <div cdkTrapFocus>
       <input>
       <button id="last" cdk-focus-end></button>
       <button id="first" cdk-focus-start>SAVE</button>
       <input>
-    </cdk-focus-trap>
+    </div>
     `
 })
 class FocusTrapTargetTestApp {

src/lib/core/a11y/focus-trap.ts

@@ -24,17 +24,12 @@ export class FocusTrap {
   private _endAnchor: HTMLElement = this._createAnchor();
 
   /** Whether the focus trap is active. */
-  get disabled(): boolean { return this._disabled; }
-  set disabled(val: boolean) {
-    this._disabled = val;
-    this._startAnchor.tabIndex = this._endAnchor.tabIndex = this._disabled ? -1 : 0;
-  }
-  private _disabled: boolean = false;
-
-  /** Element to which the focus trap is attached. */
-  get element(): HTMLElement {
-    return this._element;
+  get enabled(): boolean { return this._enabled; }
+  set enabled(val: boolean) {
+    this._enabled = val;
+    this._startAnchor.tabIndex = this._endAnchor.tabIndex = this._enabled ? 0 : -1;
   }
+  private _enabled: boolean = false;
 
   constructor(
     private _element: HTMLElement,
@@ -170,10 +165,10 @@ export class FocusTrap {
 
 
 /**
- * Service that allows easy instantiation of focus traps.
+ * Factory that allows easy instantiation of focus traps.
  */
 @Injectable()
-export class FocusTrapService {
+export class FocusTrapFactory {
   constructor(private _checker: InteractivityChecker, private _ngZone: NgZone) { }
 
   attach(element: HTMLElement, deferAnchors = false): FocusTrap {
@@ -184,22 +179,51 @@ export class FocusTrapService {
 
 /**
  * Directive for trapping focus within a region.
+ * @deprecated
  */
 @Directive({
-  selector: 'cdk-focus-trap, [cdkFocusTrap]',
+  selector: 'cdk-focus-trap',
 })
-export class FocusTrapDirective implements OnDestroy, AfterContentInit {
+export class FocusTrapDeprecatedDirective implements OnDestroy, AfterContentInit {
   focusTrap: FocusTrap;
 
   /** Whether the focus trap is active. */
   @Input()
-  get disabled(): boolean { return this.focusTrap.disabled; }
+  get disabled(): boolean { return !this.focusTrap.enabled; }
   set disabled(val: boolean) {
-    this.focusTrap.disabled = coerceBooleanProperty(val);
+    this.focusTrap.enabled = !coerceBooleanProperty(val);
+  }
+
+  constructor(private _elementRef: ElementRef, private _focusTrapFactory: FocusTrapFactory) {
+    this.focusTrap = this._focusTrapFactory.attach(this._elementRef.nativeElement, true);
+  }
+
+  ngOnDestroy() {
+    this.focusTrap.destroy();
+  }
+
+  ngAfterContentInit() {
+    this.focusTrap.attachAnchors();
   }
+}
+
+
+/**
+ * Directive for trapping focus within a region.
+ */
+@Directive({
+  selector: '[cdkTrapFocus]'
+})
+export class FocusTrapDirective implements OnDestroy, AfterContentInit {
+  focusTrap: FocusTrap;
+
+  /** Whether the focus trap is active. */
+  @Input('cdkTrapFocus')
+  get enabled(): boolean { return this.focusTrap.enabled; }
+  set enabled(val: boolean) { this.focusTrap.enabled = val; }
 
-  constructor(private _elementRef: ElementRef, private _focusTrapService: FocusTrapService) {
-    this.focusTrap = this._focusTrapService.attach(this._elementRef.nativeElement, true);
+  constructor(private _elementRef: ElementRef, private _focusTrapFactory: FocusTrapFactory) {
+    this.focusTrap = this._focusTrapFactory.attach(this._elementRef.nativeElement, true);
   }
 
   ngOnDestroy() {

src/lib/core/a11y/index.ts

@@ -1,15 +1,15 @@
 import {NgModule, ModuleWithProviders} from '@angular/core';
-import {FocusTrapDirective, FocusTrapService} from './focus-trap';
+import {FocusTrapDirective, FocusTrapDeprecatedDirective, FocusTrapFactory} from './focus-trap';
 import {LIVE_ANNOUNCER_PROVIDER} from './live-announcer';
 import {InteractivityChecker} from './interactivity-checker';
 import {CommonModule} from '@angular/common';
 import {PlatformModule} from '../platform/index';
 
 @NgModule({
   imports: [CommonModule, PlatformModule],
-  declarations: [FocusTrapDirective],
-  exports: [FocusTrapDirective],
-  providers: [InteractivityChecker, FocusTrapService, LIVE_ANNOUNCER_PROVIDER]
+  declarations: [FocusTrapDirective, FocusTrapDeprecatedDirective],
+  exports: [FocusTrapDirective, FocusTrapDeprecatedDirective],
+  providers: [InteractivityChecker, FocusTrapFactory, LIVE_ANNOUNCER_PROVIDER]
 })
 export class A11yModule {
   /** @deprecated */

src/lib/dialog/dialog-container.ts

@@ -12,7 +12,7 @@ import {BasePortalHost, ComponentPortal, PortalHostDirective, TemplatePortal} fr
 import {MdDialogConfig} from './dialog-config';
 import {MdDialogRef} from './dialog-ref';
 import {MdDialogContentAlreadyAttachedError} from './dialog-errors';
-import {FocusTrapService, FocusTrap} from '../core/a11y/focus-trap';
+import {FocusTrapFactory, FocusTrap} from '../core/a11y/focus-trap';
 import 'rxjs/add/operator/first';
 
 
@@ -51,7 +51,7 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
     private _ngZone: NgZone,
     private _renderer: Renderer,
     private _elementRef: ElementRef,
-    private _focusTrapService: FocusTrapService) {
+    private _focusTrapFactory: FocusTrapFactory) {
 
     super();
   }
@@ -90,7 +90,7 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
    */
   private _trapFocus() {
     if (!this._focusTrap) {
-      this._focusTrap = this._focusTrapService.attach(this._elementRef.nativeElement);
+      this._focusTrap = this._focusTrapFactory.attach(this._elementRef.nativeElement);
     }
 
     // If were to attempt to focus immediately, then the content of the dialog would not yet be

src/lib/sidenav/sidenav.html

@@ -1,3 +1 @@
-<cdk-focus-trap class="mat-sidenav-focus-trap" [disabled]="isFocusTrapDisabled">
-  <ng-content></ng-content>
-</cdk-focus-trap>
+<ng-content></ng-content>

src/lib/sidenav/sidenav.scss

@@ -100,6 +100,9 @@
   z-index: 3;
   min-width: 5vw;
   outline: 0;
+  box-sizing: border-box;
+  height: 100%;
+  overflow-y: auto; // TODO(kara): revisit scrolling behavior for sidenavs
 
   @include mat-sidenav-transition(0, -100%);
 
@@ -133,16 +136,6 @@
   }
 }
 
-.mat-sidenav-focus-trap {
-  box-sizing: border-box;
-  height: 100%;
-  display: block;
-  overflow-y: auto; // TODO(kara): revisit scrolling behavior for sidenavs
-
-  // Prevents unnecessary repaints while scrolling.
-  transform: translateZ(0);
-}
-
 .mat-sidenav-invalid {
   display: none;
 }

src/lib/sidenav/sidenav.ts

@@ -13,13 +13,13 @@ import {
   EventEmitter,
   Renderer,
   ViewEncapsulation,
-  ViewChild,
-  NgZone
+  NgZone,
+  OnDestroy,
 } from '@angular/core';
 import {CommonModule} from '@angular/common';
 import {Dir, MdError, coerceBooleanProperty, CompatibilityModule} from '../core';
 import {A11yModule} from '../core/a11y/index';
-import {FocusTrapDirective} from '../core/a11y/focus-trap';
+import {FocusTrapFactory, FocusTrap} from '../core/a11y/focus-trap';
 import {ESCAPE} from '../core/keyboard/keycodes';
 import {OverlayModule} from '../core/overlay/overlay-directives';
 import 'rxjs/add/operator/first';
@@ -71,8 +71,8 @@ export class MdSidenavToggleResult {
   changeDetection: ChangeDetectionStrategy.OnPush,
   encapsulation: ViewEncapsulation.None,
 })
-export class MdSidenav implements AfterContentInit {
-  @ViewChild(FocusTrapDirective) _focusTrapDirective: FocusTrapDirective;
+export class MdSidenav implements AfterContentInit, OnDestroy {
+  private _focusTrap: FocusTrap;
 
   /** Alignment of the sidenav (direction neutral); whether 'start' or 'end'. */
   private _align: 'start' | 'end' = 'start';
@@ -138,21 +138,25 @@ export class MdSidenav implements AfterContentInit {
    */
   private _resolveToggleAnimationPromise: (animationFinished: boolean) => void = null;
 
-  get isFocusTrapDisabled() {
+  get isFocusTrapEnabled() {
     // The focus trap is only enabled when the sidenav is open in any mode other than side.
-    return !this.opened || this.mode == 'side';
+    return this.opened && this.mode !== 'side';
   }
 
   /**
    * @param _elementRef The DOM element reference. Used for transition and width calculation.
    *     If not available we do not hook on transitions.
    */
-  constructor(private _elementRef: ElementRef, private _renderer: Renderer) {
+  constructor(
+    private _elementRef: ElementRef,
+    private _renderer: Renderer,
+    private _focusTrapFactory: FocusTrapFactory) {
+
     this.onOpen.subscribe(() => {
       this._elementFocusedBeforeSidenavWasOpened = document.activeElement as HTMLElement;
 
-      if (!this.isFocusTrapDisabled) {
-        this._focusTrapDirective.focusTrap.focusFirstTabbableElementWhenReady();
+      if (this.isFocusTrapEnabled && this._focusTrap) {
+        this._focusTrap.focusFirstTabbableElementWhenReady();
       }
     });
 
@@ -168,14 +172,23 @@ export class MdSidenav implements AfterContentInit {
   }
 
   ngAfterContentInit() {
-    // This can happen when the sidenav is set to opened in the template and the transition
-    // isn't ended.
+    this._focusTrap = this._focusTrapFactory.attach(this._elementRef.nativeElement);
+    this._focusTrap.enabled = this.isFocusTrapEnabled;
+
+    // This can happen when the sidenav is set to opened in
+    // the template and the transition hasn't ended.
     if (this._toggleAnimationPromise) {
       this._resolveToggleAnimationPromise(true);
       this._toggleAnimationPromise = this._resolveToggleAnimationPromise = null;
     }
   }
 
+  ngOnDestroy() {
+    if (this._focusTrap) {
+      this._focusTrap.destroy();
+    }
+  }
+
   /**
    * Whether the sidenav is opened. We overload this because we trigger an event when it
    * starts or end.
@@ -220,6 +233,10 @@ export class MdSidenav implements AfterContentInit {
 
     this._opened = isOpen;
 
+    if (this._focusTrap) {
+      this._focusTrap.enabled = this.isFocusTrapEnabled;
+    }
+
     if (isOpen) {
       this.onOpenStart.emit();
     } else {