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

Add focus restoration to Dialog, ModalDialog #930

Merged
merged 2 commits into from
Mar 22, 2023
Merged

Conversation

lyzadanger
Copy link
Contributor

This PR adds restore-focus support when a Dialog is closed (unmounted in this case, as Dialogs do not manage state otherwise). This is disabled by default for (i.e. non-modal) Dialogs, but enabled by default for ModalDialog.

This is consistent with WAI-ARIA behavior guidelines for modal dialogs.

Depends on #929
Part of #77

@lyzadanger lyzadanger force-pushed the dialog-restore-focus branch from 6175c03 to 3c20b42 Compare March 21, 2023 19:22
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #930 (00afebe) into main (b1c85f7) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #930   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        64    +1     
  Lines          795       807   +12     
  Branches       287       289    +2     
=========================================
+ Hits           795       807   +12     
Impacted Files Coverage Δ
src/hooks/use-element-should-close.js 100.00% <ø> (ø)
src/components/Checkbox.js 100.00% <100.00%> (ø)
src/components/feedback/Dialog.tsx 100.00% <100.00%> (ø)
src/components/feedback/ModalDialog.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

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

👍🏼

},
// We only want to run this effect once when the dialog is mounted.
// eslint-disable-next-line react-hooks/exhaustive-deps
[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing restoreFocusEl here should be the same in practice, as the ref object won't change on renders. Maybe we can explicitly add the dependency instead of suppressing the rule, to avoid unexpected future side effects if something else changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exhaustive dependencies would also require restoreFocus (a prop). I don't want this hook to run again if the restoreFocus prop changes...

Base automatically changed from split-modal-dialog to main March 22, 2023 14:58
@lyzadanger lyzadanger force-pushed the dialog-restore-focus branch from 3c20b42 to 00afebe Compare March 22, 2023 16:57
@lyzadanger lyzadanger merged commit a798382 into main Mar 22, 2023
@lyzadanger lyzadanger deleted the dialog-restore-focus branch March 22, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants