Skip to content

Commit

Permalink
fix(material/sidenav): end position sidenav tab order not matching vi…
Browse files Browse the repository at this point in the history
…sual order

We project all sidenavs before the content in the DOM since we can't know ahead of time what their
position will be. This is problematic when the drawer is in the end position, because the visual
order of the content no longer matches the tab order. These changes fix the issue by moving the
sidenav after the content manually when it's set to `end` and then moving it back if it's set to
`start` again.

A couple of notes:
1. We could technically do this with content projection, but it would only work when the `position`
value is static (e.g. `position="end"`). I did it this way so we can cover the case where it's
data bound.
2. Currently the focus trap anchors are set around the sidenav, but that's problematic when we're
moving the element around since the anchors will be left at their old positions. To avoid adding
extra logic for moving the anchors, I've moved the focus trap to be inside the sidenav. Here's
what the DOM looks like at the moment:

```html
<container>
  <anchor/>
  <sidenav>Content</sidenav>
  <anchor/>
</container>
```

And this is what I've changed it to:
```html
<container>
  <sidenav>
    <anchor/>
    Content
    <anchor/>
  </sidenav>
</container
```

Fixes #15247.
  • Loading branch information
crisbeto committed Oct 15, 2020
1 parent 00f3274 commit 7620342
Show file tree
Hide file tree
Showing 4 changed files with 196 additions and 12 deletions.
2 changes: 1 addition & 1 deletion src/material/sidenav/drawer.html
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<div class="mat-drawer-inner-container">
<div class="mat-drawer-inner-container" #content>
<ng-content></ng-content>
</div>
134 changes: 133 additions & 1 deletion src/material/sidenav/drawer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,111 @@ describe('MatDrawer', () => {
}));

});

