Skip to content

Commit

Permalink
fix(a11y): focus monitor not checking children if monitor is called m…
Browse files Browse the repository at this point in the history
…ultiple times with different parameters

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.
  • Loading branch information
crisbeto committed May 3, 2020
1 parent 658896f commit d3eecb1
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 4 deletions.
19 changes: 18 additions & 1 deletion src/cdk/a11y/focus-monitor/focus-monitor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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 {}

Expand Down
9 changes: 6 additions & 3 deletions src/cdk/a11y/focus-monitor/focus-monitor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,11 +235,14 @@ 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) {
cachedInfo.checkChildren = true;
}

return cachedInfo.subject.asObservable();
}

Expand Down

0 comments on commit d3eecb1

Please sign in to comment.