fix(focus-monitor): cleanup global listeners and don't require Renderer2 #7728
Conversation
mmalerba
requested review from
amcdnl,
andrewseguin,
devversion,
josephperrott and
tinayuangao
as code owners
googlebot
added
the
cla: yes
src/cdk/a11y/focus-monitor.spec.ts
Outdated
| spyOn(window, 'addEventListener').and.callThrough(); | ||
| spyOn(window, 'removeEventListener').and.callThrough(); | ||
| spyOn(document, 'addEventListener').and.callThrough(); | ||
| spyOn(document, 'removeEventListener').and.callThrough(); |
There was a problem hiding this comment.
This will permanently make these things spies for the rest of the test run; I'm not sure there's a good way to test global listener deregistration.
There was a problem hiding this comment.
I believe they are automatically cleaned up at the end of the describe block. There's some discussion about it here: jasmine/jasmine#1281
src/cdk/a11y/focus-monitor.ts
Outdated
| private _unregisterGlobalListeners = () => {}; | ||
|
|
||
| /** The number of elements currently being monitored. */ | ||
| private get _monitoredElementCount() { |
There was a problem hiding this comment.
Since this is private, use functions instead of a getter/setter?
| renderer: Renderer2 | boolean, | ||
| checkChildren?: boolean): Observable<FocusOrigin> { | ||
| // TODO(mmalerba): clean up after deprecated signature is removed. | ||
| if (!(renderer instanceof Renderer2)) { |
There was a problem hiding this comment.
Can you add this to the google cleanups tracker?
|
comments addressed |
jelbourn
added
pr: lgtm
action: merge
|
Needs rebase. |
andrewseguin
added
pr: needs rebase
and removed
action: merge
mmalerba
added
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
fixes #7609