Fix glitches when closing various modal dialogs

[techanvil]

Feature Description

There are a number of glitches evident that occur when closing various Site Kit modal dialogs.

Note that a dialog can be closed via the Cancel button, or clicking outside the dialog/pressing the Escape key.

When a dialog is closed, it should restore focus to the previously focused element. However, some dialogs are not restoring the focus when closing it via one or more of the above closing mechanisms. Additionally, the button that opens the dialog is being broken in some cases.

To illustrate, here's a mostly well-behaved dialog. As can be seen the Dashboard Sharing dialog restores focus to the button that opens it when closing via the Cancel button, when clicking outside the dialog, and when pressing Escape. Clicking the button to open it again works as expected. The only minor glitch evident here is the tooltip on the button is a bit inconsistent and only pops up on refocusing in the case where the dialog is closed via Escape.

https://github.com/user-attachments/assets/4f79a61b-da85-4823-86bb-126da1933849

Here's a less well-behaved dialog. The Consent Mode dialog does restore focus to the Enable consent mode switch when closing by clicking outside the dialog or pressing Escape. However, it doesn't restore focus when closing by pressing Cancel. Furthermore, it actually breaks the switch when closing by clicking outside the dialog or pressing Escape.

https://github.com/user-attachments/assets/4b045002-f94d-423f-901d-4a0258eabda3

It turns out that all of our dialogs have one or more glitches that need addressing.

• Additional Permissions Required
  • This is the generic dialog that pops up when the user doesn't have permission to do something. As can be seen on the Tag Manager settings screen when trying to create a container, this dialog:
    • Does not restore focus when pressing Cancel or when clicking outside/pressing Escape.
    • Breaks the Complete setup button when clicking outside/pressing Escape.
    • See assets/js/components/Permissionsdialog/AuthenticatedPermissionsdialog.js.
• Disconnect
  • The dialog triggered by the Disconnect button present in the User Menu on the dashboard.
    • Breaks the Disconnect button when clicking outside.
    • See assets/js/components/UserMenu/index.js.
• Disconnect {module} from Site Kit?
  • The dialog triggered by the Disconnect {module} from Site Kit button on the Settings edit view for a module.
    • Does not restore focus when pressing Cancel or pressing Escape.
    • Breaks the Disconnect {module} from Site Kit button when clicking outside (the button needs to be pressed twice to work).
    • See assets/js/components/settings/SettingsActiveModule/ConfirmDisconnect.js
• Reset Site Kit
  • The dialog triggered by the Reset Site Kit button present in Settings > Admin Settings > Plugin Status, and on the splash screen that appears when a user disconnects Site Kit.
    • Breaks the Reset Site Kit button when clicking outside.
    • See assets/js/components/ResetButton.js.
• Disable consent mode?
  • The dialog that appears when toggling Consent Mode off in Settings > Admin Settings > Consent Mode.
    • Does not restore focus when pressing Cancel or when clicking outside/pressing Escape.
    • Breaks the Enable consent mode switch when clicking outside/pressing Escape.
    • See assets/js/components/consent-mode/ConfirmDisableConsentModeDialog.js.
• Disable enhanced conversion tracking
  • The dialog that appears when toggling enhanced conversion tracking off in the Settings edit view for the Ads module,
    • Does not restore focus when pressing Cancel or when clicking outside/pressing Escape.
    • See assets/js/components/conversion-tracking/ConfirmDisableConversionTrackingDialog.js.
• The Audiences Error Modal - Failed to set up visitor groups etc.
  • This is the dialog that appears when an error occurs while setting up the Audience Segmentation feature.
    • Doesn't restore focus when pressing Cancel.
    • See assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceErrorModal.js
• Dashboard sharing & permissions
  • The dialog triggered by the dashboard sharing button on the dashboard.
    • A minor glitch, the tooltip on the dashboard sharing button is a bit inconsistent and only pops up on refocusing in the case where the dialog is closed via Escape.
    • See assets/js/components/dashboard-sharing/DashboardSharingDialog/index.js.
