Skip to content

Commit 44a210d

Browse files
committed
fix(sidenav): end position sidenav tab order not matching visual 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.
1 parent db4b0cd commit 44a210d

File tree

4 files changed

+196
-12
lines changed

4 files changed

+196
-12
lines changed

src/material/sidenav/drawer.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
<div class="mat-drawer-inner-container">
1+
<div class="mat-drawer-inner-container" #content>
22
<ng-content></ng-content>
33
</div>

src/material/sidenav/drawer.spec.ts

+133-1
Original file line numberDiff line numberDiff line change
@@ -643,6 +643,111 @@ describe('MatDrawer', () => {
643643
}));
644644

645645
});
646+
647+
describe('DOM position', () => {
648+
it('should project start drawer before the content', () => {
649+
const fixture = TestBed.createComponent(BasicTestApp);
650+
fixture.componentInstance.position = 'start';
651+
fixture.detectChanges();
652+
653+
const allNodes = getDrawerNodesArray(fixture);
654+
const drawerIndex = allNodes.indexOf(fixture.nativeElement.querySelector('.mat-drawer'));
655+
const contentIndex =
656+
allNodes.indexOf(fixture.nativeElement.querySelector('.mat-drawer-content'));
657+
658+
expect(drawerIndex).toBeGreaterThan(-1, 'Expected drawer to be inside the container');
659+
expect(contentIndex).toBeGreaterThan(-1, 'Expected content to be inside the container');
660+
expect(drawerIndex).toBeLessThan(contentIndex, 'Expected drawer to be before the content');
661+
});
662+
663+
it('should project end drawer after the content', () => {
664+
const fixture = TestBed.createComponent(BasicTestApp);
665+
fixture.componentInstance.position = 'end';
666+
fixture.detectChanges();
667+
668+
const allNodes = getDrawerNodesArray(fixture);
669+
const drawerIndex = allNodes.indexOf(fixture.nativeElement.querySelector('.mat-drawer'));
670+
const contentIndex =
671+
allNodes.indexOf(fixture.nativeElement.querySelector('.mat-drawer-content'));
672+
673+
expect(drawerIndex).toBeGreaterThan(-1, 'Expected drawer to be inside the container');
674+
expect(contentIndex).toBeGreaterThan(-1, 'Expected content to be inside the container');
675+
expect(drawerIndex).toBeGreaterThan(contentIndex, 'Expected drawer to be after the content');
676+
});
677+
678+
it('should move the drawer before/after the content when its position changes after being ' +
679+
'initialized at `start`', () => {
680+
const fixture = TestBed.createComponent(BasicTestApp);
681+
fixture.componentInstance.position = 'start';
682+
fixture.detectChanges();
683+
684+
const drawer = fixture.nativeElement.querySelector('.mat-drawer');
685+
const content = fixture.nativeElement.querySelector('.mat-drawer-content');
686+
687+
let allNodes = getDrawerNodesArray(fixture);
688+
const startDrawerIndex = allNodes.indexOf(drawer);
689+
const startContentIndex = allNodes.indexOf(content);
690+
691+
expect(startDrawerIndex).toBeGreaterThan(-1, 'Expected drawer to be inside the container');
692+
expect(startContentIndex)
693+
.toBeGreaterThan(-1, 'Expected content to be inside the container');
694+
expect(startDrawerIndex)
695+
.toBeLessThan(startContentIndex, 'Expected drawer to be before the content on init');
696+
697+
fixture.componentInstance.position = 'end';
698+
fixture.detectChanges();
699+
allNodes = getDrawerNodesArray(fixture);
700+
701+
expect(allNodes.indexOf(drawer)).toBeGreaterThan(allNodes.indexOf(content),
702+
'Expected drawer to be after content when position changes to `end`');
703+
704+
fixture.componentInstance.position = 'start';
705+
fixture.detectChanges();
706+
allNodes = getDrawerNodesArray(fixture);
707+
708+
expect(allNodes.indexOf(drawer)).toBeLessThan(allNodes.indexOf(content),
709+
'Expected drawer to be before content when position changes back to `start`');
710+
});
711+
712+
it('should move the drawer before/after the content when its position changes after being ' +
713+
'initialized at `end`', () => {
714+
const fixture = TestBed.createComponent(BasicTestApp);
715+
fixture.componentInstance.position = 'end';
716+
fixture.detectChanges();
717+
718+
const drawer = fixture.nativeElement.querySelector('.mat-drawer');
719+
const content = fixture.nativeElement.querySelector('.mat-drawer-content');
720+
721+
let allNodes = getDrawerNodesArray(fixture);
722+
const startDrawerIndex = allNodes.indexOf(drawer);
723+
const startContentIndex = allNodes.indexOf(content);
724+
725+
expect(startDrawerIndex).toBeGreaterThan(-1, 'Expected drawer to be inside the container');
726+
expect(startContentIndex)
727+
.toBeGreaterThan(-1, 'Expected content to be inside the container');
728+
expect(startDrawerIndex)
729+
.toBeGreaterThan(startContentIndex, 'Expected drawer to be after the content on init');
730+
731+
fixture.componentInstance.position = 'start';
732+
fixture.detectChanges();
733+
allNodes = getDrawerNodesArray(fixture);
734+
735+
expect(allNodes.indexOf(drawer)).toBeLessThan(allNodes.indexOf(content),
736+
'Expected drawer to be before content when position changes to `start`');
737+
738+
fixture.componentInstance.position = 'end';
739+
fixture.detectChanges();
740+
allNodes = getDrawerNodesArray(fixture);
741+
742+
expect(allNodes.indexOf(drawer)).toBeGreaterThan(allNodes.indexOf(content),
743+
'Expected drawer to be after content when position changes back to `end`');
744+
});
745+
746+
function getDrawerNodesArray(fixture: ComponentFixture<any>): HTMLElement[] {
747+
return Array.from(fixture.nativeElement.querySelector('.mat-drawer-container').childNodes);
748+
}
749+
750+
});
646751
});
647752

