Focus capturing for sidenav. #1695

mmalerba · 2016-11-02T19:14:01Z

Captures focus when sidenav is open in "over" or "push" mode, but not
when opened in "side" mode.

mmalerba · 2016-11-02T19:16:27Z

src/lib/dialog/dialog-container.ts

-      this._elementFocusedBeforeDialogWasOpened = document.activeElement;
-      this._focusTrap.focusFirstTabbableElement();
-    });
+    this._elementFocusedBeforeDialogWasOpened = document.activeElement;


Do you think its worth moving this to the FocusTrap as well? Maybe like this: focusOnDeactivate=""

mmalerba · 2016-11-02T19:59:22Z

fixes #50

jelbourn · 2016-11-03T01:05:25Z

cc @crisbeto, relevant to dialog

jelbourn · 2016-11-03T01:06:27Z

@marcysutton as far as you know, is there ever a time when you would want to trap focus within some subtree of the DOM and not automatically move focus into that subtree as soon as it's added to the page?

crisbeto · 2016-11-03T16:43:33Z

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

@@ -1,4 +1,6 @@
-import {Component, ViewEncapsulation, ViewChild, ElementRef} from '@angular/core';
+import {
+    Component, ViewEncapsulation, ViewChild, ElementRef, Input, AfterContentInit, NgZone


I think these are supposed to all be on separate lines.

crisbeto · 2016-11-03T16:46:01Z

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

+  set active(val: boolean) {
+    this._active = val;
+    if (val && this._contentReady) {
+      this._ngZone.onMicrotaskEmpty.first().subscribe(() => {


This one is used here and repeated down inside ngAfterContentInit. It might a good idea to move it to a private method like waitForTabbableElement.

crisbeto · 2016-11-03T16:47:24Z

src/lib/dialog/dialog-container.ts

@@ -45,6 +41,9 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
  /** Reference to the open dialog. */
  dialogRef: MdDialogRef<any>;

+  /** Whether the focus trap is active. */
+  focusTrapActive: boolean = false;


Not sure what the style guide says about this, but wouldn't the boolean be inferred from the default value anyway @jelbourn?

marcysutton · 2016-11-03T18:06:46Z

@jelbourn focus traps are usually enabled/disabled on command when the UI requires it, such as a modal dialog being shown. Otherwise you run the risk of trapping a keyboard user inside a menu at the wrong time.

You might want to check out HTML inert and the related polyfill for this purpose. https://github.com/GoogleChrome/inert-polyfill

mmalerba · 2016-11-04T16:40:07Z

focus traps are usually enabled/disabled on command when the UI requires it

@marcysutton That sounds like what I'm doing here, I added an active attribute to focus-trap when it changes to true the focus trap automatically focuses the first focusable element inside it.

mmalerba · 2016-11-09T20:01:28Z

Changed to not automatically focus the first element when it's activated.

jelbourn · 2016-11-03T00:54:25Z

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

+  private _contentReady: boolean = false;
+
+  /** Whether the focus trap is active. */
+  private _active: boolean = true;


How do you feel about inverting this to disabled (with coerceBooleanProperty)? It might make more sense if active is the default.

jelbourn · 2016-11-10T04:38:35Z

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

+    // Trigger setter behavior.
+    if (this.active) {
+
+    }


Empty if?

jelbourn · 2016-11-10T04:41:36Z

src/lib/sidenav/sidenav.ts

@@ -106,6 +111,10 @@ export class MdSidenav implements AfterContentInit {
  /** Event emitted when the sidenav alignment changes. */
  @Output('align-changed') onAlignChanged = new EventEmitter<void>();

+  get focusTrapActive() {


isFocusTrapActive? (or isFocusTrapDisabled is we invert it)

Add a comment like

// The focus trap is only enabled when the sidenav is open in any mode // except for side.

jelbourn · 2016-11-10T04:43:39Z

src/lib/sidenav/sidenav.spec.ts

+      link2Element.focus();
+    });
+
+    it('should trp focus when opened in "over" mode', fakeAsync(() => {


trp -> trap

jelbourn · 2016-11-10T04:44:03Z

src/lib/sidenav/sidenav.spec.ts

+      expect(document.activeElement).toBe(link1Element);
+    }));
+
+    it('should trap tabs when opened in "push" mode', fakeAsync(() => {


jelbourn · 2016-11-10T17:32:00Z

src/lib/sidenav/sidenav.spec.ts

+      sidenav = fixture.debugElement.query(By.directive(MdSidenav)).componentInstance;
+      link1Element = fixture.debugElement.query(By.css('.link1')).nativeElement;
+      link2Element = fixture.debugElement.query(By.css('.link1')).nativeElement;
+      link2Element.focus();


Can we call these something like firstFocusableElement and lastFocusableElement?

jelbourn · 2016-11-14T23:14:55Z

LGTM

kara · 2016-11-15T03:11:21Z

@mmalerba Presubmit is unhappy because

Property 'active' does not exist on type 'FocusTrap'

mmalerba · 2016-11-15T15:56:49Z

@kara fixed, strange that neither build nor lint caught that

kara · 2016-11-16T00:59:57Z

@mmalerba Property '_focusTrap' is private and only accessible within class 'MdSidenav'.

I think it needs to be internal instead of private.

jelbourn · 2016-12-02T23:41:55Z

@mmalerba can you rebase?

Captures focus when sidenav is open in "over" or "push" mode, but not when opened in "side" mode.

angular-automatic-lock-bot · 2019-09-06T06:01:48Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mmalerba added the pr: needs review label Nov 2, 2016

mmalerba assigned jelbourn Nov 2, 2016

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 2, 2016

mmalerba commented Nov 2, 2016

View reviewed changes

crisbeto reviewed Nov 3, 2016

View reviewed changes

mmalerba force-pushed the snav-focus branch from 3ec486a to f64c59c Compare November 9, 2016 19:45

jelbourn reviewed Nov 10, 2016

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Nov 14, 2016

kara removed the action: merge The PR is ready for merge by the caretaker label Nov 15, 2016

kara added the action: merge The PR is ready for merge by the caretaker label Nov 16, 2016

kara removed the action: merge The PR is ready for merge by the caretaker label Nov 16, 2016

mmalerba added the action: merge The PR is ready for merge by the caretaker label Nov 16, 2016

mmalerba force-pushed the snav-focus branch from af967cb to ab0e985 Compare November 28, 2016 23:18

tinayuangao added the pr: needs rebase label Dec 1, 2016

mmalerba removed the pr: needs rebase label Dec 1, 2016

jelbourn added the pr: needs rebase label Dec 2, 2016

Focus capturing for sidenav.

a639109

Captures focus when sidenav is open in "over" or "push" mode, but not when opened in "side" mode.

mmalerba added 5 commits December 2, 2016 16:09

lint fixes.

c56579a

address comments

ce93610

addressed comments

e316dca

s/active/!disabled

b44b0fb

fix tests

aaf4aa4

mmalerba force-pushed the snav-focus branch from 65780da to aaf4aa4 Compare December 3, 2016 00:15

fix lint

7b123c9

mmalerba removed the pr: needs rebase label Dec 5, 2016

fix visibility issue

7b86418

mmalerba merged commit b9c3304 into angular:master Dec 6, 2016

mmalerba mentioned this pull request Dec 7, 2016

feat(sidenav): close via escape key and restore focus to trigger element #1990

Merged

mmalerba deleted the snav-focus branch December 25, 2016 23:37

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Focus capturing for sidenav. #1695

Focus capturing for sidenav. #1695

mmalerba commented Nov 2, 2016

mmalerba Nov 2, 2016

mmalerba commented Nov 2, 2016

jelbourn commented Nov 3, 2016

jelbourn commented Nov 3, 2016 •

edited

Loading

crisbeto Nov 3, 2016

crisbeto Nov 3, 2016

crisbeto Nov 3, 2016

marcysutton commented Nov 3, 2016

mmalerba commented Nov 4, 2016 •

edited

Loading

mmalerba commented Nov 9, 2016

jelbourn Nov 3, 2016

jelbourn Nov 10, 2016

jelbourn Nov 10, 2016

jelbourn Nov 10, 2016

jelbourn Nov 10, 2016

jelbourn Nov 10, 2016

jelbourn commented Nov 14, 2016

kara commented Nov 15, 2016

mmalerba commented Nov 15, 2016

kara commented Nov 16, 2016

jelbourn commented Dec 2, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019

Focus capturing for sidenav. #1695

Focus capturing for sidenav. #1695

Conversation

mmalerba commented Nov 2, 2016

Choose a reason for hiding this comment

mmalerba commented Nov 2, 2016

jelbourn commented Nov 3, 2016

jelbourn commented Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcysutton commented Nov 3, 2016

mmalerba commented Nov 4, 2016 • edited Loading

mmalerba commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented Nov 14, 2016

kara commented Nov 15, 2016

mmalerba commented Nov 15, 2016

kara commented Nov 16, 2016

jelbourn commented Dec 2, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019

jelbourn commented Nov 3, 2016 •

edited

Loading

mmalerba commented Nov 4, 2016 •

edited

Loading