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.

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

DomHandler: getFocusableElements returns elements with hidden parents #14691

Closed
jasonverber opened this issue Feb 1, 2024 · 5 comments · Fixed by #14692
Closed

DomHandler: getFocusableElements returns elements with hidden parents #14691

jasonverber opened this issue Feb 1, 2024 · 5 comments · Fixed by #14692
Assignees
Labels
Type: Bug Issue contains a bug related to a specific component. Something about the component is not working
Milestone

Comments

@jasonverber
Copy link
Contributor

Describe the bug

DomHandler.getFocusableElements checks the display and visibility of each element itself but doesn't account for ancestors with display: none. This results in unexpected behavior when tabbing in, for example, a dialog box with focus trap enabled.

Environment

Angular 17.0.8

Reproducer

No response

Angular version

17.0.8

PrimeNG version

17.3.0

Build / Runtime

Angular CLI App

Language

TypeScript

Node version (for AoT issues node --version)

18.19.0

Browser(s)

No response

Steps to reproduce the behavior

  1. Hide the parent of an otherwise focusable element via display: none
  2. Use focus trap and tab around
  3. Observe how focus gets stuck or otherwise behaves unexpectedly.

Expected behavior

An element that is not displayed because its parent has display: none shouldn't be eligible for focus and thus should be excluded from the results.

@jasonverber jasonverber added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Feb 1, 2024
jasonverber pushed a commit to jasonverber/primeng that referenced this issue Feb 1, 2024
@jasonverber
Copy link
Contributor Author

I can add a Stackblitz if 100% necessary.

@cetincakiroglu
Copy link
Contributor

Hi,

Yes, could you please share a stackblitz also, so we can identify the issue clearly?

@cetincakiroglu cetincakiroglu added Status: Pending Review Issue or pull request is being reviewed by Core Team and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Feb 12, 2024
@cetincakiroglu cetincakiroglu modified the milestones: 17.7.0, 17.8.0 Feb 12, 2024
@jasonverber
Copy link
Contributor Author

Hopefully this Stackblitz reproduces the issue clearly.

cetincakiroglu added a commit that referenced this issue Feb 22, 2024
…ents-ancestors-display-none

Fix #14691 DomHandler.getFocusableElements returns visible elements
@cetincakiroglu cetincakiroglu added Type: Bug Issue contains a bug related to a specific component. Something about the component is not working and removed Status: Pending Review Issue or pull request is being reviewed by Core Team labels Feb 22, 2024
@cetincakiroglu
Copy link
Contributor

Hi,

Thanks for reporting the issue and the PR but it caused errors in some of the components. For example; tab functionality throws console errors in dropdown. Probably it's same in the multiselect, autocomplete etc. Reverting the PR and re-opening the issue. Moving the issue to the 17.Future.

@cetincakiroglu cetincakiroglu modified the milestones: 17.8.0, 17.Future Feb 22, 2024
@github-actions github-actions bot added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Feb 22, 2024
cetincakiroglu added a commit that referenced this issue Feb 22, 2024
…ocusableElements-ancestors-display-none

Revert "Fix #14691 DomHandler.getFocusableElements returns visible elements"
@cetincakiroglu cetincakiroglu added Status: Pending Review Issue or pull request is being reviewed by Core Team and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Feb 22, 2024
@cetincakiroglu cetincakiroglu self-assigned this Feb 22, 2024
@jasonverber
Copy link
Contributor Author

@cetincakiroglu sorry about that -- it looks like it was the dropdowns with virtual scroll that were causing the issue? My new PR fixes this issue without breaking those.

I was calling fosuableElement.getComputedStyle whereas previously the global getComputedStyle was being used twice. I adjusted my fix to use the global one and consolidated what had been two calls into one. I hope this fixes it without causing any additional issues -- I ran it locally and tried all the dropdowns as well as all (I think, there's so many!) of the autocomplete and multiselect examples.

@cetincakiroglu cetincakiroglu modified the milestones: 17.Future, 17.8.0 Feb 23, 2024
@github-actions github-actions bot added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Feb 23, 2024
cetincakiroglu added a commit that referenced this issue Feb 23, 2024
…cusableElements

Fix #14691 DomHandler.getFocusableElements should return only visible els
@cetincakiroglu cetincakiroglu removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible Status: Pending Review Issue or pull request is being reviewed by Core Team labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issue contains a bug related to a specific component. Something about the component is not working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants