fix(a11y): focus monitor not checking children if monitor is called multiple times with different parameters #19237
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
crisbeto
added
P3
googlebot
added
the
cla: yes
crisbeto
commented
May 3, 2020
| cachedInfo.checkChildren = checkChildren; | ||
| if (cachedInfo) { | ||
| if (checkChildren) { | ||
| cachedInfo.checkChildren = true; |
There was a problem hiding this comment.
@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:
- There can be a conflict between the CSS focus classes, because both the
checkChildrenand the non-checkChildrencalls are trying to set classes on the same element. - 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
focusandblurevent. - We'd have to introduce a
checkChildrenparameter onstopMonitoringso 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 theirstopMonitoringcalls won't work anymore.
There was a problem hiding this comment.
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
…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
force-pushed
the
19218/focus-monitor-multiple-calls
branch
from
d3eecb1 to
a4d4ea2
Compare
|
I've addressed the feedback @mmalerba. |
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.
With the current logic, if an element is being monitored with
checkChildrenset totrueand then we start monitoring it withfalse, the initial monitor call will stop checking the children, because we take thecheckChildrenvalue 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 tomonitorwithcheckChildrenturned on.Fixes #19218.