DataViews: action modals without focusable elements within the modal content do not close when pressing the `Esc` key

[oandregal]

Description

DataViews supports adding actions that have modals. When a modal is open, it can be closed by pressing the Esc key, but only if the modal has a button that calls the closeModal on click.

Step-by-step reproduction instructions

Modify the DataViews story for render modals:

• Go to the DataViews story, where the render modal action is defined (link to code).
• Change hideModalHeader to modalHeader: 'Delete action'.
• Remove the buttons from the RenderModal component.

Run the storybook:

• npm install && npm run storybook:dev
• It'll automatically open a browser. Go to the DataViews default story at http://localhost:50240/?path=/docs/dataviews-dataviews--docs
• Open the "Delete item" action and verify that it has a header with a close button.
• Press the Esc key in the keyboard. The expected result was that it closed the modal, but it didn't.
• Verify that pressing the X button in the modal header does close the modal.

[oandregal]

cc @WordPress/gutenberg-components for awareness.

[himanshupathak95]

Hey @oandregal, Thanks for raising the issue.

Upon reproduction and investigation, I found out that after removing the button, the modal is not focused. That is the reason pressing Esc key doesn't close the modal.

I observed that if we manually focus the modal after it opens, pressing Esc successfully closes the modal -

Screen.Recording.2025-02-21.at.13.43.39.mov

So, when the modal is mounted to the DOM, we can shift the focus to it, thereby activating all the keypress functionalities.

[himanshupathak95]

Currently, focusOnMount="firstContentElement" doesn't work well when the modal lacks interactive elements. I propose updating the ActionModal component to use a priority-based focus strategy:

• Use "firstElement" when the modal has form inputs (ensuring immediate input focus and respecting the changes made in Align naming modals #62788)
• Fall back to "firstContentElement" for modals with headers but no inputs
• Use true as a final fallback to focus the modal itself

Something like this should work and provide a better focus on different scenarios. After finalizing the approach and outcomes maybe I can take this up. Better approaches are always welcomed :)

[yogeshbhutkar]

Thanks for the issue!

The diagnosis above is accurate. However, the focus behavior, which is currently hardcoded within the Action Modal's implementation of the Modal component, should instead be exposed via props to consumers. This makes the issue closely related to #69294.

Rather than selectively exposing props and adding redundancy each time the Modal component requires customization, a more efficient approach would be to extend the Modal props within the Action Modal interface, as Action Modal simply acts as a wrapper around Modal. Specifically, this:

Diff

