Skip to content

Commit 5e161c6

Browse files
committed
fix(material/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 8424209 commit 5e161c6

File tree

4 files changed

+190
-8
lines changed

4 files changed

+190
-8
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" cdkScrollable>
1+
<div class="mat-drawer-inner-container" cdkScrollable #content>
22
<ng-content></ng-content>
33
</div>

src/material/sidenav/drawer.spec.ts

+132-1
Original file line numberDiff line numberDiff line change
@@ -658,6 +658,110 @@ describe('MatDrawer', () => {
658658
expect(scrollable.getElementRef().nativeElement).toBe(content.nativeElement);
659659
});
660660

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

663767
describe('MatDrawerContainer', () => {
@@ -943,6 +1047,32 @@ describe('MatDrawerContainer', () => {
9431047
expect(spy).toHaveBeenCalled();
9441048
subscription.unsubscribe();
9451049
}));
1050+
1051+
it('should position the drawers before/after the content in the DOM based on their position',
1052+
fakeAsync(() => {
1053+
const fixture = TestBed.createComponent(DrawerContainerTwoDrawerTestApp);
1054+
fixture.detectChanges();
1055+
1056+
const drawerDebugElements = fixture.debugElement.queryAll(By.directive(MatDrawer));
1057+
const [start, end] = drawerDebugElements.map(el => el.componentInstance);
1058+
const [startNode, endNode] = drawerDebugElements.map(el => el.nativeElement);
1059+
const contentNode = fixture.nativeElement.querySelector('.mat-drawer-content');
1060+
const allNodes: HTMLElement[] =
1061+
Array.from(fixture.nativeElement.querySelector('.mat-drawer-container').childNodes);
1062+
const startIndex = allNodes.indexOf(startNode);
1063+
const endIndex = allNodes.indexOf(endNode);
1064+
const contentIndex = allNodes.indexOf(contentNode);
1065+
1066+
expect(start.position).toBe('start');
1067+
expect(end.position).toBe('end');
1068+
expect(contentIndex).toBeGreaterThan(-1, 'Expected content to be inside the container');
1069+
expect(startIndex).toBeGreaterThan(-1, 'Expected start drawer to be inside the container');
1070+
expect(endIndex).toBeGreaterThan(-1, 'Expected end drawer to be inside the container');
1071+
1072+
expect(startIndex).toBeLessThan(contentIndex, 'Expected start drawer to be before content');
1073+
expect(endIndex).toBeGreaterThan(contentIndex, 'Expected end drawer to be after content');
1074+
}));
1075+
9461076
});
9471077

9481078

@@ -966,7 +1096,7 @@ class DrawerContainerTwoDrawerTestApp {
9661096
@Component({
9671097
template: `
9681098
<mat-drawer-container (backdropClick)="backdropClicked()" [hasBackdrop]="hasBackdrop">
969-
<mat-drawer #drawer="matDrawer" position="start"
1099+
<mat-drawer #drawer="matDrawer" [position]="position"
9701100
(opened)="open()"
9711101
(openedStart)="openStart()"
9721102
(closed)="close()"
@@ -992,6 +1122,7 @@ class BasicTestApp {
9921122
closeStartCount = 0;
9931123
backdropClickedCount = 0;
9941124
hasBackdrop: boolean | null = null;
1125+
position = 'start';
9951126

9961127
@ViewChild('drawer') drawer: MatDrawer;
9971128
@ViewChild('drawerButton') drawerButton: ElementRef<HTMLButtonElement>;

src/material/sidenav/drawer.ts

+53-4
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ import {
4343
ViewEncapsulation,
4444
HostListener,
4545
HostBinding,
46+
AfterViewInit,
4647
} from '@angular/core';
4748
import {fromEvent, merge, Observable, Subject} from 'rxjs';
4849
import {
@@ -146,20 +147,31 @@ export class MatDrawerContent extends CdkScrollable implements AfterContentInit
146147
changeDetection: ChangeDetectionStrategy.OnPush,
147148
encapsulation: ViewEncapsulation.None,
148149
})
149-
export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestroy {
150+
export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy {
150151
private _focusTrap: FocusTrap;
151152
private _elementFocusedBeforeDrawerWasOpened: HTMLElement | null = null;
152153

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

157+
/** Whether the view of the component has been attached. */
158+
private _isAttached: boolean;
159+
160+
/** Anchor node used to restore the drawer to its initial position. */
161+
private _anchor: Comment | null;
162+
156163
/** The side that the drawer is attached to. */
157164
@Input()
158165
get position(): 'start' | 'end' { return this._position; }
159166
set position(value: 'start' | 'end') {
160167
// Make sure we have a valid value.
161168
value = value === 'end' ? 'end' : 'start';
162-
if (value != this._position) {
169+
if (value !== this._position) {
170+
// Static inputs in Ivy are set before the element is in the DOM.
171+
if (this._isAttached) {
172+
this._updatePositionInParent(value);
173+
}
174+
163175
this._position = value;
164176
this.onPositionChanged.emit();
165177
}
@@ -273,6 +285,9 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
273285
// tslint:disable-next-line:no-output-on-prefix
274286
@Output('positionChanged') readonly onPositionChanged = new EventEmitter<void>();
275287

288+
/** Reference to the inner element that contains all the content. */
289+
@ViewChild('content') _content: ElementRef<HTMLElement>;
290+
276291
/**
277292
* An observable that emits when the drawer mode changes. This is used by the drawer container to
278293
* to know when to when the mode changes so it can adapt the margins on the content.
@@ -418,13 +433,20 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
418433

419434
/** Whether focus is currently within the drawer. */
420435
private _isFocusWithinDrawer(): boolean {
421-
const activeEl = this._doc?.activeElement;
436+
const activeEl = this._doc.activeElement;
422437
return !!activeEl && this._elementRef.nativeElement.contains(activeEl);
423438
}
424439

425-
ngAfterContentInit() {
440+
ngAfterViewInit() {
441+
this._isAttached = true;
426442
this._focusTrap = this._focusTrapFactory.create(this._elementRef.nativeElement);
427443
this._updateFocusTrapState();
444+
445+
// Only update the DOM position when the sidenav is positioned at
446+
// the end since we project the sidenav before the content by default.
447+
if (this._position === 'end') {
448+
this._updatePositionInParent('end');
449+
}
428450
}
429451

430452
ngAfterContentChecked() {
@@ -442,6 +464,11 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
442464
this._focusTrap.destroy();
443465
}
444466

467+
if (this._anchor && this._anchor.parentNode) {
468+
this._anchor.parentNode.removeChild(this._anchor);
469+
}
470+
471+
this._anchor = null;
445472
this._animationStarted.complete();
446473
this._animationEnd.complete();
447474
this._modeChanged.complete();
@@ -525,6 +552,28 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
525552
}
526553
}
527554

555+
/**
556+
* Updates the position of the drawer in the DOM. We need to move the element around ourselves
557+
* when it's in the `end` position so that it comes after the content and the visual order
558+
* matches the tab order. We also need to be able to move it back to `start` if the sidenav
559+
* started off as `end` and was changed to `start`.
560+
*/
561+
private _updatePositionInParent(newPosition: 'start' | 'end') {
562+
const element = this._elementRef.nativeElement;
563+
const parent = element.parentNode!;
564+
565+
if (newPosition === 'end') {
566+
if (!this._anchor) {
567+
this._anchor = this._doc.createComment('mat-drawer-anchor')!;
568+
parent.insertBefore(this._anchor!, element);
569+
}
570+
571+
parent.appendChild(element);
572+
} else if (this._anchor) {
573+
this._anchor.parentNode!.insertBefore(element, this._anchor);
574+
}
575+
}
576+
528577
// We have to use a `HostListener` here in order to support both Ivy and ViewEngine.
529578
// In Ivy the `host` bindings will be merged when this class is extended, whereas in
530579
// ViewEngine they're overwritten.

tools/public_api_guard/material/sidenav.md

+4-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import { AfterContentChecked } from '@angular/core';
88
import { AfterContentInit } from '@angular/core';
9+
import { AfterViewInit } from '@angular/core';
910
import { AnimationEvent as AnimationEvent_2 } from '@angular/animations';
1011
import { AnimationTriggerMetadata } from '@angular/animations';
1112
import { BooleanInput } from '@angular/cdk/coercion';
@@ -48,7 +49,7 @@ export const MAT_DRAWER_DEFAULT_AUTOSIZE: InjectionToken<boolean>;
4849
export function MAT_DRAWER_DEFAULT_AUTOSIZE_FACTORY(): boolean;
4950

5051
// @public
51-
export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestroy {
52+
export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy {
5253
constructor(_elementRef: ElementRef<HTMLElement>, _focusTrapFactory: FocusTrapFactory, _focusMonitor: FocusMonitor, _platform: Platform, _ngZone: NgZone, _interactivityChecker: InteractivityChecker, _doc: any, _container?: MatDrawerContainer | undefined);
5354
// (undocumented)
5455
_animationDoneListener(event: AnimationEvent_2): void;
@@ -65,6 +66,7 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
6566
_closeViaBackdropClick(): Promise<MatDrawerToggleResult>;
6667
// (undocumented)
6768
_container?: MatDrawerContainer | undefined;
69+
_content: ElementRef<HTMLElement>;
6870
get disableClose(): boolean;
6971
set disableClose(value: boolean);
7072
// (undocumented)
@@ -81,7 +83,7 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr
8183
// (undocumented)
8284
ngAfterContentChecked(): void;
8385
// (undocumented)
84-
ngAfterContentInit(): void;
86+
ngAfterViewInit(): void;
8587
// (undocumented)
8688
ngOnDestroy(): void;
8789
readonly onPositionChanged: EventEmitter<void>;

0 commit comments

Comments
 (0)