pradel / react-responsive-modal

Simple responsive react modal
https://react-responsive-modal.leopradel.com/
MIT License
604 stars 95 forks source link

feat: add options to set initial focus within modal #476

Closed megawebmaster closed 3 years ago

megawebmaster commented 3 years ago

Hi!

First of all: I really like your library - it's lean and works great. As I use it extensively I needed to work around issue with trapping focus within the modal, but without focusing first focusable element (like a button or link), but instead focus the modal container. This allows me to style :focus properly and have accessibility (so that keyboard users can browse the page properly).

To do this I decided to add options for FocusTrap that allows me to set where I want to have focus when trapped. I designed it in a way that allows people to update (it shouldn't change default behavior).

Let me know your thoughts! :muscle:

codecov[bot] commented 3 years ago

Codecov Report

Merging #476 (02cbb04) into master (23cccc6) will decrease coverage by 1.96%. The diff coverage is 95.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
- Coverage   97.20%   95.23%   -1.97%     
==========================================
  Files           6        6              
  Lines         179      189      +10     
  Branches       66       70       +4     
==========================================
+ Hits          174      180       +6     
- Misses          5        9       +4     
Impacted Files Coverage Δ
react-responsive-modal/src/FocusTrap.tsx 70.00% <50.00%> (-9.17%) :arrow_down:
react-responsive-modal/src/index.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 23cccc6...02cbb04. Read the comment docs.

megawebmaster commented 3 years ago

The code fails to build due to a weird error: offsetParent is null on all DOM nodes causing FocusTrap to think that nothing is visible and skipping the focus on a first visible element part. I filed a bug in @testing-library/react to get this fixed: https://github.com/testing-library/react-testing-library/issues/915

EDIT: The jsdom does not support such case, so I decided to add a Cypress test and a proper docs page for the case of focusing modal. Works as expected :muscle:

pradel commented 3 years ago

In any case thanks for the amazing pr, looking forward to add this feature :)

pradel commented 3 years ago

@megawebmaster do you think of a way to update the unit tests so the coverage is not decreasing?

megawebmaster commented 3 years ago

@pradel - unfortunately this is impossible due to missing capabilities in JSDOM used as the environment by React Testing Library. I even tried to find solutions upstream (because I thought it's important to have a good coverage).

EDIT: Do you have any idea why size checking fails even though it passes locally with npx size-limit?

pradel commented 3 years ago

Error creating comment. This can happen for PR's originating from a fork without write permissions.

This is the error message I can see for the size limit so all good :D

pradel commented 3 years ago

@megawebmaster thank you so much for this amazing pr :D I will create a new release shortly

megawebmaster commented 3 years ago

Great, thanks for the review and many good ideas how to make it even better :muscle: