Issue / 10548 Restore Focus to Trigger When Closing Various Modal Dialogs

[10upsimon]

Summary

Addresses issue:

• Restore focus to trigger when closing various modal dialogs #10548

Relevant technical choices

PR Author Checklist

• My code is tested and passes existing unit tests.
• My code has an appropriate set of unit tests which all pass.
• My code is backward-compatible with WordPress 5.2 and PHP 7.4.
• My code follows the WordPress coding standards.
• My code has proper inline documentation.
• I have added a QA Brief on the issue linked above.
• I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

• Run the code.
• Ensure the acceptance criteria are satisfied.
• Reassess the implementation with the IB.
• Ensure no unrelated changes are included.
• Ensure CI checks pass.
• Check Storybook where applicable.
• Ensure there is a QA Brief.
• Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

• Ensure the PR has the correct target branch.
• Double-check that the PR is okay to be merged.
• Ensure the corresponding issue has a ZenHub release assigned.
• Add a changelog message to the issue.

[github-actions]

Size Change: +9.71 kB (+0.46%)

Total Size: 2.14 MB

Filename	Size	Change
./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js	59.1 kB	+1.16 kB (+2%)
./dist/assets/js/googlesitekit-main-dashboard-********************.js	151 kB	+1.17 kB (+0.78%)
./dist/assets/js/googlesitekit-metric-selection-********************.js	57.1 kB	+1.1 kB (+1.96%)
./dist/assets/js/googlesitekit-settings-********************.js	137 kB	+1.06 kB (+0.78%)
./dist/assets/js/googlesitekit-user-input-********************.js	49.4 kB	+1 kB (+2.07%)

ℹ️ View Unchanged

Filename	Size	Change
./dist/assets/css/googlesitekit-admin-css-********************.min.css	62.8 kB	0 B
./dist/assets/css/googlesitekit-adminbar-css-********************.min.css	11.8 kB	0 B
./dist/assets/css/googlesitekit-authorize-application-css-********************.min.css	846 B	0 B
./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css	8.44 kB	0 B
./dist/assets/js/32-********************.js	2.76 kB	0 B
./dist/assets/js/33-********************.js	2.25 kB	0 B
./dist/assets/js/34-********************.js	3.64 kB	0 B
./dist/assets/js/35-********************.js	936 B	0 B
./dist/assets/js/36-********************.js	893 B	0 B
./dist/assets/js/37-********************.js	1.83 kB	0 B
./dist/assets/js/38-********************.js	3.12 kB	0 B
./dist/assets/js/analytics-advanced-tracking-********************.js	903 B	0 B
./dist/assets/js/blocks/reader-revenue-manager/block-editor-plugin/editor-styles.css	124 B	0 B
./dist/assets/js/blocks/reader-revenue-manager/block-editor-plugin/editor-styles.js	492 B	0 B
./dist/assets/js/blocks/reader-revenue-manager/block-editor-plugin/index.js	8.42 kB	0 B
./dist/assets/js/blocks/reader-revenue-manager/common/editor-styles.css	307 B	0 B
./dist/assets/js/blocks/reader-revenue-manager/common/editor-styles.js	492 B	0 B
./dist/assets/js/blocks/reader-revenue-manager/contribute-with-google/index.js	9.47 kB	0 B
./dist/assets/js/blocks/reader-revenue-manager/contribute-with-google/non-site-kit-user.js	8.95 kB	0 B
./dist/assets/js/blocks/reader-revenue-manager/subscribe-with-google/index.js	9.47 kB	0 B
./dist/assets/js/blocks/reader-revenue-manager/subscribe-with-google/non-site-kit-user.js	8.96 kB	0 B
./dist/assets/js/blocks/sign-in-with-google/editor-styles.css	84 B	0 B
./dist/assets/js/blocks/sign-in-with-google/editor-styles.js	492 B	0 B
./dist/assets/js/blocks/sign-in-with-google/index.js	18.1 kB	0 B
./dist/assets/js/googlesitekit-activation-********************.js	24.6 kB	+327 B (+1.34%)
./dist/assets/js/googlesitekit-adminbar-********************.js	37.4 kB	+321 B (+0.87%)
./dist/assets/js/googlesitekit-api-********************.js	10.4 kB	0 B
./dist/assets/js/googlesitekit-components-********************.js	6.46 kB	-14 B (-0.22%)
./dist/assets/js/googlesitekit-consent-mode-********************.js	25.6 kB	0 B
./dist/assets/js/googlesitekit-data-********************.js	2.37 kB	-4 B (-0.17%)
./dist/assets/js/googlesitekit-datastore-forms-********************.js	9.13 kB	+3 B (+0.03%)
./dist/assets/js/googlesitekit-datastore-location-********************.js	2.08 kB	-1 B (-0.05%)
./dist/assets/js/googlesitekit-datastore-site-********************.js	20.9 kB	+6 B (+0.03%)
./dist/assets/js/googlesitekit-datastore-ui-********************.js	10.2 kB	0 B
./dist/assets/js/googlesitekit-datastore-user-********************.js	27.2 kB	+1 B (0%)
./dist/assets/js/googlesitekit-entity-dashboard-********************.js	69.6 kB	+917 B (+1.34%)
./dist/assets/js/googlesitekit-events-provider-contact-form-7-********************.js	646 B	0 B
./dist/assets/js/googlesitekit-events-provider-easy-digital-downloads-********************.js	977 B	0 B
./dist/assets/js/googlesitekit-events-provider-mailchimp-********************.js	630 B	0 B
./dist/assets/js/googlesitekit-events-provider-ninja-forms-********************.js	717 B	0 B
./dist/assets/js/googlesitekit-events-provider-optin-monster-********************.js	675 B	0 B
./dist/assets/js/googlesitekit-events-provider-popup-maker-********************.js	634 B	0 B
./dist/assets/js/googlesitekit-events-provider-woocommerce-********************.js	1.36 kB	0 B
./dist/assets/js/googlesitekit-events-provider-wpforms-********************.js	633 B	0 B
./dist/assets/js/googlesitekit-i18n-********************.js	4.16 kB	0 B
./dist/assets/js/googlesitekit-modules-ads-********************.js	53.2 kB	+332 B (+0.63%)
./dist/assets/js/googlesitekit-modules-adsense-********************.js	134 kB	+104 B (+0.08%)
./dist/assets/js/googlesitekit-modules-analytics-4-********************.js	209 kB	+321 B (+0.15%)
./dist/assets/js/googlesitekit-modules-********************.js	24.1 kB	+1 B (0%)
./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js	26.6 kB	+24 B (+0.09%)
./dist/assets/js/googlesitekit-modules-reader-revenue-manager-********************.js	48.9 kB	+10 B (+0.02%)
./dist/assets/js/googlesitekit-modules-search-console-********************.js	75.1 kB	+17 B (+0.02%)
./dist/assets/js/googlesitekit-modules-sign-in-with-google-********************.js	36.3 kB	-22 B (-0.06%)
./dist/assets/js/googlesitekit-modules-tagmanager-********************.js	33.1 kB	+11 B (+0.03%)
./dist/assets/js/googlesitekit-notifications-********************.js	55.7 kB	+211 B (+0.38%)
./dist/assets/js/googlesitekit-polyfills-********************.js	377 B	0 B
./dist/assets/js/googlesitekit-splash-********************.js	76.1 kB	+904 B (+1.2%)
./dist/assets/js/googlesitekit-vendor-********************.js	313 kB	+1 B (0%)
./dist/assets/js/googlesitekit-widgets-********************.js	113 kB	+446 B (+0.4%)
./dist/assets/js/googlesitekit-wp-dashboard-********************.js	65.4 kB	+318 B (+0.49%)
./dist/assets/js/runtime-********************.js	1.32 kB	-2 B (-0.15%)

