From 3bb9bd4ed6a2ec96ea2325ed19b2632c018ec852 Mon Sep 17 00:00:00 2001
From: Kristiyan Kostadinov <crisbeto@users.noreply.github.com>
Date: Fri, 10 Sep 2021 14:03:33 +0200
Subject: [PATCH] fix(material/stepper): incorrect navigation order when steps
 are added later on (#23541)

The Material stepper's template is set up such that we have a single `ng-template` at the bottom declaring the header which is then rendered out with an `ngTemplateOutlet` inside an `ngFor`. We do this in order to reduce duplication, because a lot of properties and attributes have to be set on `mat-step-header`.

Then problem with this approach is that because the header is outside the `ngFor`, it may not appear in the correct order in the `ngFor` if the list changes. This results in incorrect navigation order when using the next/previous buttons or the keyboard.

These changes add some logic that will sort the headers based on their position in the DOM in order to work around the issue. Alternatively, we could fix this by inlining the `mat-step-header` template, but that'll result in some duplicated code that is somewhat error-prone if we ever need to add more attributes.

Fixes #23539.

(cherry picked from commit 741a57ecf08baa53c4ab1a0a9107f2f94b1da785)
---
 src/cdk/stepper/stepper.ts           | 27 ++++++++++++++++++++++++++-
 src/material/stepper/stepper.spec.ts | 23 +++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/src/cdk/stepper/stepper.ts b/src/cdk/stepper/stepper.ts
index 70164d14e1f3..7f555569dae5 100644
--- a/src/cdk/stepper/stepper.ts
+++ b/src/cdk/stepper/stepper.ts
@@ -272,6 +272,9 @@ export class CdkStepper implements AfterContentInit, AfterViewInit, OnDestroy {
   /** The list of step headers of the steps in the stepper. */
   @ContentChildren(CdkStepHeader, {descendants: true}) _stepHeader: QueryList<CdkStepHeader>;
 
+  /** List of step headers sorted based on their DOM order. */
+  private _sortedHeaders = new QueryList<CdkStepHeader>();
+
   /** Whether the validity of previous steps should be checked or not. */
   @Input()
   get linear(): boolean {
@@ -362,10 +365,31 @@ export class CdkStepper implements AfterContentInit, AfterViewInit, OnDestroy {
   }
 
   ngAfterViewInit() {
+    // If the step headers are defined outside of the `ngFor` that renders the steps, like in the
+    // Material stepper, they won't appear in the `QueryList` in the same order as they're
+    // rendered in the DOM which will lead to incorrect keyboard navigation. We need to sort
+    // them manually to ensure that they're correct. Alternatively, we can change the Material
+    // template to inline the headers in the `ngFor`, but that'll result in a lot of
+    // code duplciation. See #23539.
+    this._stepHeader.changes
+      .pipe(startWith(this._stepHeader), takeUntil(this._destroyed))
+      .subscribe((headers: QueryList<CdkStepHeader>) => {
+        this._sortedHeaders.reset(headers.toArray().sort((a, b) => {
+          const documentPosition =
+            a._elementRef.nativeElement.compareDocumentPosition(b._elementRef.nativeElement);
+
+          // `compareDocumentPosition` returns a bitmask so we have to use a bitwise operator.
+          // https://developer.mozilla.org/en-US/docs/Web/API/Node/compareDocumentPosition
+          // tslint:disable-next-line:no-bitwise
+          return documentPosition & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1;
+        }));
+        this._sortedHeaders.notifyOnChanges();
+      });
+
     // Note that while the step headers are content children by default, any components that
     // extend this one might have them as view children. We initialize the keyboard handling in
     // AfterViewInit so we're guaranteed for both view and content children to be defined.
-    this._keyManager = new FocusKeyManager<FocusableOption>(this._stepHeader)
+    this._keyManager = new FocusKeyManager<FocusableOption>(this._sortedHeaders)
                            .withWrap()
                            .withHomeAndEnd()
                            .withVerticalOrientation(this._orientation === 'vertical');
@@ -393,6 +417,7 @@ export class CdkStepper implements AfterContentInit, AfterViewInit, OnDestroy {
 
   ngOnDestroy() {
     this.steps.destroy();
+    this._sortedHeaders.destroy();
     this._destroyed.next();
     this._destroyed.complete();
   }
diff --git a/src/material/stepper/stepper.spec.ts b/src/material/stepper/stepper.spec.ts
index b7e79c36fc75..b04082e1699f 100644
--- a/src/material/stepper/stepper.spec.ts
+++ b/src/material/stepper/stepper.spec.ts
@@ -1019,6 +1019,16 @@ describe('MatStepper', () => {
       assertArrowKeyInteractionInRtl(fixture, stepHeaders);
     });
 
+    it('should maintain the correct navigation order when a step is added later on', () => {
+      const fixture = createComponent(HorizontalStepperWithDelayedStep);
+      fixture.detectChanges();
+      fixture.componentInstance.renderSecondStep = true;
+      fixture.detectChanges();
+
+      const stepHeaders = fixture.debugElement.queryAll(By.css('.mat-horizontal-stepper-header'));
+      assertCorrectKeyboardInteraction(fixture, stepHeaders, 'horizontal');
+    });
+
     it('should reverse arrow key focus when switching into RTL after init', () => {
       const fixture = createComponent(SimpleMatHorizontalStepperApp);
       fixture.detectChanges();
@@ -2032,3 +2042,16 @@ class StepperWithStaticOutOfBoundsIndex {
 class StepperWithLazyContent {
   selectedIndex = 0;
 }
+
+@Component({
+  template: `
+    <mat-stepper>
+      <mat-step label="Step 1">Content 1</mat-step>
+      <mat-step label="Step 2" *ngIf="renderSecondStep">Content 2</mat-step>
+      <mat-step label="Step 3">Content 3</mat-step>
+    </mat-stepper>
+  `
+})
+class HorizontalStepperWithDelayedStep {
+  renderSecondStep = false;
+}