From 07711b45d947c32989f044f13904ad8ecf83345c Mon Sep 17 00:00:00 2001
From: Kristiyan Kostadinov <crisbeto@users.noreply.github.com>
Date: Wed, 13 May 2020 04:10:23 +0200
Subject: [PATCH] fix(a11y): focus monitor not checking children if monitor is
 called multiple times with different parameters (#19237)

With the current logic, if an element is being monitored with `checkChildren` set to `true` and then we start monitoring it with `false`, the initial monitor call will stop checking the children, because we take the `checkChildren` value from the most-recent call. This is something that we discussed in https://github.com/angular/components/pull/19135#discussion_r412471591, but now we have a case where it can happen: setting a tooltip on a sort header. The sort header looks for child focus, whereas the tooltip doesn't, causing the sort focus indication to break.

These changes fix the issue by treating all focus as `checkChildren`, as soon as there's at least one call to `monitor` with `checkChildren` turned on.

Fixes #19218.
---
 .../a11y/focus-monitor/focus-monitor.spec.ts  | 19 ++++++++++++++++++-
 src/cdk/a11y/focus-monitor/focus-monitor.ts   | 12 +++++++++---
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/src/cdk/a11y/focus-monitor/focus-monitor.spec.ts b/src/cdk/a11y/focus-monitor/focus-monitor.spec.ts
index a3350ba97dc2..1611aefefc94 100644
--- a/src/cdk/a11y/focus-monitor/focus-monitor.spec.ts
+++ b/src/cdk/a11y/focus-monitor/focus-monitor.spec.ts
@@ -277,6 +277,23 @@ describe('FocusMonitor', () => {
        // After 2 ticks, the timeout has cleared the origin. Default is 'program'.
        expect(changeHandler).toHaveBeenCalledWith('program');
      }));
+
+  it('should check children if monitor was called with different checkChildren', fakeAsync(() => {
+    const parent = fixture.nativeElement.querySelector('.parent');
+
+    focusMonitor.monitor(parent, true);
+    focusMonitor.monitor(parent, false);
+
+    // Simulate focus via mouse.
+    dispatchMouseEvent(buttonElement, 'mousedown');
+    buttonElement.focus();
+    fixture.detectChanges();
+    flush();
+
+    expect(parent.classList).toContain('cdk-focused');
+    expect(parent.classList).toContain('cdk-mouse-focused');
+  }));
+
 });
 
 describe('FocusMonitor with "eventual" detection', () => {
@@ -569,7 +586,7 @@ describe('FocusMonitor observable stream', () => {
 
 
 @Component({
-  template: `<button>focus me!</button>`
+  template: `<div class="parent"><button>focus me!</button></div>`
 })
 class PlainButton {}
 
diff --git a/src/cdk/a11y/focus-monitor/focus-monitor.ts b/src/cdk/a11y/focus-monitor/focus-monitor.ts
index 3e42634ec91f..8688bdac29d9 100644
--- a/src/cdk/a11y/focus-monitor/focus-monitor.ts
+++ b/src/cdk/a11y/focus-monitor/focus-monitor.ts
@@ -235,11 +235,17 @@ export class FocusMonitor implements OnDestroy {
     // the shadow root, rather than the `document`, because the browser won't emit focus events
     // to the `document`, if focus is moving within the same shadow root.
     const rootNode = (_getShadowRoot(nativeElement) as HTMLElement|null) || this._getDocument();
+    const cachedInfo = this._elementInfo.get(nativeElement);
 
     // Check if we're already monitoring this element.
-    if (this._elementInfo.has(nativeElement)) {
-      const cachedInfo = this._elementInfo.get(nativeElement)!;
-      cachedInfo.checkChildren = checkChildren;
+    if (cachedInfo) {
+      if (checkChildren) {
+        // TODO(COMP-318): this can be problematic, because it'll turn all non-checkChildren
+        // observers into ones that behave as if `checkChildren` was turned on. We need a more
+        // robust solution.
+        cachedInfo.checkChildren = true;
+      }
+
       return cachedInfo.subject.asObservable();
     }