_{compressed-size-action}

[github-actions]

Build files for 91d2d12 are ready:

• Download development build
• Download production build

[zutigrm]

Thanks @10upsimon , just a few comments from me

[10upsimon]

Thanks @zutigrm . Most of these sound good but I'd like @techanvil to read over the proposed changes as well, to be sure he is in agreement.

I also believe the active check is in place so that pressing escape while the modal is not visible doesn't invoke a refocus, perhaps @techanvil can confirm that.

[techanvil]

Thanks @zutigrm . Most of these sound good but I'd like @techanvil to read over the proposed changes as well, to be sure he is in agreement.

I also believe the active check is in place so that pressing escape while the modal is not visible doesn't invoke a refocus, perhaps @techanvil can confirm that.

Thanks @10upsimon. As discussed on Slack, I've noticed a couple of issues beyond what's been discussed here so far.

• The IB, and the implementation so far doesn't fully cover the AC, as the problems with the Cancel buttons are not addressed.
• The current approach taken for identifying the element to refocus on doesn't work in all existing known scenarios and is not easy to scale. I've discussed this in more detail in this comment, Issue / 10548 Restore Focus to Trigger When Closing Various Modal Dialogs #10863 (comment), where I've also suggested an alternative approach for consideration.
• Regarding the active check - I'm actually not sure about that. @10upsimon's suggestion that it's there so that pressing escape while the modal is not visible doesn't invoke a refocus sounds plausible, however I did try removing the active check and didn't notice any change to the behaviour (i.e. the target didn't refocus when pressing escape while the modal wasn't visible). I'd suggest checking in with @benbowler on this one as he drafted the IB and associated exploratory branch.

[zutigrm]

Thanks @techanvil I update the PR to cover the refocus on the cancel button click, and implemented your proposed approach in the referenced PR.

[techanvil]

Thanks @techanvil I update the PR to cover the refocus on the cancel button click, and implemented your proposed approach in the referenced PR.

Sounds good, thanks @zutigrm!

[eugene-manuilov]

Thanks, @10upsimon. I know this is how it was specced out in IB but let's refactor this stuff a bit to make it more reliable and less messy.

Let's create a new RefocusableModalDialog component that implement what you have added to the ModalDialog in this PR to separate this logic from the actual dialog implementation. It will have the same props as ModalDialog has + two new props that you added to this PR.

Inside the RefocusableModalDialog component you will need to add a useEffect hook with dialogActive as the only dependency to track a currently focused element when that prop becomes truthy and restore the focus when it is falsy.

[eugene-manuilov]

Oh, I see @zutigrm took it over. Aleksej, please, see my comment above.

[zutigrm]

Thanks @eugene-manuilov I refactored the current implementation, and removed the unecesarry hook. I tried also to simplify the the last button click, by using activeElement check on the document from useEffect which worked, but problem arrises when there are 2 triggers on the same page, like in case of module settings, the second click on different button will restore focus to previously focused button, instead of the last clicked one. So we will have to use the current approach of storing and listening to the last click on the module level.

Other than that rest of the logic has been simplified, and refocus has been implemented on modal closing and unmount - since we conditionally render the modal in some components, so dialogActive when falsy will never reach the modal component as it will be removed from the DOM.