648753
describe('MatDrawerContainer', () => {
@@ -926,6 +1031,32 @@ describe('MatDrawerContainer', () => {
9261031
expect(spy).toHaveBeenCalled();
9271032
subscription.unsubscribe();
9281033
}));
1034+
1035+
it('should position the drawers before/after the content in the DOM based on their position',
1036+
fakeAsync(() => {
1037+
const fixture = TestBed.createComponent(DrawerContainerTwoDrawerTestApp);
1038+
fixture.detectChanges();
1039+
1040+
const drawerDebugElements = fixture.debugElement.queryAll(By.directive(MatDrawer));
1041+
const [start, end] = drawerDebugElements.map(el => el.componentInstance);
1042+
const [startNode, endNode] = drawerDebugElements.map(el => el.nativeElement);
1043+
const contentNode = fixture.nativeElement.querySelector('.mat-drawer-content');
1044+
const allNodes: HTMLElement[] =
1045+
Array.from(fixture.nativeElement.querySelector('.mat-drawer-container').childNodes);
1046+
const startIndex = allNodes.indexOf(startNode);
1047+
const endIndex = allNodes.indexOf(endNode);
1048+
const contentIndex = allNodes.indexOf(contentNode);
1049+
1050+
expect(start.position).toBe('start');
1051+
expect(end.position).toBe('end');
1052+
expect(contentIndex).toBeGreaterThan(-1, 'Expected content to be inside the container');
1053+
expect(startIndex).toBeGreaterThan(-1, 'Expected start drawer to be inside the container');
1054+
expect(endIndex).toBeGreaterThan(-1, 'Expected end drawer to be inside the container');
1055+
1056+
expect(startIndex).toBeLessThan(contentIndex, 'Expected start drawer to be before content');
1057+
expect(endIndex).toBeGreaterThan(contentIndex, 'Expected end drawer to be after content');
1058+
}));
1059+
9291060
});
9301061

9311062

