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

:focusable incorrectly requires tabindex on details and summary elements #1885

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bbenjamin
Copy link
Contributor

I noticed this when opening a dialog with <details> elements that

contained no interactive elements other than <summary>.
Was not followed by any other interactive elements.
I was unable to access <details> with tab navigation as the focus was trapped at what it believed to be the last focusable element.
This is addressed by adding details and summary to the list of elements $.ui.focusable assumes tabbable.

Copy link

@fuzzbomb fuzzbomb left a comment

Choose a reason for hiding this comment

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

<summary> elements are expected to be tabbable, but <details> elements are not.

Does this actually need to add |details|summary to the list of element types, rather than just |summary?

@dmethvin
Copy link
Member

@bbenjamin can you answer the question from @fuzzbomb ?

@bbenjamin
Copy link
Contributor Author

@dmethvin - @fuzzbomb was correct, only summary is required. The PR was updated shortly after that based on his feedback. Looks like I neglected to follow that up with a comment here.

In case it's of use to people with a similar problem - After realizing jQueryUI hadn't had a commit since 2017 I proposed an alternate solution for its use in Drupal https://www.drupal.org/project/drupal/issues/3038336 - the patch there provides the overrides necessary to correct this issue without having to alter jQueryUI.

@dmethvin
Copy link
Member

dmethvin commented Aug 1, 2019

Thank you for the quick reply! We're trying to get a little love started here for jQuery UI, mainly bug fixes and compat with the latest jQuery version. This is a good one to add.

@mgol
Copy link
Member

mgol commented Dec 8, 2019

This change requires a test as well.

Sorry for the late reply but I'm not really in the UI team, although I have commit access. If you're still interested in getting it landed, feel free to ping me when a test is added.

Base automatically changed from master to main February 19, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants