Skip to content

ActionModal: Add support for customisable focusOnMount #69609

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 5, 2025
Merged

ActionModal: Add support for customisable focusOnMount #69609

merged 4 commits into from
Apr 5, 2025

Conversation

@yogeshbhutkar
Copy link
Contributor

@yogeshbhutkar yogeshbhutkar commented Mar 18, 2025

What?

Closes #69265

This PR makes the hardcoded focusOnMount prop on ActionModal customisable.

Why?

Hardcoding focusOnMount to firstContentElement prioritizes focusing on the first focusable element. However, if no such element exists, the focus is lost, preventing the Esc key from closing the modal.

To avoid this, it's best to give developers control over focusOnMount, as relying solely on firstContentElement may not always be feasible.

Defaulting the focusOnMount to either firstElement or firstContentElement is debatable, but there were strong arguments made stating that the ActionModal should work always as expected by default. (Ref. #69265 (comment)).

How?

A new prop modalFocusOnMount is exposed.

Testing Instructions

  1. Start the storybook development server (npm run storybook:dev).
  2. Remove buttons from RenderModal here:

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

    Line 578 in 8898589

    export const actions: Action< SpaceObject >[] = [
  3. Confirm that the esc keypress to close the modal works as expected.

@yogeshbhutkar yogeshbhutkar marked this pull request as ready for review March 18, 2025 12:35
@github-actions
Copy link

github-actions bot commented Mar 18, 2025 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: yogeshbhutkar <yogeshbhutkar@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: himanshupathak95 <abcd95@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
Co-authored-by: oandregal <oandregal@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Mar 19, 2025
ciampo
ciampo reviewed Mar 19, 2025
View reviewed changes
packages/dataviews/src/components/dataviews-item-actions/index.tsx Outdated
__experimentalHideHeader={ !! action.hideModalHeader }
onRequestClose={ closeModal }
focusOnMount="firstContentElement"
focusOnMount={ action.modalFocusOnMount }
Copy link
Contributor

@ciampo ciampo Mar 19, 2025

Choose a reason for hiding this comment

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

Is the code actually defaulting focusOnMount to true?

Copy link
Contributor Author

@yogeshbhutkar yogeshbhutkar Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@ciampo, if no value is provided, the internal Modal component defaults focusOnMount to true. I didn’t explicitly add { action.modalFocusOnMount ?? true } for this reason.

Ref.

gutenberg/packages/components/src/modal/index.tsx

Line 59 in ceab31d

focusOnMount = true,

Copy link
Contributor

@ciampo ciampo Mar 19, 2025

Choose a reason for hiding this comment

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

I'd rather have an explicit default value in case the underlying Modal implementation ever changes

Copy link
Contributor Author

@yogeshbhutkar yogeshbhutkar Mar 31, 2025

Choose a reason for hiding this comment

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

Thanks! Implemented this in 1e79d9f

t-hamano
t-hamano reviewed Mar 19, 2025
View reviewed changes
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This PR changes the default value of the internal focusOnMount property of the ActionModal component.

I think we need to investigate all DataViews components already used in Gutenberg and adjust them to work as before. In other words, I think we need to add focusOnMount: 'firstContentElement' to all Actions. What do you think?

@yogeshbhutkar
Copy link
Contributor Author

yogeshbhutkar commented Mar 19, 2025

What do you think?

Absolutely! If the proposal is accepted, implementing this will be essential to preserving existing functionality.

@yogeshbhutkar yogeshbhutkar force-pushed the fix-69265/dataviews-focus-on-mount branch from 4a9ec79 to 67ec546 Compare March 31, 2025 08:26
@yogeshbhutkar
Copy link
Contributor Author

yogeshbhutkar commented Mar 31, 2025

I think we need to investigate all DataViews components already used in Gutenberg and adjust them to work as before. In other words, I think we need to add focusOnMount: 'firstContentElement' to all Actions.

I felt it was best to include this change in this PR. I've reviewed all instances and added the firstContentElement prop to the necessary actions in commit 67ec546. Please take a look when you have a moment.

P.S. The failing tests are unrelated to this changeset.

CC: @t-hamano, @ciampo

t-hamano
t-hamano approved these changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for the update! I believe we are ready to ship.

Nit: Can we update the DataViews story as well?

diff --git a/packages/dataviews/src/components/dataviews/stories/fixtures.tsx b/packages/dataviews/src/components/dataviews/stories/fixtures.tsx
index cd4d3b97e6..9b819dbbc0 100644
--- a/packages/dataviews/src/components/dataviews/stories/fixtures.tsx
+++ b/packages/dataviews/src/components/dataviews/stories/fixtures.tsx
@@ -582,6 +582,7 @@ export const actions: Action< SpaceObject >[] = [
                isPrimary: true,
                icon: trash,
                hideModalHeader: true,
+               modalFocusOnMount: 'firstContentElement',
                RenderModal: ( { items, closeModal } ) => {
                        return (
                                <VStack spacing="5">

image

@yogeshbhutkar yogeshbhutkar force-pushed the fix-69265/dataviews-focus-on-mount branch from d53b52f to 56436aa Compare April 3, 2025 12:46
@yogeshbhutkar
Copy link
Contributor Author

yogeshbhutkar commented Apr 3, 2025

Thanks for the review, @t-hamano! I've implemented your suggestions.

@t-hamano t-hamano merged commit da7adc0 into WordPress:trunk Apr 5, 2025
59 checks passed
@github-actions github-actions bot added this to the Gutenberg 20.7 milestone Apr 5, 2025
chriszarate pushed a commit to chriszarate/gutenberg that referenced this pull request Jul 1, 2025
@yogeshbhutkar @ciampo
@t-hamano @afercia @oandregal @chriszarate
ActionModal: Add support for customisable focusOnMount (WordPress#6…
5a95a45 
…9609)

* feat: support customisable `focusOnMount`

* fix: ensure `focusOnMount` defaults to true in ActionModal

* feat: add `modalFocusOnMount` property to all eligible actions

* feat: add `modalFocusOnMount` property to ActionModal fixture

Co-authored-by: yogeshbhutkar <yogeshbhutkar@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: himanshupathak95 <abcd95@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
Co-authored-by: oandregal <oandregal@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t-hamano t-hamano t-hamano approved these changes

@Mamaduka Mamaduka Awaiting requested review from Mamaduka

@oandregal oandregal Awaiting requested review from oandregal oandregal is a code owner

@ntsekouras ntsekouras Awaiting requested review from ntsekouras ntsekouras is a code owner

@youknowriad youknowriad Awaiting requested review from youknowriad

@ciampo ciampo Awaiting requested review from ciampo

Assignees

No one assigned

Labels

[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Projects

None yet

Milestone

Gutenberg 20.7

Development

Successfully merging this pull request may close these issues.

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

4 participants

@yogeshbhutkar @ciampo @t-hamano @Mamaduka