Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-modal manager] Implemented focus lock to confine focus within the modal popup. #2014

Merged
merged 37 commits into from
Feb 15, 2024

Conversation

AH106586Harika
Copy link
Contributor

@AH106586Harika AH106586Harika commented Feb 1, 2024

Summary

Fixed- Focus is continuing to go outside of the Modal Manager when navigating through the component.

What was changed:
Added FocusLock to confine the focus within the modal popup

Why it was changed:
Pressing the TAB key causes the focus to shift away from the modal popup.

Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

This PR resolves:
UXPLATFORM-10090
UXPLATFORM-10169


Thank you for contributing to Terra.
@cerner/terra

@AH106586Harika AH106586Harika changed the title [terra-modal manager] Implemented focustrap to confine focus within the modal popup. [terra-modal manager] Implemented focus lock to confine focus within the modal popup. Feb 1, 2024
This reverts commit afc75c2.
@JessieRandle
Copy link
Contributor

We had added focus trap to this element and UX had us remove it. @scottwilmarth Can you chime in with why we don't want focus trap in modal manager?

@rbsree
Copy link

rbsree commented Feb 13, 2024

As suggested in the PR, the user focus when navigating using the keyboard is confined inside the modal dialog and the focus is to be navigated in cyclic order within the dialog. However, the issue is that tabindex="0" added to the modal dialog container, causing it to receive focus every time the user navigates within the modal dialog in cyclic order.

It is recommended to add tabindex="-1" to the modal dialog container

, to set the initial focus on the modal dialog container when it is opened.

@AH106586Harika
Copy link
Contributor Author

As suggested in the PR, the user focus when navigating using the keyboard is confined inside the modal dialog and the focus is to be navigated in cyclic order within the dialog. However, the issue is that tabindex="0" added to the modal dialog container, causing it to receive focus every time the user navigates within the modal dialog in cyclic order.

It is recommended to add tabindex="-1" to the modal dialog container

, to set the initial focus on the modal dialog container when it is opened.

Implemented. Thanks.

const modalContent = (
<div
{...customProps}
tabIndex={platformIsiOS || isCalledFromNotificationDialog ? '-1' : '0'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does container receive initial focus without tabIndex ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the primary focus is on the container.PFA

modal_popup.mov

{...customProps}
aria-label={ariaLabel}
aria-labelledby={ariaLabelledBy}
aria-describedby={ariaDescribedBy}
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to validate with screen readers to ensure values provided to above aria attributes are read when the focus is on modal container. Since there is no tabIndex on this div, it may get ignored by screen readers

Copy link
Contributor Author

@AH106586Harika AH106586Harika Feb 15, 2024

Choose a reason for hiding this comment

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

Hi @supreethmr , when the focus is on modal container all the aria attributes are working as expected. Please find the attached video for reference.

test.mov
Screenshot 2024-02-15 at 2 25 43 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Please verify on JAWS as well

Copy link
Contributor Author

@AH106586Harika AH106586Harika Feb 15, 2024

Choose a reason for hiding this comment

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

In JAWS as well It is announcing all the aria attributes PFA video.

Screen.Recording.2024-02-15.at.2.53.45.PM.mov

@rahulkumar8cs
Copy link

+1

@github-actions github-actions bot temporarily deployed to preview-pr-2014 February 15, 2024 11:45 Destroyed
@saket2403 saket2403 merged commit 2b3357e into main Feb 15, 2024
22 checks passed
@saket2403 saket2403 deleted the AH106586-modal-manager-focus-lock branch February 15, 2024 12:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants