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

@bbenjamin bbenjamin commented Mar 15, 2019

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

<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

@dmethvin dmethvin commented Jul 31, 2019

@bbenjamin can you answer the question from @fuzzbomb ?

@bbenjamin
Copy link
Author

@bbenjamin bbenjamin commented Aug 1, 2019

@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 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 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 Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants