From eae6680b04c5eee1148d3739c1dd08ea67362c58 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 15 Oct 2020 21:44:09 +0200 Subject: [PATCH] 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 Content ``` And this is what I've changed it to: ```html Content +
diff --git a/src/material/sidenav/drawer.spec.ts b/src/material/sidenav/drawer.spec.ts index 5417122de04e..8348b357a352 100644 --- a/src/material/sidenav/drawer.spec.ts +++ b/src/material/sidenav/drawer.spec.ts @@ -661,6 +661,144 @@ describe('MatDrawer', () => { expect(scrollable).toBeTruthy(); expect(scrollable.getElementRef().nativeElement).toBe(content.nativeElement); }); + + 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) + .withContext('Expected drawer to be inside the container') + .toBeGreaterThan(-1); + expect(contentIndex) + .withContext('Expected content to be inside the container') + .toBeGreaterThan(-1); + expect(drawerIndex) + .withContext('Expected drawer to be before the content') + .toBeLessThan(contentIndex); + }); + + 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) + .withContext('Expected drawer to be inside the container') + .toBeGreaterThan(-1); + expect(contentIndex) + .withContext('Expected content to be inside the container') + .toBeGreaterThan(-1); + expect(drawerIndex) + .withContext('Expected drawer to be after the content') + .toBeGreaterThan(contentIndex); + }); + + 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) + .withContext('Expected drawer to be inside the container') + .toBeGreaterThan(-1); + expect(startContentIndex) + .withContext('Expected content to be inside the container') + .toBeGreaterThan(-1); + expect(startDrawerIndex) + .withContext('Expected drawer to be before the content on init') + .toBeLessThan(startContentIndex); + + fixture.componentInstance.position = 'end'; + fixture.detectChanges(); + allNodes = getDrawerNodesArray(fixture); + + expect(allNodes.indexOf(drawer)) + .withContext('Expected drawer to be after content when position changes to `end`') + .toBeGreaterThan(allNodes.indexOf(content)); + + fixture.componentInstance.position = 'start'; + fixture.detectChanges(); + allNodes = getDrawerNodesArray(fixture); + + expect(allNodes.indexOf(drawer)) + .withContext('Expected drawer to be before content when position changes back to `start`') + .toBeLessThan(allNodes.indexOf(content)); + }, + ); + + 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): HTMLElement[] { + return Array.from(fixture.nativeElement.querySelector('.mat-drawer-container').childNodes); + } + }); }); describe('MatDrawerContainer', () => { @@ -949,6 +1087,41 @@ 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) + .withContext('Expected content to be inside the container') + .toBeGreaterThan(-1); + expect(startIndex) + .withContext('Expected start drawer to be inside the container') + .toBeGreaterThan(-1); + expect(endIndex) + .withContext('Expected end drawer to be inside the container') + .toBeGreaterThan(-1); + + expect(startIndex) + .withContext('Expected start drawer to be before content') + .toBeLessThan(contentIndex); + expect(endIndex) + .withContext('Expected end drawer to be after content') + .toBeGreaterThan(contentIndex); + })); }); /** Test component that contains an MatDrawerContainer but no MatDrawer. */ @@ -971,7 +1144,7 @@ class DrawerContainerTwoDrawerTestApp { @Component({ template: ` - ; diff --git a/src/material/sidenav/drawer.ts b/src/material/sidenav/drawer.ts index 1fc21f92193c..068c16a8d5eb 100644 --- a/src/material/sidenav/drawer.ts +++ b/src/material/sidenav/drawer.ts @@ -22,6 +22,7 @@ import {DOCUMENT} from '@angular/common'; import { AfterContentChecked, AfterContentInit, + AfterViewInit, ChangeDetectionStrategy, ChangeDetectorRef, Component, @@ -153,13 +154,19 @@ 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; /** 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' { @@ -168,7 +175,12 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr 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(); } @@ -293,6 +305,9 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr // tslint:disable-next-line:no-output-on-prefix @Output('positionChanged') readonly onPositionChanged = new EventEmitter(); + /** Reference to the inner element that contains all the content. */ + @ViewChild('content') _content: ElementRef; + /** * 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. @@ -448,13 +463,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._doc.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() { @@ -472,6 +494,8 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr this._focusTrap.destroy(); } + this._anchor?.remove(); + this._anchor = null; this._animationStarted.complete(); this._animationEnd.complete(); this._modeChanged.complete(); @@ -567,6 +591,28 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr this._focusTrap.enabled = this.opened && this.mode !== 'side'; } } + + /** + * 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._doc.createComment('mat-drawer-anchor')!; + parent.insertBefore(this._anchor!, element); + } + + parent.appendChild(element); + } else if (this._anchor) { + this._anchor.parentNode!.insertBefore(element, this._anchor); + } + } } /** diff --git a/tools/public_api_guard/material/sidenav.md b/tools/public_api_guard/material/sidenav.md index 85762e2a76ef..8e4cc8e41f00 100644 --- a/tools/public_api_guard/material/sidenav.md +++ b/tools/public_api_guard/material/sidenav.md @@ -6,6 +6,7 @@ import { AfterContentChecked } from '@angular/core'; import { AfterContentInit } from '@angular/core'; +import { AfterViewInit } from '@angular/core'; import { AnimationEvent as AnimationEvent_2 } from '@angular/animations'; import { AnimationTriggerMetadata } from '@angular/animations'; import { BooleanInput } from '@angular/cdk/coercion'; @@ -48,7 +49,7 @@ export const MAT_DRAWER_DEFAULT_AUTOSIZE: InjectionToken; export function MAT_DRAWER_DEFAULT_AUTOSIZE_FACTORY(): boolean; // @public -export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestroy { +export class MatDrawer implements AfterViewInit, AfterContentChecked, OnDestroy { constructor(_elementRef: ElementRef, _focusTrapFactory: FocusTrapFactory, _focusMonitor: FocusMonitor, _platform: Platform, _ngZone: NgZone, _interactivityChecker: InteractivityChecker, _doc: any, _container?: MatDrawerContainer | undefined); readonly _animationEnd: Subject; readonly _animationStarted: Subject; @@ -61,6 +62,7 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr _closeViaBackdropClick(): Promise; // (undocumented) _container?: MatDrawerContainer | undefined; + _content: ElementRef; get disableClose(): boolean; set disableClose(value: BooleanInput); // (undocumented) @@ -71,7 +73,7 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr // (undocumented) ngAfterContentChecked(): void; // (undocumented) - ngAfterContentInit(): void; + ngAfterViewInit(): void; // (undocumented) ngOnDestroy(): void; readonly onPositionChanged: EventEmitter;