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

Improve the focus management of Modal component #2483

Open
1 task done
ntdiary opened this issue Feb 7, 2023 · 5 comments
Open
1 task done

Improve the focus management of Modal component #2483

ntdiary opened this issue Feb 7, 2023 · 5 comments
Labels
enhancement Requires extension or creation of new React Native API

Comments

@ntdiary
Copy link
Contributor

ntdiary commented Feb 7, 2023

Is there an existing request?

  • I have searched for this request

Describe the feature request

a concise description of the request

Supports setting the focus on the specified element when closing the Modal.

potential solutions

Add a focusAfterClosed?: ?() => HTMLElement prop

additional context

// To be fully compliant with WCAG we need to refocus element that triggered opening modal
// after closing it
React.useEffect(function () {
if (canUseDOM) {
const lastFocusedElementOutsideTrap = document.activeElement;
return function () {
if (
lastFocusedElementOutsideTrap &&
document.contains(lastFocusedElementOutsideTrap)
) {
UIManager.focus(lastFocusedElementOutsideTrap);
}
};
}
}, []);

The current code implements Example point 3.
However, in practice, we often also need the focus to be able to return to the specified element. (Return to the trigger element is still the default behavior.)
source: https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/examples/dialog/

example.mp4

In this case:
The expected order of focus is:
Add Delivery Address button --> Street input --> Add button --> OK button --> Add Delivery Address button
The actual order of focus is:
Add Delivery Address button --> Street input --> Add button --> OK button --> body
https://codesandbox.io/s/focus-order-djuzdk

This issue is from my comment. And the original issue #2175 has been explained in my comments. Maybe that issue could be closed as well?

this commit is trying to implement this feature. If this feature is acceptable, I would improve the docs, tests, and then submit a PR.

@necolas
Copy link
Owner

necolas commented Mar 27, 2023

Is this an issue because there are 2 modals being created?

  1. Focus moves from App to Modal 1 to Modal 2
  2. Once Modal 2 is closed the logic tries to find the "trigger" from Modal 1
  3. Modal 1 and the last trigger were unmounted, so focus goes back to the document

@necolas
Copy link
Owner

necolas commented Mar 27, 2023

Re: focusAfterClosed proposal. That would only work on RNW if we added it, because there's no equivalent prop in React Native.

@ntdiary
Copy link
Contributor Author

ntdiary commented Mar 30, 2023

Is this an issue because there are 2 modals being created?

  1. Focus moves from App to Modal 1 to Modal 2
  2. Once Modal 2 is closed the logic tries to find the "trigger" from Modal 1
  3. Modal 1 and the last trigger were unmounted, so focus goes back to the document

Yeah, that means we want the refocusing behavior to be controllable.

Re: focusAfterClosed proposal. That would only work on RNW if we added it, because there's no equivalent prop in React Native.

I'm also hesitant to suggest adding a prop that React native doesn't have. It seems that focus trap and refocusing behavior only exists on the web. They are very useful most of time, it's just that they really stuck us in a few scenarios, and these requirements also meet the accessibility norms.

I had another idea: export a few functions somewhere, for example, add disableTrap and setReturnFocus in the UIManager as an escape hatch. The former can help us to disable the focus trap before the user has clicked to close the current modal but it has not been unmounted. The latter can help us to specify which element we want to refocus when the current modal is closed.

The implementation may be like this: ntdiary@274d344
Maybe it's still not a good practice for the library, but it can help us out and give users a good experience.

I fully understand that you may have to consider many factors, so here is just a discussion, and sincerely looking forward to your thoughts. Thanks again for making such a great library. 😄

@necolas
Copy link
Owner

necolas commented Apr 13, 2023

Can we avoid making API changes at this stage by storing a stack of triggers, and refocusing the first that is still connected to the document?

@ntdiary
Copy link
Contributor Author

ntdiary commented Apr 25, 2023

Can we avoid making API changes at this stage by storing a stack of triggers, and refocusing the first that is still connected to the document?

Thank you for your reply, The above example is just one scenario. I'm afraid this approach can only solve limited scenarios. Perhaps we can just lower its priority first. If other people have similar needs in the future, we can then consider how to do it. : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Requires extension or creation of new React Native API
Projects
None yet
Development

No branches or pull requests

3 participants
@necolas @ntdiary and others