• Reset Dashboard Sharing permissions
  • The dialog triggered by the Reset sharing permissions button on the Dashboard sharing & permissions dialog.
    • Does not restore focus when pressing Cancel or when clicking outside/pressing Escape.
    • Additionally, the transition is different when pressing Cancel vs clicking outside/pressing Escape. When pressing Cancel the dialog immediately changes back to the Dashboard sharing & permissions version. However, when clicking outside/pressing Escape, the dialog disappears and the Dashboard sharing & permissions is re-shown with its fade-in transition.
    • See assets/js/components/dashboard-sharing/DashboardSharingDialog/index.js.

Note that the Dashboard Sharing dialogs will be addressed via a separate issue, https://github.com/google/site-kit-wp/issues/9138, as their defects and implementation are a bit different to the other dialogs.

---

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

Acceptance criteria

• When a dialog is closed, it should restore focus to the previously focused element, which should continue to function normally. This applies to the following dialogs, which should be fixed as specified:
• Additional Permissions Required
  • This is the generic dialog that pops up when the user doesn't have permission to do something.
    • It should restore focus to the previous element when pressing Cancel or when clicking outside/pressing Escape to close the dialog.
    • It should not break the behaviour that triggers it.
    • A concrete example is the Tag Manager settings screen when trying to create a container, the dialog should restore focus to the Complete setup button on close, and the button should continue to work as usual.
• Disconnect
  • The dialog triggered by the Disconnect button present in the User Menu on the dashboard.
    • Should not breaks the Disconnect button when clicking outside.
• Disconnect {module} from Site Kit?
  • The dialog triggered by the Disconnect {module} from Site Kit button on the Settings edit view for a module.
    • Should restore focus to the Disconnect {module} from Site Kit button when pressing Cancel or pressing Escape.
    • Should not break the Disconnect {module} from Site Kit button when clicking outside.
• Reset Site Kit
  • The dialog triggered by the Reset Site Kit button present in Settings > Admin Settings > Plugin Status, and on the splash screen that appears when a user disconnects Site Kit.
    • Should not break the Reset Site Kit button when clicking outside.
• Disable consent mode?
  • The dialog that appears when toggling Consent Mode off in Settings > Admin Settings > Consent Mode.
    • Should restore focus when pressing Cancel or when clicking outside/pressing Escape.
    • Should not break the Enable consent mode switch when clicking outside/pressing Escape.
• Disable enhanced conversion tracking
  • The dialog that appears when toggling enhanced conversion tracking off in the Settings edit view for the Ads module,
    • Should restore focus when pressing Cancel or when clicking outside/pressing Escape.
• The Audiences Error Modal - Failed to set up visitor groups etc.
  • This is the dialog that appears when an error occurs while setting up the Audience Segmentation feature.
    • Should restore focus when pressing Cancel.

Implementation Brief

FIx for button getting broken after clicking outside the modal to close it

• [ ] Update ModalDialog component in:
  • assets/js/components/PermissionsModal/AuthenticatedPermissionsModal.js
    • Include onClose prop and pass the onCancel callback.
  • assets/js/components/UserMenu/index.js
    • Include onClose prop and pass the handleDialog callback
  • assets/js/components/settings/SettingsActiveModule/ConfirmDisconnect.js
    • Include onClose prop and pass the handleDialog callback
  • assets/js/components/ResetButton.js
    • Include onClose prop and pass the toggleDialogActive callback
  • assets/js/components/consent-mode/ConfirmDisableConsentModeDialog.js
    • Include onClose prop and pass the onCancel callback

Test Coverage

QA Brief

Changelog entry

[zutigrm]

