ActionModal: Add support for customisable focusOnMount
#69609
Conversation
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| @@ -110,7 +110,7 @@ export function ActionModal< Item >( { | |||
| title={ action.modalHeader || label } | |||
| __experimentalHideHeader={ !! action.hideModalHeader } | |||
| onRequestClose={ closeModal } | |||
| focusOnMount="firstContentElement" | |||
| focusOnMount={ action.modalFocusOnMount } | |||
There was a problem hiding this comment.
Is the code actually defaulting focusOnMount to true?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I'd rather have an explicit default value in case the underlying Modal implementation ever changes
There was a problem hiding this comment.
Thanks! Implemented this in 1e79d9f
There was a problem hiding this comment.
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?
Absolutely! If the proposal is accepted, implementing this will be essential to preserving existing functionality. |
4a9ec79 to
67ec546
Compare
I felt it was best to include this change in this PR. I've reviewed all instances and added the P.S. The failing tests are unrelated to this changeset. |
There was a problem hiding this comment.
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">d53b52f to
56436aa
Compare
|
Thanks for the review, @t-hamano! I've implemented your suggestions. |
…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>
What?
Closes #69265
This PR makes the hardcoded
focusOnMountprop onActionModalcustomisable.Why?
Hardcoding
focusOnMounttofirstContentElementprioritizes focusing on the first focusable element. However, if no such element exists, the focus is lost, preventing theEsckey from closing the modal.To avoid this, it's best to give developers control over
focusOnMount, as relying solely onfirstContentElementmay not always be feasible.Defaulting the
focusOnMountto eitherfirstElementorfirstContentElementis debatable, but there were strong arguments made stating that theActionModalshould work always as expected by default. (Ref. #69265 (comment)).How?
A new prop
modalFocusOnMountis exposed.Testing Instructions
npm run storybook:dev).buttonsfromRenderModalhere:gutenberg/packages/dataviews/src/components/dataviews/stories/fixtures.tsx
Line 578 in 8898589
esckeypress to close the modal works as expected.