Allow Modals to optionally ignore the Close button when determining where to place focus #54296
Allow Modals to optionally ignore the Close button when determining where to place focus #54296
Close button when determining where to place focus #54296Conversation
|
Also pinging @andrewhayward who was involved in original discussions. |
| timerId.current = setTimeout( () => { | ||
| const tabbables = focus.tabbable.find( node ); | ||
|
|
||
| const elementToFocus = focusOnMountRef.current( tabbables ); |
There was a problem hiding this comment.
What should happen if tabbables or elementToFocus is empty?
There was a problem hiding this comment.
Focus on close? I figure this works for when modal is a dialog for info only?
nothing.
There was a problem hiding this comment.
I wonder if we should provide a better DX by console.info or something? Might be overthinking...
| // Modals should ignore the `Close` button which is the first focusable element. | ||
| // Remap `true` to select the next focusable element instead. | ||
| const focusOnMountRef = useFocusOnMount( | ||
| focusOnMount === true ? focusFirstNonCloseButtonElement : focusOnMount |
There was a problem hiding this comment.
I wonder if we should throw an error or something because this code inadvertantly supports functions as props for the Modal component.
There was a problem hiding this comment.
what if the focusOnMount prop of the Modal component also supported passing a callback, same way as the hook?
There was a problem hiding this comment.
It did that until 1f71011 😄
See #54296 (comment)
There was a problem hiding this comment.
I wanted to avoid opening too much APIs but I guess I may be on the minority here. It's fine.
There was a problem hiding this comment.
I see!
To recap: until this PR, if I'm not mistaken:
- passing
falsewould disable the focus on mount functionality - passing
truewould focus theModal's content wrapper - passing
firstElementwould focus the first focusable element in terms of DOM order inside theModal(ie. usually the close button)
What we could do, could be to:
- preserve same behaviour when passing
trueorfalse - tweak the behaviour when passing
firstElement, focusing the first element ignoring the header (as suggested in Allow Modals to optionally ignore theClosebutton when determining where to place focus #54296 (comment)) - optionally, also add support for passing a callback instead, thus enabling even more flexibility
We could also start without the callback option and do everything else, and add the callback only if we find it to be needed in real world usage.
There was a problem hiding this comment.
add the callback only if we find it to be needed in real world usage.
I'd agree with that.
...tweak the behaviour when passing firstElement, focusing the first element ignoring the header
Is it counter intuitive to have firstElement not actually focus the first element though?
I wanted to avoid opening too much APIs but I guess I may be on the minority here. It's fine.
I would also like to avoid opening up too many APIs. Let's start with the minimal changes we need to allow us to address the issue and then consider opening up more if/when use cases become apparent.
There was a problem hiding this comment.
Is it counter intuitive to have firstElement not actually focus the first element though?
We could specify that it will focus the first element inside the Modal's content (ie. excluding the header)?
I would also like to avoid opening up too many APIs. Let's start with the minimal changes we need to allow us to address the issue and then consider opening up more if/when use cases become apparent.
Sounds good.
There was a problem hiding this comment.
tweak the behaviour when passing firstElement, focusing the first element ignoring the header
We could specify that it will focus the first element inside the Modal's content (ie. excluding the header)?
Yes, that's my thinking too. Changing the "true" behavior is more disruptive and is harder to explain.
There was a problem hiding this comment.
I've updated the code. However the following comment still applies right?
I wonder if we should throw an error or something because this code inadvertantly supports functions as props for the Modal component.
So if someone passes a function as focusOnMount to the Modal shall we coerce to true and then console.warn or just throw?
There was a problem hiding this comment.
Both the docs and the TypeScript types should suggest that the focusOnMount prop doesn't support a function — in that sense, I don't think that any action is required. If folks want to try hacking around and pass a function, they're deciding to use the component in an unsupported way and they may incur in runtime errors.
The same would happen on any other component when passing prop values that are not supported
|
I said this in a comment, but just for greater visibility, is it explicitly the 'close' button we want to skip, or any actionable header item? Is the intent actually to skip anything in the "top corner", and find the first focusable "content" node?
|
|
One more thought — is it possible that a Basically, we should not "ignore" the close button if it's only available tabbable element. |
@ciampo Yes I agree. As mentioned under |
Close button when determining where to place focus Close button when determining where to place focus
There was a problem hiding this comment.
Note with this change consumers of Modal will have to opt into this behaviour by passing firstElement. The default of Modal is true.
|
Size Change: +1.26 kB (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
f506d7e to
0403a21
Compare
|
Flaky tests detected in 9fec0cf. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6170906328
|
There was a problem hiding this comment.
Great job so far! I tested quickly on my local storybook instance and the Modal component seems to behave as advertised.
I've got a couple of extra comments, on top of the inline ones:
- We could improve the Storybook controls to make it easier to test for different values of
focusOnMount
Here's how:
diff --git a/packages/components/src/modal/stories/index.story.tsx b/packages/components/src/modal/stories/index.story.tsx
index 8405a6eb01..fe43db1ad6 100644
--- a/packages/components/src/modal/stories/index.story.tsx
+++ b/packages/components/src/modal/stories/index.story.tsx
@@ -28,7 +28,8 @@ const meta: Meta< typeof Modal > = {
control: { type: null },
},
focusOnMount: {
- control: { type: 'boolean' },
+ options: [ true, false, 'firstElement' ],
+ control: { type: 'select' },
},
role: {
control: { type: 'text' },
- I think that we should add a couple of unit tests to certify the behaviour:
focusOnMount = truefocuses the modal wrapperfocusOnMount = falsedoesn't move focusfocusOnMount = firstElementmoves focus to the first element in the modal content (ignoring header)focusOnMount = firstElementmoves focus to the close button if there aren't any focusable elements in the modal content
| // Modals should ignore the `Close` button which is the first focusable element. | ||
| // Remap `true` to select the next focusable element instead. | ||
| const focusOnMountRef = useFocusOnMount( | ||
| focusOnMount === true ? focusFirstNonCloseButtonElement : focusOnMount |
There was a problem hiding this comment.
Both the docs and the TypeScript types should suggest that the focusOnMount prop doesn't support a function — in that sense, I don't think that any action is required. If folks want to try hacking around and pass a function, they're deciding to use the component in an unsupported way and they may incur in runtime errors.
The same would happen on any other component when passing prop values that are not supported
I'd not really thought about it until it was spelled out explicitly, but what is the scenario that we're supporting where focus shouldn't move ( |
|
Note to self that I still need to update the Storybook entries. Also to fix the e2e test. |
|
@alexstine Just flagging this PR. This allows consumers of We've achieved this by changing the behaviour of The behaviour of Note that contributors have to opt into this behaviour by passing |
Co-authored-by: Ben Dwyer <ben@scruffian.com>
ciampo
left a comment
There was a problem hiding this comment.
Code changes LGTM and test well as per instructions 🚀
Left a couple of extra comments, feel free to merge once those are addressed :)
Thank you for working on this!
|
|
||
| ## Unreleased | ||
|
|
||
| ### Breaking changes |
There was a problem hiding this comment.
I'm actually not sure I would call this a breaking change, maybe more of an enhancement?
|
|
||
| // If focusOnMount is `firstElement`, Modals should ignore the `Close` button which is the first focusable element. | ||
| // Remap `true` to select the next focusable element instead. | ||
| const focusOnMountRef = useFocusOnMount( |
There was a problem hiding this comment.
Forgive me if it's already mentioned above but it's a long PR and I'm not on my laptop 😅 .
Could we instead just assign the focusOnMountRef to the <div> with childrenContainerRef on L320 when focusOnMount === 'firstElement'? This way we avoid the use-focusOnMount API change and the awkward CSS class name selector etc.
There was a problem hiding this comment.
That would also change the focus behaviour when the focusOnMount prop is set to true, because the children container would gain focus instead of the dialog container — in my opinion, that behaviour is not correct as we'd like to focus the dialog component.
There was a problem hiding this comment.
when
focusOnMount === 'firstElement'
I mean only change the ref callback to be assigned to the children container div if and only if foucsOnMount === 'firstElement'. Is it still going to be incorrect?
There was a problem hiding this comment.
when
focusOnMount === 'firstElement'I mean only change the ref callback to be assigned to the children container div if and only if
foucsOnMount === 'firstElement'. Is it still going to be incorrect?
I wouldn't know off the top of my head, but I guess we could try the approach keeping the current set of unit tests and see how it goes.
There was a problem hiding this comment.
Would this give us the flexibility to find the first focusable node outside of the content wrapper if there's nothing appropriate within it? Feels like we still need the "global" context, and moving the ref would take that away.
There was a problem hiding this comment.
True! FWIW, I would prefer to add a new type like firstContentElement instead of patching firstElement. It's also clearer for folks that don't read the doc/changelog or have false (but fair) assumptions.
That said, I don't really have a strong preference here. I'm just sharing a suggestion, and I'm okay with whatever solution we end up with! ❤️
There was a problem hiding this comment.
I would prefer to add a new type like
firstContentElementinstead of patchingfirstElement
That could also be an option, I wouldn't be opposed to that.
There was a problem hiding this comment.
Using firstContentElement would avoid regressions. I'm tempted to try that approach next week alongside the suggestion to conditionally move the ref when firstContentElement is set.
I knew this wouldn't be easy 😅
There was a problem hiding this comment.
You all are doing a great job! Happy to help in any way when I have the capacity! 💪
There was a problem hiding this comment.
@kevin940726 Alternative now available in #54590
|
Closing in favour of #54590 |
What?
Updates all instances of
<Modal>in order that they ignore theClosebutton when determining where to place focus when mounting.Alternative to #54590
Closes #54106
Why?
As discussed in #54106 dialogs (including Modal) should ideally place focus on the first focusable element on mount. Currently all Modals find that the
Closebutton is the first element in the DOM inside the Modal that is focusable and thus it receives focus. A11y feedback has been that this is unhelpful.How?
Updates the
useFocusOnMounthook API to (optionally) accept a callback which is passed a list of "tabbable" elements within the Modal. The consumer can then use this to programmatically select the element which should received focus.Previously the API only allowed for "first element".
The
<Modal>component is then updated to utilise this API to ensure that it selects the first focusable element which is not theClosebuttonThis appraoch is inline with that @ciampo suggested in #54106 (comment).
Questions/Concerns
Closebutton outside of thefocusOnMountref whilst remaining within the constrained tabbing ref. @ciampo felt this would be very difficult however.Closebutton? Is that the developer's problem? Or should we focus theClosebutton in that case?focusOnMount.currentis not callable?setTimeout()portion?Testing Instructions
Rename.inputand not onClosebuttonClosebutton and constrained tabbing is still activeTesting Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2023-09-08.at.11-43-30.mp4