Skip to content

Commit fe87bda

Browse files
authored
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 #19135 (comment), 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.
1 parent 9377b49 commit fe87bda

File tree

2 files changed

+27
-4
lines changed

2 files changed

+27
-4
lines changed

src/cdk/a11y/focus-monitor/focus-monitor.spec.ts

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,23 @@ describe('FocusMonitor', () => {
277277
// After 2 ticks, the timeout has cleared the origin. Default is 'program'.
278278
expect(changeHandler).toHaveBeenCalledWith('program');
279279
}));
280+
281+
it('should check children if monitor was called with different checkChildren', fakeAsync(() => {
282+
const parent = fixture.nativeElement.querySelector('.parent');
283+
284+
focusMonitor.monitor(parent, true);
285+
focusMonitor.monitor(parent, false);
286+
287+
// Simulate focus via mouse.
288+
dispatchMouseEvent(buttonElement, 'mousedown');
289+
buttonElement.focus();
290+
fixture.detectChanges();
291+
flush();
292+
293+
expect(parent.classList).toContain('cdk-focused');
294+
expect(parent.classList).toContain('cdk-mouse-focused');
295+
}));
296+
280297
});
281298

282299
describe('FocusMonitor with "eventual" detection', () => {
@@ -569,7 +586,7 @@ describe('FocusMonitor observable stream', () => {
569586

570587

571588
@Component({
572-
template: `<button>focus me!</button>`
589+
template: `<div class="parent"><button>focus me!</button></div>`
573590
})
574591
class PlainButton {}
575592

src/cdk/a11y/focus-monitor/focus-monitor.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -235,11 +235,17 @@ export class FocusMonitor implements OnDestroy {
235235
// the shadow root, rather than the `document`, because the browser won't emit focus events
236236
// to the `document`, if focus is moving within the same shadow root.
237237
const rootNode = (_getShadowRoot(nativeElement) as HTMLElement|null) || this._getDocument();
238+
const cachedInfo = this._elementInfo.get(nativeElement);
238239

239240
// Check if we're already monitoring this element.
240-
if (this._elementInfo.has(nativeElement)) {
241-
const cachedInfo = this._elementInfo.get(nativeElement)!;
242-
cachedInfo.checkChildren = checkChildren;
241+
if (cachedInfo) {
242+
if (checkChildren) {
243+
// TODO(COMP-318): this can be problematic, because it'll turn all non-checkChildren
244+
// observers into ones that behave as if `checkChildren` was turned on. We need a more
245+
// robust solution.
246+
cachedInfo.checkChildren = true;
247+
}
248+
243249
return cachedInfo.subject.asObservable();
244250
}
245251

0 commit comments

Comments
 (0)