@@ -949,7 +1080,7 @@ class DrawerContainerTwoDrawerTestApp {
9491080
@Component({
9501081
template: `
9511082
<mat-drawer-container (backdropClick)="backdropClicked()" [hasBackdrop]="hasBackdrop">
952-
<mat-drawer #drawer="matDrawer" position="start"
1083+
<mat-drawer #drawer="matDrawer" [position]="position"
9531084
(opened)="open()"
9541085
(openedStart)="openStart()"
9551086
(closed)="close()"
@@ -975,6 +1106,7 @@ class BasicTestApp {
9751106
closeStartCount = 0;
9761107
backdropClickedCount = 0;
9771108
hasBackdrop: boolean | null = null;
1109+
position = 'start';
9781110

9791111
@ViewChild('drawer') drawer: MatDrawer;
9801112
@ViewChild('drawerButton') drawerButton: ElementRef<HTMLButtonElement>;

src/material/sidenav/drawer.ts

+58-7
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import {
3737
ViewEncapsulation,
3838
HostListener,
3939
HostBinding,
40+
AfterViewInit,
4041
} from '@angular/core';
4142
import {fromEvent, merge, Observable, Subject} from 'rxjs';
4243
import {
@@ -137,20 +138,32 @@ export class MatDrawerContent extends CdkScrollable implements AfterContentInit
137138
changeDetection: ChangeDetectionStrategy.OnPush,
138139
encapsulation: ViewEncapsulation.None,
139140
})
140-
export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestroy {
141+
export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy {
141142
private _focusTrap: FocusTrap;
142143
private _elementFocusedBeforeDrawerWasOpened: HTMLElement | null = null;
144+
private _document: Document;
143145

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

149+
/** Whether the view of the component has been attached. */
150+
private _isAttached: boolean;
151+
152+
/** Anchor node used to restore the drawer to its initial position. */
153+
private _anchor: Comment | null;
154+
147155
/** The side that the drawer is attached to. */
148156
@Input()
149157
get position(): 'start' | 'end' { return this._position; }
150158
set position(value: 'start' | 'end') {
151159
// Make sure we have a valid value.
152160
value = value === 'end' ? 'end' : 'start';
153-
if (value != this._position) {
161+
if (value !== this._position) {
162+
// Static inputs in Ivy are set before the element is in the DOM.
163+
if (this._isAttached) {
164+
this._updatePositionInParent(value);
165+
}
166+
154167
this._position = value;
155168
this.onPositionChanged.emit();
156169
}
@@ -258,6 +271,9 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
258271
// tslint:disable-next-line:no-output-on-prefix
259272
@Output('positionChanged') onPositionChanged: EventEmitter<void> = new EventEmitter<void>();
260273

274+
/** Reference to the inner element that contains all the content. */
275+
@ViewChild('content') _content: ElementRef<HTMLElement>;
276+
261277
/**
262278
* An observable that emits when the drawer mode changes. This is used by the drawer container to
263279
* to know when to when the mode changes so it can adapt the margins on the content.
@@ -269,17 +285,18 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
269285
private _focusMonitor: FocusMonitor,
270286
private _platform: Platform,
271287
private _ngZone: NgZone,
272-
@Optional() @Inject(DOCUMENT) private _doc: any,
288+
@Optional() @Inject(DOCUMENT) _document: any,
273289
/**
274290
* @deprecated `_container` parameter to be made required.
275291
* @breaking-change 10.0.0
276292
*/
277293
@Optional() @Inject(MAT_DRAWER_CONTAINER) public _container?: MatDrawerContainer) {
278294

295+
this._document = _document;
279296
this.openedChange.subscribe((opened: boolean) => {
280297
if (opened) {
281-
if (this._doc) {
282-
this._elementFocusedBeforeDrawerWasOpened = this._doc.activeElement as HTMLElement;
298+
if (this._document) {
299+
this._elementFocusedBeforeDrawerWasOpened = this._document.activeElement as HTMLElement;
283300
}
284301

285302
this._takeFocus();
@@ -360,13 +377,20 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
360377

361378
/** Whether focus is currently within the drawer. */
362379
private _isFocusWithinDrawer(): boolean {
363-
const activeEl = this._doc?.activeElement;
380+
const activeEl = this._document.activeElement;
364381
return !!activeEl && this._elementRef.nativeElement.contains(activeEl);
365382
}
366383

367-
ngAfterContentInit() {
384+
ngAfterViewInit() {
385+
this._isAttached = true;
368386
this._focusTrap = this._focusTrapFactory.create(this._elementRef.nativeElement);
369387
this._updateFocusTrapState();
388+
389+
// Only update the DOM position when the sidenav is positioned at
390+
// the end since we project the sidenav before the content by default.
391+
if (this._position === 'end') {
392+
this._updatePositionInParent('end');
393+
}
370394
}
371395

372396
ngAfterContentChecked() {
@@ -384,6 +408,11 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
384408
this._focusTrap.destroy();
385409
}
386410

411+
if (this._anchor && this._anchor.parentNode) {
412+
this._anchor.parentNode.removeChild(this._anchor);
413+
}
414+
415+
this._anchor = null;
387416
this._animationStarted.complete();
388417
this._animationEnd.complete();
389418
this._modeChanged.complete();
@@ -467,6 +496,28 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
467496
}
468497
}
469498

499+
/**
500+
* Updates the position of the drawer in the DOM. We need to move the element around ourselves
501+
* when it's in the `end` position so that it comes after the content and the visual order
502+
* matches the tab order. We also need to be able to move it back to `start` if the sidenav
503+
* started off as `end` and was changed to `start`.
504+
*/
505+
private _updatePositionInParent(newPosition: 'start' | 'end') {
506+
const element = this._elementRef.nativeElement;
507+
const parent = element.parentNode!;
508+
509+
if (newPosition === 'end') {
510+
if (!this._anchor) {
511+
this._anchor = this._document.createComment('mat-drawer-anchor');
512+
parent.insertBefore(this._anchor, element);
513+
}
514+
515+
parent.appendChild(element);
516+
} else if (this._anchor) {
517+
this._anchor.parentNode!.insertBefore(element, this._anchor);
518+
}
519+
}
520+
470521
// We have to use a `HostListener` here in order to support both Ivy and ViewEngine.
471522
// In Ivy the `host` bindings will be merged when this class is extended, whereas in
472523
// ViewEngine they're overwritten.

tools/public_api_guard/material/sidenav.d.ts

+4-3
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@ export declare const MAT_DRAWER_DEFAULT_AUTOSIZE: InjectionToken<boolean>;
22

33
export declare function MAT_DRAWER_DEFAULT_AUTOSIZE_FACTORY(): boolean;
44

5-
export declare class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestroy {
5+
export declare class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy {
66
_animationEnd: Subject<AnimationEvent>;
77
_animationStarted: Subject<AnimationEvent>;
88
_animationState: 'open-instant' | 'open' | 'void';
99
get _closedStream(): Observable<void>;
1010
_container?: MatDrawerContainer | undefined;
11+
_content: ElementRef<HTMLElement>;
1112
readonly _modeChanged: Subject<void>;
1213
get _openedStream(): Observable<void>;
1314
get _width(): number;
@@ -25,14 +26,14 @@ export declare class MatDrawer implements AfterContentInit, AfterContentChecked,
2526
get openedStart(): Observable<void>;
2627
get position(): 'start' | 'end';
2728
set position(value: 'start' | 'end');
28-
constructor(_elementRef: ElementRef<HTMLElement>, _focusTrapFactory: FocusTrapFactory, _focusMonitor: FocusMonitor, _platform: Platform, _ngZone: NgZone, _doc: any,
29+
constructor(_elementRef: ElementRef<HTMLElement>, _focusTrapFactory: FocusTrapFactory, _focusMonitor: FocusMonitor, _platform: Platform, _ngZone: NgZone, _document: any,
2930
_container?: MatDrawerContainer | undefined);
3031
_animationDoneListener(event: AnimationEvent): void;
3132
_animationStartListener(event: AnimationEvent): void;
3233
_closeViaBackdropClick(): Promise<MatDrawerToggleResult>;
3334
close(): Promise<MatDrawerToggleResult>;
3435
ngAfterContentChecked(): void;
35-
ngAfterContentInit(): void;
36+
ngAfterViewInit(): void;
3637
ngOnDestroy(): void;
3738
open(openedVia?: FocusOrigin): Promise<MatDrawerToggleResult>;
3839
toggle(isOpen?: boolean, openedVia?: FocusOrigin): Promise<MatDrawerToggleResult>;

0 commit comments

Comments
 (0)