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

Skip hidden dropdowns while focusing #29523

Merged
merged 9 commits into from
Oct 17, 2019
Merged

Skip hidden dropdowns while focusing #29523

merged 9 commits into from
Oct 17, 2019

Conversation

jeremyvii
Copy link
Contributor

This PR fixes issue #29405 by filtering out dropdown items set to display: none when focusing between items. The isVisible() utility method filters out the hidden items.

@jeremyvii jeremyvii requested a review from a team as a code owner October 14, 2019 17:23
@patrickhlauke
Copy link
Member

not had a chance to test, but i'll just throw out that you might also need to check for visible, but disabled, ones as well

@jeremyvii
Copy link
Contributor Author

I think disabled items are ignored per this dropdown selector here, but I will double check.

Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jeremyvii,

Thanks for your PR!
Can you add a unit test please ?

@jeremyvii
Copy link
Contributor Author

While adding this unit test, I had noticed that isVisible() doesn't account for the d-none class. I added a check for this class in isVisible().

@jeremyvii
Copy link
Contributor Author

I was just reviewing the change I made, and think it may be better to use getComputedStyle() for checking if an element is hidden. Rather than checking for the d-none class explicitly. That way if a user has some rouge class that hides an element it will still be skipped while focusing between items.

@XhmikosR XhmikosR merged commit c1ee395 into twbs:master Oct 17, 2019
lucanos pushed a commit to lucanos/bootstrap that referenced this pull request Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants