fix(a11y): focus monitor not working inside shadow dom #19135
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In angular#18667 event delegation was implemented for the `FocusMonitor` which is based around binding a single event on the `document`. The problem is that the browser won't invoke the `focus` and `blur` handlers on the `document`, if focus is moved within the same shadow root. These changes switch to delegating the events either through the closest shadow root or the `document`. Fixes angular#19133.
crisbeto
added
P2
googlebot
added
the
cla: yes
crisbeto
added
Accessibility
|
|
googlebot
added
the
cla: yes
mmalerba
reviewed
Apr 21, 2020
| * handlers differently from the rest of the events, because the browser won't emit events | ||
| * to the document when focus moves inside of a shadow root. | ||
| */ | ||
| private _rootNodeFocusListenerCount = new Map<HTMLElement|Document, number>(); |
There was a problem hiding this comment.
To be safe, we should scan through this in ngOnDestroy and unlisten to any remaining entries
There was a problem hiding this comment.
We're looping through and calling stopMonitoring on all the remaining elements which eventually empties this out as well.
| cachedInfo!.checkChildren = checkChildren; | ||
| return cachedInfo!.subject.asObservable(); | ||
| const cachedInfo = this._elementInfo.get(nativeElement)!; | ||
| cachedInfo.checkChildren = checkChildren; |
There was a problem hiding this comment.
I see that you didn't change this, but is this right? Won't this cause all previous calls to monitor on this element to behave according to the checkChildren parameter on the current call?
There was a problem hiding this comment.
Judging by how it's written, I think that's the point. E.g. you could turn a shallow watch into a deep one by calling monitor again with checkChildren. We could separate it out and have 2 separate streams, but we'd have to make a cache key out of the options.
There was a problem hiding this comment.
Yeah, I'm not sure it should work that way. The main way I see it being called twice is if there's e.g. a directive and a component that need to know about focus state on the same element. They might have different requirements about whether to include the children, and we wouldn't want them to affect each other's subscriptions.
So it seems like a bug, but a pre-existing one and I haven't seen anyone complain about it yet, so I think its safe to leave this for another PR.
jelbourn
pushed a commit
that referenced
this pull request
Apr 24, 2020
In #18667 event delegation was implemented for the `FocusMonitor` which is based around binding a single event on the `document`. The problem is that the browser won't invoke the `focus` and `blur` handlers on the `document`, if focus is moved within the same shadow root. These changes switch to delegating the events either through the closest shadow root or the `document`. Fixes #19133.
soro-google
pushed a commit
to soro-google/components
that referenced
this pull request
Apr 24, 2020
In angular#18667 event delegation was implemented for the `FocusMonitor` which is based around binding a single event on the `document`. The problem is that the browser won't invoke the `focus` and `blur` handlers on the `document`, if focus is moved within the same shadow root. These changes switch to delegating the events either through the closest shadow root or the `document`. Fixes angular#19133.
crisbeto
added a commit
to crisbeto/material2
that referenced
this pull request
May 3, 2020
…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 angular#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 angular#19218.
crisbeto
added a commit
to crisbeto/material2
that referenced
this pull request
May 5, 2020
…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 angular#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 angular#19218.
mmalerba
pushed a commit
that referenced
this pull request
May 13, 2020
…ultiple 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.
mmalerba
pushed a commit
that referenced
this pull request
May 15, 2020
…ultiple 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.
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #18667 event delegation was implemented for the
FocusMonitorwhich is based around binding a single event on thedocument. The problem is that the browser won't invoke thefocusandblurhandlers on thedocument, if focus is moved within the same shadow root. These changes switch to delegating the events either through the closest shadow root or thedocument.Fixes #19133.
Note: I spent some time trying to reproduce this in a unit test, but I wasn't able to because the way we simulate focus programmatically isn't how things behave in the browser, and we have to use programmatic focus in order to avoid flakes on the CI.