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

Support slots/shadow DOM for custom tab focusing #1

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

Conversation

KeithKosh
Copy link
Collaborator

No description provided.

@KeithKosh
Copy link
Collaborator Author

@elidupuis This is my first attempt at this.

Basically, the issue is that in the original, trapTabKey() builds a list of focusable children, but because the shadow DOM is not traversable via querySelectorAll, any buttons/links in the shadow DOM are not included in that list and tabbing will not focus those elements.

With this change, if there is a shadow DOM (labeled with data-a11y-dialog-shadow-dom) included in the popup, its selectable elements are appended onto the regular list and used for tabbing and re-focusing when the first/last element is selected.

Still some things to consider:

  • Better way to automatically add shadow DOM elements? It doesn't seem like there's a way to automatically select them via JS without having a specific reference to the shadow DOM itself.
  • Any way to include shadow DOM elements in their normal order compared to other selectable elements, instead of always appending to the end?

@KeithKosh
Copy link
Collaborator Author

Looks like in our implementation, there's also issues with _bindKeypress() exiting early (though it's not happening in this test file). Investigating...

@KeithKosh
Copy link
Collaborator Author

KeithKosh commented Mar 24, 2022

@elidupuis Updated with very custom recursive code.

Basically, we now replace querySelectorAll (when needed) with a function that iterates through child nodes, including shadow DOM, and then compiles selectable elements from each of those child nodes (by running querySelectorAll on child nodes). This way we aren't assuming that the shadow DOM is in a particular place - no matter where one is (or isn't) it will be included as a place to look for selectable elements.

It's also different enough from a library like kagekiri that I haven't included a reference to that library in the comments. Even pulling out the specific function from that library was quickly ballooning into requiring many other functions and even a dependency - so this solution is quite different/simpler.

@KeithKosh KeithKosh marked this pull request as ready for review March 30, 2022 21:50
@KeithKosh
Copy link
Collaborator Author

@elidupuis Ready for review! Just adding some tests in cypress/integrations.

Also... maybe this branch doesn't need the package-lock change? Not sure why it's in there... 🤔

@elidupuis
Copy link
Member

package-lock is likely changed because we have a newer version of npm than the last time the upstream repository was updated.

@KeithKosh
Copy link
Collaborator Author

No longer used as we are moving to use <sl-dialog> for our popups. See https://github.com/showbie/web/pull/4679.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants