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

fix: restore old logic for getting focusable elements #3413

Merged
merged 3 commits into from
Feb 10, 2022

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Feb 9, 2022

Description

  1. Restored the previous (Vaadin 14-23) logic for collecting focusable elements that was changed in refactor: move focus utils to component-base #3141 and added tests for handling hidden elements
  2. Added tests for vaadin-accordion inside vaadin-dialog to ensure Tab works

Fixes #3410

Type of change

  • Bugfix

@web-padawan web-padawan added the a11y Accessibility issue label Feb 9, 2022
@web-padawan web-padawan marked this pull request as draft February 9, 2022 09:37
@web-padawan web-padawan removed the request for review from tomivirkki February 9, 2022 09:37
@web-padawan
Copy link
Member Author

This change causes tests to fail. I will investigate further how to handle this properly.

@web-padawan web-padawan changed the title fix: exclude hidden focusable elements fix: exclude focusable elements hidden indirectly Feb 9, 2022
@web-padawan web-padawan marked this pull request as ready for review February 9, 2022 10:09
@web-padawan web-padawan changed the title fix: exclude focusable elements hidden indirectly fix: restore old logic for getting focusable elements Feb 9, 2022
@sonarcloud
Copy link

sonarcloud bot commented Feb 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -137,6 +134,7 @@ export class FocusTrapController {
* @private
*/
get __focusedElementIndex() {
return this.__focusableElements.findIndex(isElementFocused);
const focusableElements = this.__focusableElements;
return focusableElements.indexOf(focusableElements.filter(isElementFocused).pop());
Copy link
Member Author

Choose a reason for hiding this comment

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

This check makes it work for vaadin-details and vaadin-accordion-panel: in those cases both host element and its focusElement satisfy the isElementFocused condition, so we should pick the right one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the old logic from 22.0 branch where it was previously placed:

_focusedIndex(elements) {
elements = elements || this._getFocusableElements();
return elements.indexOf(elements.filter(this._isFocused).pop());
}

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 23.0.0.beta2 and is also targeting the upcoming stable 23.0.0 version.

@knoobie
Copy link
Contributor

knoobie commented Oct 28, 2022

I have an edge case here.. not sure if it's worth to be reported

I have the following structure:

dialog > accordion > accordion panel > content

In the content I've a list of buttons that, once clicked, remove itself and add a h3 with tabindex=-1, text and a back button to show the list of buttons again. Of course, to manage focus, I'm focusing the h3 for a11y compliance. Now the focus sits on the h3 with tabindex=-1 - I expect that the next tab focuses the back button.. but instead it vanishes and I need like 3 tabs to reach the back button. This can be worked around by using tabindex=0 on the header. So I'm not sure if you even wanna do anything 😅

@web-padawan
Copy link
Member Author

Sounds like an edge case indeed. It's hard to say if we can make it work as expected without taking a closer look.
Feel free to create an issue with a code example, if possible, so that I could reproduce it in a web component.

@knoobie
Copy link
Contributor

knoobie commented Oct 28, 2022

Created #4863 for it - really minor, I have a workarond :) Not sure if it has other implications, that I can't forsee tho.

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.

[dialog] Accordion's content inside Dialog is not accessible by keyboard
4 participants