diff --git a/packages/dataviews/src/components/dataviews-item-actions/index.tsx b/packages/dataviews/src/compo
nents/dataviews-item-actions/index.tsx
index 70df04e433..2b6e4e94a7 100644
--- a/packages/dataviews/src/components/dataviews-item-actions/index.tsx
+++ b/packages/dataviews/src/components/dataviews-item-actions/index.tsx
@@ -110,11 +110,10 @@ export function ActionModal< Item >( {
                        title={ action.modalHeader || label }
                        __experimentalHideHeader={ !! action.hideModalHeader }
                        onRequestClose={ closeModal }
-                       focusOnMount="firstContentElement"
-                       size="medium"
                        overlayClassName={ `dataviews-action-modal dataviews-action-modal__${ kebabCase(
                                action.id
                        ) }` }
+                       { ...action.modalProps }
                >
                        <action.RenderModal items={ items } closeModal={ closeModal } />
                </Modal>
diff --git a/packages/dataviews/src/types.ts b/packages/dataviews/src/types.ts
index 8ea13ed0b4..0dcf6ac2d5 100644
--- a/packages/dataviews/src/types.ts
+++ b/packages/dataviews/src/types.ts
@@ -7,6 +7,7 @@ import type { ReactElement, ComponentType } from 'react';
  * Internal dependencies
  */
 import type { SetSelection } from './private-types';
+import type { ModalProps } from '../../components/src/modal/types';
 
 export type SortDirection = 'asc' | 'desc';
 
@@ -463,6 +464,14 @@ export interface ActionModal< Item > extends ActionBase< Item > {
         * The header of the modal.
         */
        modalHeader?: string;
+
+       /**
+        * The props to pass to the modal.
+        */
+       modalProps?: Omit<
+               ModalProps,
+               keyof ActionBase< Item > | 'children' | 'onRequestClose'
+       >;
 }
 
 export interface ActionButton< Item > extends ActionBase< Item > {

I wonder if the decision to avoid this was intentional. If so, I’d love to understand the reasoning behind it. Either way, clarification on whether to expose the entire modalProps or just focusOnMount would help me proceed with creating a PR.

Thanks!

[afercia]

From an accessibility perspective, modal dialogs should always open on explicit user action e.g. users click a button, a modal dialog opens. When closing the modal dialog, the expected behavior is that focus must be returned back to the control that opened the modal dialog. That is one of the reasons why there should be a button.
Without a specific 'opener' interactive element, whe the modal dialog closes then focus should be managed anyways to avoid a focus loss and should be moved back to the most reasonable focusable element in the UI, the one that makes most sense fo the users flow. But that's difficult to determine programmatically and anyways based on assumptions on a specific user's flow.
For these reasons, I'd argue that modal dialogs without a button that opens them should not be allowed.

Regarding focus on mount, I think that should be discussed in a separate issue specific to the Modal component behavior. Initial focus on the Modal component has been discussed already a few times. In a few wordsL sometimes setting initial focus on the first focusable element may not be ideal. That is why focusOnMount has an option to set focus on first element or the container. That's aligned with the new behavior inmplemented for the HTML dialog element initial focus.
References for history:
https://github.com/whatwg/html/wiki/dialog--initial-focus,-a-proposal
whatwg/html#8199

[yogeshbhutkar]

For these reasons, I'd argue that modal dialogs without a button that opens them should not be allowed.

@afercia, I believe when the issue states to Remove the buttons from the RenderModal component., we are exclusively removing the buttons placed within the modal, specifically these:

gutenberg/packages/dataviews/src/components/dataviews/stories/fixtures.tsx

Lines 592 to 605 in d9fa3ac

	<Button
	__next40pxDefaultSize
	variant="tertiary"
	onClick={ closeModal }
	>
	Cancel
	</Button>
	<Button
	__next40pxDefaultSize
	variant="primary"
	onClick={ closeModal }
	>
	Delete
	</Button>

The first paragraph gives the impression that you might be referring to the removal of the opener interactive element, but that's not the case. The opener remains intact; only the buttons inside the modal are removed. (Or was I not able to understand your comment correctly?)

The problem mentioned in this issue occurs because the Modal which is implemented by ActionModal, hardcodes focusOnMount to firstContentElement. Since the buttons inside the modal are removed, the focus gets lost.

See:

gutenberg/packages/dataviews/src/components/dataviews-item-actions/index.tsx

Line 113 in e757d1d

focusOnMount="firstContentElement"

Currently, ActionModal lacks several props that the underlying Modal component supports. To effectively address this and other limitations that might be introduced because of this in the future, it's best to extend ActionModal's types to include all props supported by Modal.

[afercia]

@yogeshbhutkar thanks for letting me better understand the underlying issue.

The problem mentioned in this issue occurs because the Modal which is implemented by ActionModal, hardcodes focusOnMount to firstContentElement. Since the buttons inside the modal are removed, the focus gets lost.

Then I would say the root problem is that firstContentElement should not be hardcoded in the first place. As you pointed out, passing all the props the Modal component needs would be best.

firstContentElement assumes the Modal dialog content has at lease one focusable element, otherwise it will fail. I see @ciampo mentioned this edge case when firstContentElement was added in #54590 (comment). The Modal component documentation clearly states that when firstContentElement is used, then it is the responsibility of the consumer to ensure there is at least one tabbable element within the children or the focus will be lost.

As such, this DataViews implementation doesn't fully follow the Modal documented usage. It should either enforce that the Modal content always contains at least one focusable element or alternatively focusOnMount should be set to firstElement or true (that is just using focusOnMount with no value as it's a boolean prop).

To my understanding, the DataViews package is meant to be a generic package that can also be used outside of the WordPress contexts. As such, ActionModal should always work regardless of whether the modal dialog contains focusable elements or not. In this scenario, setting focusOnMount to true seems the most reasonable option, for now.

Question:
I'm assuming the 'Delete item' example is used here only to illustrate this edge case?

I'm asking because such a modal dialog that asks to perform or abort an action wouldn't be usable without explicit Cancel / Ok buttons.

[afercia]

Hope you all don't mind that I updated this issue title to better illustrate i's about the modal content not containing focusable elements.

[yogeshbhutkar]

Question:
I'm assuming the 'Delete item' example is used here only to illustrate this edge case?

Yes, this seems like the simplest way to reproduce the underlying edge case. Thanks for the insights, @afercia!