Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(a11y): focus monitor not checking children if monitor is called multiple times with different parameters #19237

Merged
merged 1 commit into from
May 13, 2020
Merged

fix(a11y): focus monitor not checking children if monitor is called multiple times with different parameters #19237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/cdk/a11y/focus-monitor/focus-monitor.spec.ts
View file
Open in desktop
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
12 changes: 9 additions & 3 deletions src/cdk/a11y/focus-monitor/focus-monitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

@crisbeto crisbeto May 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmalerba I think that this will work for most of the cases, but I'm not sure whether it's ideal. This means that the initial subscriber that signed up only for focus events on the element will now get emissions from children as well. Ideally, we'd be keeping track of two separate streams: own focus and child focus, but I didn't do it in this PR, because there are some issues with having two streams:

  1. There can be a conflict between the CSS focus classes, because both the checkChildren and the non-checkChildren calls are trying to set classes on the same element.
  2. We'd need a way to match focus either to the own focus stream or the child focus stream. Matching it is easy, but it can have performance implications, because we'd have to loop through and check all of the monitored elements on each focus and blur event.
  3. We'd have to introduce a checkChildren parameter on stopMonitoring so that the two streams can be stopped independently. This would either have to be breaking API change, or we'd introduce memory leaks in people's code, because their stopMonitoring calls won't work anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok those are some good points, especially #⁠1. Can you file a design issue with the above and link it in a TODO? This PR seems like the best option for the moment, but it would be good to have that documented

}

return cachedInfo.subject.asObservable();
}

Expand Down