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

[ACS-4257] - Resolved a11y issue around jumping focus #8168

Merged
merged 8 commits into from
Feb 14, 2023

Conversation

swapnil-verma-gl
Copy link
Contributor

Clicking on filter buttons on search results no longer switches focus from button to first element

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation
  • Other... Please describe:

What is the current behaviour? (You can also link to an open issue here)
Clicking on any of the filter button chips on the search results page would instantly switch focus from the button to the first element in the filter layover

What is the new behaviour?
Focus no longer switches to the first element. Instead, it remains on the button. Pressing tab moves and then traps the focus within the layover.

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@swapnil-verma-gl
Copy link
Contributor Author

swapnil-verma-gl commented Jan 19, 2023

I've also noticed a similar around the permissions-list.component, when clicking on the Show/Hide button for inherited permissions. Clicking on the show button switches focus from the button, to the table header for permission name. There, it is happening because we are providing the selector for the element to which the focus needs to be switched. See template and directive

The solution I am proposing for that is to remove the selector from the template itself. Note that this is not a reported issue yet, just something I noticed while working on the same area

@@ -46,7 +46,6 @@ export class SearchFacetChipComponent {
onMenuOpen() {
if (this.menuContainer && !this.focusTrap) {
this.focusTrap = this.focusTrapFactory.create(this.menuContainer.nativeElement);
this.focusTrap.focusInitialElement();
Copy link
Contributor

Choose a reason for hiding this comment

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

if you remove this line, the whole sense in having a focusTrab attribute gets lost... the only thing that is left is assigning this property, but not using it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The focusTrap does seem to work even after removing this line. I suspect that is because we are passing it the native element on which the focusTrap is to be initialised (see line num 48, right above this change). The user experience after this change came out to be -

  1. User performs a search (same as before, no change here)
  2. Once search results are received, he clicks on one of these (search filters?) chips (same as before, no change here)
  3. On clicking on the chip, the focus remains on the chip itself (changed from before, where the focus was getting jumped to the first element in the panel/popover)
  4. User presses Tab. The focus is switched to the first element in the popover (changed from before, this focus was received as soon as step#3 was performed)
  5. Now the user can keep on pressing 'Tab'. The focus remains within the fields in the popover (same as before, no change here)

Also, we do need to initialise the focusTrap and store it in a const reference. This is because we are using that reference when the popover is closed, to unregister the focusTrap (see line num 52 onwards)

@@ -47,7 +47,6 @@ export class SearchWidgetChipComponent {
onMenuOpen() {
if (this.menuContainer && !this.focusTrap) {
this.focusTrap = this.focusTrapFactory.create(this.menuContainer.nativeElement);
this.focusTrap.focusInitialElement();
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, you are essentially disabling this whole feature

@swapnil-verma-gl swapnil-verma-gl marked this pull request as ready for review January 25, 2023 08:11
@swapnil-verma-gl swapnil-verma-gl changed the title ACS-4257 - Resolved a11y issue around jumping focus [ACS-4257] - Resolved a11y issue around jumping focus Jan 25, 2023
@suneet-gupta suneet-gupta self-requested a review January 26, 2023 14:06
@suneet-gupta suneet-gupta merged commit 275d30b into develop Feb 14, 2023
@suneet-gupta suneet-gupta deleted the ACS-4257-swapnil-a11y-focus-jump branch February 14, 2023 16:31
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