describe('DOM position', () => {
it('should project start drawer before the content', () => {
const fixture = TestBed.createComponent(BasicTestApp);
fixture.componentInstance.position = 'start';
fixture.detectChanges();

const allNodes = getDrawerNodesArray(fixture);
const drawerIndex = allNodes.indexOf(fixture.nativeElement.querySelector('.mat-drawer'));
const contentIndex =
allNodes.indexOf(fixture.nativeElement.querySelector('.mat-drawer-content'));

expect(drawerIndex).toBeGreaterThan(-1, 'Expected drawer to be inside the container');
expect(contentIndex).toBeGreaterThan(-1, 'Expected content to be inside the container');
expect(drawerIndex).toBeLessThan(contentIndex, 'Expected drawer to be before the content');
});

it('should project end drawer after the content', () => {
const fixture = TestBed.createComponent(BasicTestApp);
fixture.componentInstance.position = 'end';
fixture.detectChanges();

const allNodes = getDrawerNodesArray(fixture);
const drawerIndex = allNodes.indexOf(fixture.nativeElement.querySelector('.mat-drawer'));
const contentIndex =
allNodes.indexOf(fixture.nativeElement.querySelector('.mat-drawer-content'));

expect(drawerIndex).toBeGreaterThan(-1, 'Expected drawer to be inside the container');
expect(contentIndex).toBeGreaterThan(-1, 'Expected content to be inside the container');
expect(drawerIndex).toBeGreaterThan(contentIndex, 'Expected drawer to be after the content');
});

it('should move the drawer before/after the content when its position changes after being ' +
'initialized at `start`', () => {
const fixture = TestBed.createComponent(BasicTestApp);
fixture.componentInstance.position = 'start';
fixture.detectChanges();

const drawer = fixture.nativeElement.querySelector('.mat-drawer');
const content = fixture.nativeElement.querySelector('.mat-drawer-content');

let allNodes = getDrawerNodesArray(fixture);
const startDrawerIndex = allNodes.indexOf(drawer);
const startContentIndex = allNodes.indexOf(content);

expect(startDrawerIndex).toBeGreaterThan(-1, 'Expected drawer to be inside the container');
expect(startContentIndex)
.toBeGreaterThan(-1, 'Expected content to be inside the container');
expect(startDrawerIndex)
.toBeLessThan(startContentIndex, 'Expected drawer to be before the content on init');

fixture.componentInstance.position = 'end';
fixture.detectChanges();
allNodes = getDrawerNodesArray(fixture);

expect(allNodes.indexOf(drawer)).toBeGreaterThan(allNodes.indexOf(content),
'Expected drawer to be after content when position changes to `end`');

fixture.componentInstance.position = 'start';
fixture.detectChanges();
allNodes = getDrawerNodesArray(fixture);

expect(allNodes.indexOf(drawer)).toBeLessThan(allNodes.indexOf(content),
'Expected drawer to be before content when position changes back to `start`');
});

it('should move the drawer before/after the content when its position changes after being ' +
'initialized at `end`', () => {
const fixture = TestBed.createComponent(BasicTestApp);
fixture.componentInstance.position = 'end';
fixture.detectChanges();

const drawer = fixture.nativeElement.querySelector('.mat-drawer');
const content = fixture.nativeElement.querySelector('.mat-drawer-content');

let allNodes = getDrawerNodesArray(fixture);
const startDrawerIndex = allNodes.indexOf(drawer);
const startContentIndex = allNodes.indexOf(content);

expect(startDrawerIndex).toBeGreaterThan(-1, 'Expected drawer to be inside the container');
expect(startContentIndex)
.toBeGreaterThan(-1, 'Expected content to be inside the container');
expect(startDrawerIndex)
.toBeGreaterThan(startContentIndex, 'Expected drawer to be after the content on init');

fixture.componentInstance.position = 'start';
fixture.detectChanges();
allNodes = getDrawerNodesArray(fixture);

expect(allNodes.indexOf(drawer)).toBeLessThan(allNodes.indexOf(content),
'Expected drawer to be before content when position changes to `start`');

fixture.componentInstance.position = 'end';
fixture.detectChanges();
allNodes = getDrawerNodesArray(fixture);

expect(allNodes.indexOf(drawer)).toBeGreaterThan(allNodes.indexOf(content),
'Expected drawer to be after content when position changes back to `end`');
});

function getDrawerNodesArray(fixture: ComponentFixture<any>): HTMLElement[] {
return Array.from(fixture.nativeElement.querySelector('.mat-drawer-container').childNodes);
}

});
});

describe('MatDrawerContainer', () => {
Expand Down Expand Up @@ -925,6 +1030,32 @@ describe('MatDrawerContainer', () => {
expect(spy).toHaveBeenCalled();
subscription.unsubscribe();
}));

it('should position the drawers before/after the content in the DOM based on their position',
fakeAsync(() => {
const fixture = TestBed.createComponent(DrawerContainerTwoDrawerTestApp);
fixture.detectChanges();

const drawerDebugElements = fixture.debugElement.queryAll(By.directive(MatDrawer));
const [start, end] = drawerDebugElements.map(el => el.componentInstance);
const [startNode, endNode] = drawerDebugElements.map(el => el.nativeElement);
const contentNode = fixture.nativeElement.querySelector('.mat-drawer-content');
const allNodes: HTMLElement[] =
Array.from(fixture.nativeElement.querySelector('.mat-drawer-container').childNodes);
const startIndex = allNodes.indexOf(startNode);
const endIndex = allNodes.indexOf(endNode);
const contentIndex = allNodes.indexOf(contentNode);

expect(start.position).toBe('start');
expect(end.position).toBe('end');
expect(contentIndex).toBeGreaterThan(-1, 'Expected content to be inside the container');
expect(startIndex).toBeGreaterThan(-1, 'Expected start drawer to be inside the container');
expect(endIndex).toBeGreaterThan(-1, 'Expected end drawer to be inside the container');

expect(startIndex).toBeLessThan(contentIndex, 'Expected start drawer to be before content');
expect(endIndex).toBeGreaterThan(contentIndex, 'Expected end drawer to be after content');
}));

});


Expand All @@ -948,7 +1079,7 @@ class DrawerContainerTwoDrawerTestApp {
@Component({
template: `
<mat-drawer-container (backdropClick)="backdropClicked()" [hasBackdrop]="hasBackdrop">
<mat-drawer #drawer="matDrawer" position="start"
<mat-drawer #drawer="matDrawer" [position]="position"
(opened)="open()"
(openedStart)="openStart()"
(closed)="close()"
Expand All @@ -974,6 +1105,7 @@ class BasicTestApp {
closeStartCount = 0;
backdropClickedCount = 0;
hasBackdrop: boolean | null = null;
position = 'start';

@ViewChild('drawer') drawer: MatDrawer;
@ViewChild('drawerButton') drawerButton: ElementRef<HTMLButtonElement>;
Expand Down
65 changes: 58 additions & 7 deletions src/material/sidenav/drawer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
ViewEncapsulation,
HostListener,
HostBinding,
AfterViewInit,
} from '@angular/core';
import {fromEvent, merge, Observable, Subject} from 'rxjs';
import {
Expand Down Expand Up @@ -138,20 +139,32 @@ export class MatDrawerContent extends CdkScrollable implements AfterContentInit
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
})
export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestroy {
export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy {
private _focusTrap: FocusTrap;
private _elementFocusedBeforeDrawerWasOpened: HTMLElement | null = null;
private _document: Document;

/** Whether the drawer is initialized. Used for disabling the initial animation. */
private _enableAnimations = false;

/** Whether the view of the component has been attached. */
private _isAttached: boolean;

/** Anchor node used to restore the drawer to its initial position. */
private _anchor: Comment | null;

/** The side that the drawer is attached to. */
@Input()
get position(): 'start' | 'end' { return this._position; }
set position(value: 'start' | 'end') {
// Make sure we have a valid value.
value = value === 'end' ? 'end' : 'start';
if (value != this._position) {
if (value !== this._position) {
// Static inputs in Ivy are set before the element is in the DOM.
if (this._isAttached) {
this._updatePositionInParent(value);
}

this._position = value;
this.onPositionChanged.emit();
}
Expand Down Expand Up @@ -251,6 +264,9 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
// tslint:disable-next-line:no-output-on-prefix
@Output('positionChanged') onPositionChanged: EventEmitter<void> = new EventEmitter<void>();

/** Reference to the inner element that contains all the content. */
@ViewChild('content') _content: ElementRef<HTMLElement>;

/**
* An observable that emits when the drawer mode changes. This is used by the drawer container to
* to know when to when the mode changes so it can adapt the margins on the content.
Expand All @@ -262,13 +278,14 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
private _focusMonitor: FocusMonitor,
private _platform: Platform,
private _ngZone: NgZone,
@Optional() @Inject(DOCUMENT) private _doc: any,
@Optional() @Inject(DOCUMENT) _document: any,
@Optional() @Inject(MAT_DRAWER_CONTAINER) public _container?: MatDrawerContainer) {

this._document = _document;
this.openedChange.subscribe((opened: boolean) => {
if (opened) {
if (this._doc) {
this._elementFocusedBeforeDrawerWasOpened = this._doc.activeElement as HTMLElement;
if (this._document) {
this._elementFocusedBeforeDrawerWasOpened = this._document.activeElement as HTMLElement;
}

this._takeFocus();
Expand Down Expand Up @@ -349,13 +366,20 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr

/** Whether focus is currently within the drawer. */
private _isFocusWithinDrawer(): boolean {
const activeEl = this._doc?.activeElement;
const activeEl = this._document.activeElement;
return !!activeEl && this._elementRef.nativeElement.contains(activeEl);
}

ngAfterContentInit() {
ngAfterViewInit() {
this._isAttached = true;
this._focusTrap = this._focusTrapFactory.create(this._elementRef.nativeElement);
this._updateFocusTrapState();

// Only update the DOM position when the sidenav is positioned at
// the end since we project the sidenav before the content by default.
if (this._position === 'end') {
this._updatePositionInParent('end');
}
}

ngAfterContentChecked() {
Expand All @@ -373,6 +397,11 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
this._focusTrap.destroy();
}

if (this._anchor && this._anchor.parentNode) {
this._anchor.parentNode.removeChild(this._anchor);
}

this._anchor = null;
this._animationStarted.complete();
this._animationEnd.complete();
this._modeChanged.complete();
Expand Down Expand Up @@ -456,6 +485,28 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
}
}

/**
* Updates the position of the drawer in the DOM. We need to move the element around ourselves
* when it's in the `end` position so that it comes after the content and the visual order
* matches the tab order. We also need to be able to move it back to `start` if the sidenav
* started off as `end` and was changed to `start`.
*/
private _updatePositionInParent(newPosition: 'start' | 'end') {
const element = this._elementRef.nativeElement;
const parent = element.parentNode!;

if (newPosition === 'end') {
if (!this._anchor) {
this._anchor = this._document.createComment('mat-drawer-anchor');
parent.insertBefore(this._anchor, element);
}

parent.appendChild(element);
} else if (this._anchor) {
this._anchor.parentNode!.insertBefore(element, this._anchor);
}
}

// We have to use a `HostListener` here in order to support both Ivy and ViewEngine.
// In Ivy the `host` bindings will be merged when this class is extended, whereas in
// ViewEngine they're overwritten.
Expand Down
7 changes: 4 additions & 3 deletions tools/public_api_guard/material/sidenav.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ export declare const MAT_DRAWER_DEFAULT_AUTOSIZE: InjectionToken<boolean>;

export declare function MAT_DRAWER_DEFAULT_AUTOSIZE_FACTORY(): boolean;

export declare class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestroy {
export declare class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy {
_animationEnd: Subject<AnimationEvent>;
_animationStarted: Subject<AnimationEvent>;
_animationState: 'open-instant' | 'open' | 'void';
_closedStream: Observable<void>;
_container?: MatDrawerContainer | undefined;
_content: ElementRef<HTMLElement>;
readonly _modeChanged: Subject<void>;
_openedStream: Observable<void>;
get autoFocus(): boolean;
Expand All @@ -24,14 +25,14 @@ export declare class MatDrawer implements AfterContentInit, AfterContentChecked,
readonly openedStart: Observable<void>;
get position(): 'start' | 'end';
set position(value: 'start' | 'end');
constructor(_elementRef: ElementRef<HTMLElement>, _focusTrapFactory: FocusTrapFactory, _focusMonitor: FocusMonitor, _platform: Platform, _ngZone: NgZone, _doc: any, _container?: MatDrawerContainer | undefined);
constructor(_elementRef: ElementRef<HTMLElement>, _focusTrapFactory: FocusTrapFactory, _focusMonitor: FocusMonitor, _platform: Platform, _ngZone: NgZone, _document: any, _container?: MatDrawerContainer | undefined);
_animationDoneListener(event: AnimationEvent): void;
_animationStartListener(event: AnimationEvent): void;
_closeViaBackdropClick(): Promise<MatDrawerToggleResult>;
_getWidth(): number;
close(): Promise<MatDrawerToggleResult>;
ngAfterContentChecked(): void;
ngAfterContentInit(): void;
ngAfterViewInit(): void;
ngOnDestroy(): void;
open(openedVia?: FocusOrigin): Promise<MatDrawerToggleResult>;
toggle(isOpen?: boolean, openedVia?: FocusOrigin): Promise<MatDrawerToggleResult>;
Expand Down

0 comments on commit 7620342

Please sign in to comment.