I detected the reason for button being broken after clicking outside the Modal. But focus not getting back to the button that triggered it is pretty weird occurrence. What I tracked down is that once modal is closed, the focus is switched to the body, and only once you click anywhere on the page, focus returns to the original element that triggered the modal. Couldn't identify the reason focus in some modals goes back to the body first before returning to the button. Un-assigning myself in order not to spend too much time going down the rabbit hole, I included the first part of the fix in the IB, for the focus issue - leaving it to someone else who might be more familiar with this ModalDialog component or have run into the reason for this focus shift before.

[techanvil]

Hi @juniovitorino, thanks for drafting this IB. It's off to a good start, but a few points need addressing:

• The fact that we don't know how to resolve the focus loss issue indicates this is not ready to move to execution yet. It's OK to defer things that we know we'll be able to figure out to execution time, but this one seems like a bit of an unquantifiable one at the moment that we won't be able to give a meaningful estimate for. We should get a clearer picture of how we're going to approach this and document it in the IB. I've left a related comment in the ongoing Slack thread.
• The onClose() handler and any associated logic also needs to be expanded on. For one thing, I am assuming you mean using the onClose() handler from the ModalDialog component in all cases, but this isn't clear in the IB, which should be more specific. Furthermore the actual logic should be clarified; taking the AuthenticatedPermissionsModal component, for example, I've found that provining the onClose={ onCancel } prop to ModalDialog restores the button's functionality. Could a similar approach be taken for the other issues where clicking outside/Escape breaks the trigger button?
• The "Issues" sections you've included aren't needed for the most part, as they are just duplicating the AC. Please only include any details here that aren't covered by the AC, and let that remain the source of truth for the requirements.
• There's no need to include the "Excluded Work" section, as the Dashboard Sharing & Permissions dialogs are not actually included in the AC.
• Please remember to add an estimate when this is ready :)

Please also note, there was a typo where PermissionsModal/AuthenticatedPermissionsModal should have referred to PermissionsModal/AuthenticatedPermissionsModal. This was present in the AC as well; I've fixed it in both places.

[techanvil]

Noting here that this issue has been split in order to address the focus issues separately via https://github.com/google/site-kit-wp/issues/10548.

[techanvil]

Thanks @juniovitorino - did you see my comment on Slack, though? I think you are pretty close to wrapping this one up.

Maybe we can pair on this one as well to get it over the line, I'd be happy to do so. We can discuss it on our sync later.

[benbowler]

Hey @techanvil, I accidentally wrote the IB here as I clicked on it while I was working on #10548 and didn't notice as they look so similar. Anyway, feel free to review and send back to me or send to @juniovitorino.

[techanvil]

Thanks for flagging that, @benbowler. As this is @juniovitorino's first IB, I feel it would be valuable for him to complete it. However your addition is useful, so let's use it as feedback for Junio's initial draft.

I've reverted your change to the IB in the issue description, but kept it here to refer to. I'll continue my review in the next comment.

---

Implementation Brief

A common pattern failure exists across multiple components: state is properly reset when using Cancel buttons but not when dialogs close through other means, resulting in broken UI triggers.

To fix Implement a consistent pattern across all affected components:

1. Identify state being reset in onCancel handlers;
2. Add equivalent onClose handlers for outside clicks/Escape key;
3. Ensure proper handler propagation to underlying ModalDialog components;

onCancel issue affected components

Component	File Path	Issue
Additional Permissions Required	assets/js/components/PermissionsModal/AuthenticatedPermissionsModal.js	"Complete setup" button breaks
Disconnect	assets/js/components/UserMenu/index.js	Disconnect button breaks
Module Disconnect	assets/js/components/settings/SettingsActiveModule/ConfirmDisconnect.js	Requires double clicks
Reset Site Kit	assets/js/components/ResetButton.js	Reset button breaks
Disable Consent Mode	assets/js/components/consent-mode/ConfirmDisableConsentModeDialog.js	Switch breaks

Honour onCancel event during escape or scrim event

• [ ] Update assets/js/components/PermissionsModal/AuthenticatedPermissionsModal.js calling onCancel on the onClose prop of ModalDialog.
• [ ] Update assets/js/components/UserMenu/index.js calling handleDialog on the onClose prop of ModalDialog.
• [ ] Update assets/js/components/settings/SettingsActiveModule/ConfirmDisconnect.js calling handleDialog on the onClose prop of ModalDialog.
• [ ] Update assets/js/components/ResetButton.js calling toggleDialogActive on the onClose prop of ModalDialog.
• [ ] Update assets/js/components/consent-mode/ConfirmDisableConsentModeDialog.js calling onCancel on the onClose prop of ModalDialog.

Test Coverage

• No additional test coverage required.

[techanvil]

@juniovitorino, thanks for updating the IB. A few points:

• Having split the issue in two, there are now only five dialogs in the AC; please remove the extra two from the IB which don't need to be addressed here.
• Although your outline of how to fix the components could arguably be enough for the IB, we would commonly go a step further as Ben has in his amendment above, digging into each component to identify the specific fix. Please review Ben's addition and apply where appropriate (i.e. for the five dialogs in the AC).
• We should always fill in the Test Coverage section, even if it's just to make it clear that no new tests are needed. In this particular case, what I would be doing to verify whether we should add more tests is to have a quick scan of the JS test coverage for each component to see if the behaviour we are fixing is tested for the Cancel button case. If so then extending the coverage would be sensible, but as it's not, we can be pragmatic and not provide additional coverage. We don't have a set coverage target and don't necessarily need to provide tests for all edge cases, and should weigh up the value of the tests we are writing vs the cost of adding and maintaining them.

[juniovitorino]

@juniovitorino, thanks for updating the IB. A few points:

• Having split the issue in two, there are now only five dialogs in the AC; please remove the extra two from the IB which don't need to be addressed here.
• Although your outline of how to fix the components could arguably be enough for the IB, we would commonly go a step further as Ben has in his amendment above, digging into each component to identify the specific fix. Please review Ben's addition and apply where appropriate (i.e. for the five dialogs in the AC).
• We should always fill in the Test Coverage section, even if it's just to make it clear that no new tests are needed. In this particular case, what I would be doing to verify whether we should add more tests is to have a quick scan of the JS test coverage for each component to see if the behaviour we are fixing is tested for the Cancel button case. If so then extending the coverage would be sensible, but as it's not, we can be pragmatic and not provide additional coverage. We don't have a set coverage target and don't necessarily need to provide tests for all edge cases, and should weigh up the value of the tests we are writing vs the cost of adding and maintaining them.

Hey @techanvil, I reviewed the IB and the additions from @benbowler, and everything looks correct. I added the test coverage and checked all the component test files—there’s no actual test coverage for the cancel button, except in one component, and that’s only for the button click. I agree that no additional tests are required.

[techanvil]

Thanks @juniovitorino, good to know.

Please can you also address this point from my previous review?

Having split the issue in two, there are now only five dialogs in the AC; please remove the extra two from the IB which don't need to be addressed here.

[techanvil]

Thanks for the update @juniovitorino, the IB LGTM.

IB ✅

[mohitwp]

QA Update ✅

Tested on dev environment. Verified all the modal dialog mentioned under QAB as per AC. Verified glitches are fixed for all mentioned dialog modals. Verified when a dialog is closed, its trigger and continue to function normally. Verified all the points mentioned under AC for the modal dialogs.

Additional Permissions Required

https://github.com/user-attachments/assets/fccf7fe6-5dc1-4ad1-a515-d762798b29c4

Disconnect {module} from Site Kit

https://github.com/user-attachments/assets/2a190d42-fe6e-466e-81ff-c8d632c1e7bc

Disconnect

https://github.com/user-attachments/assets/a1a6591b-40d2-439a-be71-1d1cf759624c

Disable consent mode?

https://github.com/user-attachments/assets/c03186fb-02f7-4b6f-9cc7-77df5e3decbe

Reset Site Kit

https://github.com/user-attachments/assets/a8792be9-77d0-49bb-8ee3-caf8204be70d