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

Jump to bottom

bug(FocusMonitor): Focus origin of already focused element doesn't change if FocusMonitor is on parent with checkChildren set to true. #21500

Closed
zelliott opened this issue Jan 5, 2021 · 3 comments · Fixed by #21512
Closed

bug(FocusMonitor): Focus origin of already focused element doesn't change if FocusMonitor is on parent with checkChildren set to true. #21500

zelliott opened this issue Jan 5, 2021 · 3 comments · Fixed by #21512
Assignees
@crisbeto
Labels
Accessibility This issue is related to accessibility (a11y) G This is is related to a Google internal issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@zelliott
Copy link
Collaborator

zelliott commented Jan 5, 2021 •
edited

Explanation

The fix #20966 made it so that the focus origin of an already focused element should change if focusVia is called on the element. Unfortunately, this doesn't appear to work if the FocusMonitor is on a parent element and set up with checkChildren = true. This is because the check here (https://github.com/angular/components/pull/20966/files#diff-02e96e7ea6a7d8f1caecdfdbb7cb38bdcdc5c43db34e4392723cc48765e330b7R316) fails, as nativeElement doesn't have a registered FocusMonitor. The FocusMonitor is instead registered on the parent.

Reproduction

Stackblitz: https://stackblitz.com/edit/angular-ivy-un8eee?file=src/app/hello.component.ts

Steps to reproduce:

  1. Open DevTools and watch the classes on document.body. There is a FocusMonitor attached to the document.body.
  2. Click on the button "Focus test button with origin". Note that the class .cdk-mouse-focused is first added, and then 5 seconds later, is replaced by the class .cdk-keyboard-focused. This is expected behavior. When this button is clicked, it calls focusVia wth keyboard origin on the "Test" button after 5 seconds.
  3. Now, again click on the button "Focus test button with origin". After clicking, and within 5 seconds, click on the button "Test". Now observe that the class .cdk-mouse-focused is present, and never changes to .cdk-keyboard-focused. This is not expected behavior. I'd expect the class to change to .cdk-keyboard-focused due to the fix fix(cdk/a11y): allow for origin of already focused element to be changed #20966. Unfortunately it doesn't, because the check this._elementInfo.has(nativeElement) fails within focus-monitor.ts.

Environment

  • Angular: Latest
  • CDK/Material: Latest
  • Browser(s): All
  • Operating System (e.g. Windows, macOS, Ubuntu): All
@zelliott zelliott added the needs triage This issue needs to be triaged by the team label Jan 5, 2021
@zelliott
Copy link
Collaborator Author

zelliott commented Jan 5, 2021

Pinging @crisbeto for his thoughts.

@crisbeto
Copy link
Member

crisbeto commented Jan 6, 2021

You're right @zelliott, I think I just didn't account for checkChildren in #20966 and it should be fixed on our end.

@crisbeto crisbeto added Accessibility This issue is related to accessibility (a11y) G This is is related to a Google internal issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels Jan 6, 2021
@crisbeto crisbeto self-assigned this Jan 7, 2021
@crisbeto crisbeto added the has pr label Jan 7, 2021
crisbeto added a commit to crisbeto/material2 that referenced this issue Jan 7, 2021
@crisbeto
fix(cdk/a11y): focusVia not accounting for focused child node
c9a63a6 
In angular#20966 some logic was added so that calling `focusVia` on an element that already
has focus would change the origin to the passed-in one. The problem is that the new
logic doesn't account for when a parent element is monitored and `focusVia` is called
on a child.

These changes add some more logic that will look through all the monitored elements
that have `checkChildren: true` and will switch the origin accordingly.

Fixes angular#21500.
@crisbeto crisbeto mentioned this issue Jan 7, 2021
Merged
crisbeto added a commit to crisbeto/material2 that referenced this issue Jan 7, 2021
@crisbeto
fix(cdk/a11y): focusVia not accounting for focused child node
2540419 
In angular#20966 some logic was added so that calling `focusVia` on an element that already
has focus would change the origin to the passed-in one. The problem is that the new
logic doesn't account for when a parent element is monitored and `focusVia` is called
on a child.

These changes add some more logic that will look through all the monitored elements
that have `checkChildren: true` and will switch the origin accordingly.

Fixes angular#21500.
annieyw pushed a commit that referenced this issue Jan 9, 2021
@crisbeto
fix(cdk/a11y): focusVia not accounting for focused child node (#21512)
f8df9f8 
In #20966 some logic was added so that calling `focusVia` on an element that already
has focus would change the origin to the passed-in one. The problem is that the new
logic doesn't account for when a parent element is monitored and `focusVia` is called
on a child.

These changes add some more logic that will look through all the monitored elements
that have `checkChildren: true` and will switch the origin accordingly.

Fixes #21500.
annieyw pushed a commit that referenced this issue Jan 9, 2021
@crisbeto @annieyw
fix(cdk/a11y): focusVia not accounting for focused child node (#21512)
612cdc1 
In #20966 some logic was added so that calling `focusVia` on an element that already
has focus would change the origin to the passed-in one. The problem is that the new
logic doesn't account for when a parent element is monitored and `focusVia` is called
on a child.

These changes add some more logic that will look through all the monitored elements
that have `checkChildren: true` and will switch the origin accordingly.

Fixes #21500.

(cherry picked from commit f8df9f8)
wagnermaciel pushed a commit to wagnermaciel/components that referenced this issue Jan 14, 2021
@crisbeto @wagnermaciel
fix(cdk/a11y): focusVia not accounting for focused child node (angula…
64e4ca2 
…r#21512)

In angular#20966 some logic was added so that calling `focusVia` on an element that already
has focus would change the origin to the passed-in one. The problem is that the new
logic doesn't account for when a parent element is monitored and `focusVia` is called
on a child.

These changes add some more logic that will look through all the monitored elements
that have `checkChildren: true` and will switch the origin accordingly.

Fixes angular#21500.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@crisbeto crisbeto

Labels
Accessibility This issue is related to accessibility (a11y) G This is is related to a Google internal issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(cdk/a11y): focusVia not accounting for focused child node crisbeto/material2
2 participants
@zelliott @crisbeto